apollo_network_benchmark: added broadcast topic metrics#11545
Conversation
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
|
There hasn't been any activity on this pull request recently, and in order to prioritize active work, it has been marked as stale. |
1f0509d to
ab1a950
Compare
602db52 to
3731f9f
Compare
ab1a950 to
2d9dcd6
Compare
2d9dcd6 to
8214b79
Compare
15e099b to
9f2229a
Compare
8214b79 to
5095a65
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
| tps * message_size_bytes.into_f64() | ||
| } | ||
|
|
||
| /// Creates barebones network metrics |
There was a problem hiding this comment.
New broadcast metrics are never registered
Medium Severity
The three newly added metrics — NETWORK_STRESS_TEST_SENT_MESSAGES, NETWORK_STRESS_TEST_RECEIVED_MESSAGES, and NETWORK_DROPPED_BROADCAST_MESSAGES — are defined in the define_metrics! macro and wired into BroadcastNetworkMetrics in create_network_metrics(), but register_metrics() was not updated to call .register() on any of them. Without registration, increments to these counters won't be reported by the metrics system, defeating the purpose of this PR.



Note
Low Risk
Low risk benchmark-only change that wires additional broadcast send/receive/drop counters into
NetworkMetricsfor the stress-test topic; main risk is metric label/mapping mistakes affecting observability, not runtime behavior.Overview
Adds per-topic broadcast observability to the broadcast network stress test node by defining new counters for sent/received messages and a labeled counter for dropped broadcasts (using
NETWORK_BROADCAST_DROP_LABELS).Updates
create_network_metrics()to populateNetworkMetrics.broadcast_metrics_by_topicwith aBroadcastNetworkMetricsentry keyed by the stress-testTOPIChash, so the network manager can record broadcast metrics for that topic.Written by Cursor Bugbot for commit 5095a65. This will update automatically on new commits. Configure here.