-
Notifications
You must be signed in to change notification settings - Fork 683
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
if (numBlocks !== null) { | ||
params.append('num_blocks', numBlocks.toString()); | ||
} | ||
const url = `http://${addr}/debug/api/block_status${params.toString() ? '?' + params : ''}`; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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])?; |
There was a problem hiding this comment.
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
There were couple things which limited my debugging experience and I want to improve it:
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 infind_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 callget_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.