-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[forge] LatencyBreakdown counters and use them in success criteria #9393
[forge] LatencyBreakdown counters and use them in success criteria #9393
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c91b8ab
to
fee3b95
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fee3b95
to
461bd1d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
461bd1d
to
3e0acf5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
3df527e
to
21b4764
Compare
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.
LGTM % minor nits
Ok(range | ||
.first() | ||
.ok_or_else(|| { | ||
anyhow!( |
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.
You can probably just unwrap()
here as you ensure the length is 1 above.
@@ -216,6 +218,24 @@ struct Resize { | |||
enable_haproxy: bool, | |||
} | |||
|
|||
// common metrics thresholds: | |||
static SYSTEM_12_CORES_5GB_THRESHOLD: Lazy<SystemMetricsThreshold> = Lazy::new(|| { |
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.
nit: Better to move it to system_metrics module
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.
there is no system_metrics module any more :)
thresholds structs are defined in the SuccessCriteria, I don't think there is better. they are only used in this file, seems fine to have constants here.
testsuite/forge-cli/src/main.rs
Outdated
.add_latency_threshold(3.4, LatencyType::P50) | ||
.add_latency_threshold(4.5, LatencyType::P90) | ||
.add_latency_breakdown_threshold(LatencyBreakdownThreshold::new_strict( | ||
0.3, 0.25, 0.8, 0.6, |
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.
Can you change this to a builder pattern so it's clear what these thresholds are and we can extend this easily further...
21b4764
to
00f6842
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
00f6842
to
bad609e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bad609e
to
f6322d5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
…9393) * refactor SystemMetrics to be cleaner, and build LatencyBreakdown metrics in the same flow * have Swarm surface API for generic querying range from prometheus, and move validation and what needs to be fetched to SuccessCriteria * move retries from system metrics alone, to prometheus calls themselves * rename system_metrics.rs to prometheus_metrics.rs - and have fetching of system and latency metrics there * move threshold logic to SuccessCriteria * add fetching QS and consensus latency breakdown, and having a way to assert they pass. * fixing construct_query_with_extra_labels, and updating land_blocking checks
refactor SystemMetrics to be cleaner, and build LatencyBreakdown metrics in the same flow
have Swarm surface API for generic querying range from prometheus, and move validation and what needs to be fetched to SuccessCriteria
move retries from system metrics alone, to prometheus calls themselves
rename system_metrics.rs to prometheus_metrics.rs - and have fetching of system and latency metrics there
move threshold logic to SuccessCriteria
add fetching QS and consensus latency breakdown, and having a way to assert they pass.
Description
Test Plan