-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_network: broadcast network stress test metrics #9461
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
018e1b1 to
00e689e
Compare
8d147d2 to
14d46fe
Compare
sirandreww-starkware
left a comment
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.
This PR adds metrics that are collected by the stress test.
it add a dependency on sysinfo and thus requires approval from some other team lead, who should I add?
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @ShahakShama)
|
Benchmark movements: No major performance changes detected. |
00e689e to
7dc7edc
Compare
14d46fe to
1677cf0
Compare
ShahakShama
left a comment
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.
@ShahakShama reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sirandreww-starkware)
crates/apollo_network/Cargo.toml line 42 at r2 (raw file):
strum.workspace = true strum_macros.workspace = true sysinfo.workspace = true
Add this as bin dependencies
Alternatively, move all the stress test code to a different crate
Cargo.toml line 323 at r2 (raw file):
strum_macros = "0.25.2" syn = "2.0.39" sysinfo = "0.37.1"
Following the 3rd party discussion, @dan-starkware should approve this
crates/apollo_network/src/bin/broadcast_network_stress_test_node/main.rs line 98 at r2 (raw file):
broadcast_topic_client.broadcast_message(message.clone()).await.unwrap(); trace!("Sent message {i}: {:?}", message); // counter!("sent_messages").increment(1);
Delete instead of commenting out
crates/apollo_network/src/bin/broadcast_network_stress_test_node/main.rs line 117 at r2 (raw file):
let _delay_seconds = duration.as_secs_f64(); // let delay_micros = duration.as_micros().try_into().unwrap();
Same here
crates/apollo_network/src/bin/broadcast_network_stress_test_node/metrics.rs line 27 at r2 (raw file):
define_metrics!( Infra => { MetricGauge { BROADCAST_MESSAGE_HEARTBEAT_MILLIS, "broadcast_message_theoretical_heartbeat_millis", "The number of ms we sleep between each two consecutive broadcasts" },
Consider adding some word that makes it clear these are the setup/config of the scenario
(If broadcast was supposed to be that word then it's not a good word)
crates/apollo_network/src/bin/broadcast_network_stress_test_node/metrics.rs line 83 at r2 (raw file):
}; // Use apollo_metrics for all metrics including labeled ones
This comment feels redundant
crates/apollo_network/src/bin/broadcast_network_stress_test_node/metrics.rs line 90 at r2 (raw file):
); // Use apollo_metrics histograms for latency measurements
This comment feels redundant
crates/apollo_network/src/bin/broadcast_network_stress_test_node/metrics.rs line 155 at r2 (raw file):
interval.tick().await; // Refresh only the specific data we need
Remove commented out code
dan-starkware
left a comment
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.
@dan-starkware reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sirandreww-starkware)
1677cf0 to
00dcd1d
Compare
d6d6346 to
4f96406
Compare
4f96406 to
5827ce7
Compare
5827ce7 to
aec47f8
Compare
aec47f8 to
79e23df
Compare

No description provided.