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

feat(s2n-quic-core): implement nominal timers #2370

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Nov 7, 2024

Description of changes:

This change implements nominal timers for the aggregate metrics. It also adds a checkpoint and nominal_checkpoint attribute to events, which allows them to record how long it took for the event to fire since the start of the connection.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft marked this pull request as ready for review November 7, 2024 22:39
Mark-Simulacrum
Mark-Simulacrum previously approved these changes Nov 7, 2024
@@ -19,6 +19,9 @@ struct PathSecretMapUninitialized {
/// The number of entries in the map
#[measure("entries")]
entries: usize,

#[measure("lifetime")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to check, we auto-unit durations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh we default to None... lemme fix that. good catch!

@Mark-Simulacrum
Copy link
Collaborator

Mark-Simulacrum commented Nov 7, 2024

I wonder if time since connection start is the right metric... Time since previous checkpoint might be more useful? Otherwise e.g. slow p100 on one phase will be hard to tell apart from sometimes slow across all the phases (when looking in aggregate)

Essentially time in phase vs. time overall

@camshaft
Copy link
Contributor Author

camshaft commented Nov 7, 2024

I wonder if time since connection start is the right metric...

Hmm... I think it probably depends on what you're trying to do. For example, if you want to know average latency at which dc_state_changed.secrets_ready is hit, you'd have to sum up all of the previous states since then to get that.

But I can definitely see another type of measurement that records "time since last update"

@camshaft camshaft merged commit ef18f1c into main Nov 8, 2024
130 checks passed
@camshaft camshaft deleted the camshaft/metric-checkpoint branch November 8, 2024 18:08
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