Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(debug-ui): extend block status api #12930

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Longarithm
Copy link
Member

@Longarithm Longarithm commented Feb 14, 2025

There were couple things which limited my debugging experience and I want to improve it:

  • Now you can choose how many blocks to display since starting_height. I limit this by 1000 on both backend and frontend for safety.
  • Sometimes I want to find the point where some incident starts. Let's say chunks for some shard are consistently missing. For that I want to give starting_height and ask what was the first block where all chunks were included. I also support the opposite logic - if the incident stopped, find the last height where chunk miss was recorded. This logic is mainly in find_first_height_to_fetch.

Also I fix logic of displaying block if only header is present. Because get_all_header_hashes_by_height data source is GC-d, we should call get_block_hash_by_height instead, which is enough once block is final. get_block_hashes_to_fetch has the logic of getting block hashes.

Preview. Note that starting height in query and first displayed height are different.

image

Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 6.10687% with 123 lines in your changes missing coverage. Please review.

Project coverage is 70.48%. Comparing base (8b3fc09) to head (0207933).

Files with missing lines Patch % Lines
chain/client/src/debug.rs 0.00% 69 Missing ⚠️
chain/client-primitives/src/debug.rs 0.00% 29 Missing ⚠️
chain/jsonrpc/src/lib.rs 26.66% 22 Missing ⚠️
core/store/src/adapter/chain_store.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12930      +/-   ##
==========================================
- Coverage   70.54%   70.48%   -0.06%     
==========================================
  Files         851      852       +1     
  Lines      174983   175096     +113     
  Branches   174983   175096     +113     
==========================================
- Hits       123433   123415      -18     
- Misses      46419    46547     +128     
- Partials     5131     5134       +3     
Flag Coverage Δ
backward-compatibility 0.36% <0.00%> (-0.01%) ⬇️
db-migration 0.36% <0.00%> (-0.01%) ⬇️
genesis-check 1.42% <0.00%> (-0.01%) ⬇️
linux 70.32% <6.10%> (-0.05%) ⬇️
linux-nightly 70.12% <6.10%> (-0.06%) ⬇️
pytests 1.73% <0.00%> (-0.01%) ⬇️
sanity-checks 1.54% <0.00%> (-0.01%) ⬇️
unittests 70.31% <6.10%> (-0.06%) ⬇️
upgradability 0.36% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Longarithm Longarithm marked this pull request as ready for review February 14, 2025 10:43
@Longarithm Longarithm requested a review from a team as a code owner February 14, 2025 10:43
@Longarithm Longarithm changed the title feat(debug-ui): extend api for block status feat(debug-ui): extend block status api Feb 14, 2025
if (numBlocks !== null) {
params.append('num_blocks', numBlocks.toString());
}
const url = `http://${addr}/debug/api/block_status${params.toString() ? '?' + params : ''}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since old neard versions expect /debug/api/block_status/{starting_height}, now the block height text field won't work with a new version of the debug UI but old neard. But I agree that putting the height in the URL params makes more sense, so this breaking change is prob not a huge deal. Idk how worth it it is, but if you want, could consider keeping the old url format with the new params until the old version is prob no longer running anywhere

if block_hashes.is_empty() {
if matches!(
mode,
DebugBlocksStartingMode::JumpToBlockMiss | DebugBlocksStartingMode::JumpToChunkMiss
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's up to you, but I would prob expect JumpToChunkMiss to show me the first existing block w some missing chunk in it

break;
}

let block_header = chain_store.get_block_header(&block_hashes[0])?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a big deal, but maybe check all of them? If there are two and the second one has a missing chunk, maybe we show that for JumpToAllChunksIncluded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants