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

Add congestion info script #12457

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

Conversation

telezhnaya
Copy link
Contributor

@telezhnaya telezhnaya commented Nov 14, 2024

Add congestion info script. It collects all the data between the given interval, going from the end till start (because we have prev_height and we don't have next_height available)
The interval can be of any length, and the process can be stopped in the middle, all the collected data would be saved.

Attaching example of collected data, please share if this is enough for further investigation.
congestion_data.csv

@telezhnaya telezhnaya force-pushed the telezhnaya/collect_congestion_info branch 2 times, most recently from 21ae6c8 to 50e9ad3 Compare November 14, 2024 14:08
@telezhnaya telezhnaya force-pushed the telezhnaya/collect_congestion_info branch from 50e9ad3 to 7e89ed0 Compare November 14, 2024 14:09
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.46%. Comparing base (fecce01) to head (7e89ed0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12457      +/-   ##
==========================================
+ Coverage   71.44%   71.46%   +0.01%     
==========================================
  Files         838      838              
  Lines      169339   169339              
  Branches   169339   169339              
==========================================
+ Hits       120985   121015      +30     
+ Misses      43006    42974      -32     
- Partials     5348     5350       +2     
Flag Coverage Δ
backward-compatibility 0.16% <ø> (ø)
db-migration 0.16% <ø> (ø)
genesis-check 1.29% <ø> (ø)
integration-tests 39.35% <ø> (+<0.01%) ⬆️
linux 70.79% <ø> (+<0.01%) ⬆️
linux-nightly 71.01% <ø> (+0.01%) ⬆️
macos 51.02% <ø> (+<0.01%) ⬆️
pytests 1.60% <ø> (ø)
sanity-checks 1.40% <ø> (ø)
unittests 63.98% <ø> (+0.01%) ⬆️
upgradability 0.21% <ø> (ø)

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.

@telezhnaya telezhnaya requested a review from wacban November 14, 2024 14:35
@telezhnaya telezhnaya marked this pull request as ready for review November 14, 2024 14:35
@telezhnaya telezhnaya requested a review from a team as a code owner November 14, 2024 14:35
@wacban
Copy link
Contributor

wacban commented Nov 15, 2024

@telezhnaya Would you like me to merge this or shall we wait for the congestion level support first and do it this way?

@wacban
Copy link
Contributor

wacban commented Nov 15, 2024

I guess my question is whether you would like me to review it now or later :)

@telezhnaya telezhnaya force-pushed the telezhnaya/collect_congestion_info branch from bfbcbbf to b424c66 Compare November 24, 2024 17:49
Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

overall looks good but there are some minor bugs, please see comments

if not current_block["header"]["height"] % 10:
print(current_block["header"]["height"])
for chunk in current_block["chunks"]:
shard_info = ShardCongestionInfo(**chunk["congestion_info"])
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Comment on lines +79 to +80
if args.shard_ids and shard_info.allowed_shard not in args.shard_ids:
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not right, we want to filter by the shard id, not the allowed shard id. I think the shard id should be in the chunk.

"height"]:
if not current_block["header"]["height"] % 10:
print(current_block["header"]["height"])
for chunk in current_block["chunks"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you happen to know how this works for missing chunks? Does it just have the old chunk data? Sanity check - is that what we want?



@dataclass(frozen=True)
class ShardCongestionInfo:
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we going to use the simpler congestion level?

Comment on lines +240 to +247
#[allow(non_snake_case)]
pub fn EXPERIMENTAL_congestion_level(
&self,
request: RpcCongestionLevelRequest,
) -> RpcRequest<RpcCongestionLevelResponse> {
call_method(&self.client, &self.server_addr, "EXPERIMENTAL_congestion_level", request)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that the congestion level isn't currently available via json rpc?

Comment on lines +20 to +21
let block =
client.block(BlockReference::BlockId(BlockId::Height(current_height))).await.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

What about missing blocks?

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