-
Notifications
You must be signed in to change notification settings - Fork 151
feat(s2n-quic-dc): implement metrics for s2n-quic-dc #2884
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
|
re:MSRV, we would need Rust 1.87 for asm goto (https://blog.rust-lang.org/2025/05/15/Rust-1.87.0/#asm-jumps-to-rust-code) regardless, I think, so keeping this at 1.88 seems fine to me. |
a5f216f to
d14936d
Compare
ef401c4 to
096f981
Compare
.github/workflows/ci.yml
Outdated
| # Pin the nightly toolchain to prevent breakage. | ||
| # This should be occasionally updated. | ||
| RUST_NIGHTLY_TOOLCHAIN: nightly-2025-03-31 | ||
| RUST_NIGHTLY_TOOLCHAIN: nightly-2025-06-29 |
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.
I think this can just be updated to the very latest one
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.
Seems like once I update the nightly toolchain to the latest, the miri test on Github for s2n-quic-core will break on the w_cubic test:
thread 'recovery::cubic::tests::w_cubic' (4012) panicked at quic/s2n-quic-core/src/recovery/cubic/tests.rs:53:5:
assertion `left == right` failed
left: 11.999997139s
right: 12s
Seems like the result that we got is 11.99999 which is extremely close to the expected one. I ran it on my local machine and it also failed by a tiny deviation:
thread 'recovery::cubic::tests::w_cubic' (1052900) panicked at quic/s2n-quic-core/src/recovery/cubic/tests.rs:53:5:
assertion `left == right` failed
left: 12.000000954s
right: 12s
I wonder if that would be the reason why we don't use the latest nightly tool. We should probably keep using nightly-2025-06-29 and wait to see if a even newer version will resolve the problem?
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.
I tried different versions of rust nightly toolchain and found out that nightly-2025-08-01 would work for our test. I can update the version to that version. It's not the latest, but close to our goal.
| @@ -0,0 +1,257 @@ | |||
| // Copyright (c) 2022 Tokio Contributors | |||
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.
is our copyright checking script not covering this crate?
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.
According to our copyright check, this would fulfill the requirements:
s2n-quic/scripts/copyright_check
Lines 14 to 16 in cf77e2b
| for file in $S2N_QUIC_FILES; do | |
| # The word "Copyright" should appear at least once in the first 3 lines of every file | |
| COUNT=`head -3 $file | grep "Copyright" | wc -l`; |
The word Copyright is indeed in the first three lines of this file.
We can make improvement to the check: maybe we should check for Copyright Amazon.com, Inc. or its affiliates.? I don't think that is in scope with this PR. I would recommend to manually fix the copyright for this PR, cut a issue to track this problem, and fix it in a separate PR.
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.
I have cut a ticket to track this issue: #2889.
0b463f3 to
a232e04
Compare
* update nightly toolchain * Cargo toml project comments * update copyright
a232e04 to
3920668
Compare
| env: [default] | ||
| include: | ||
| - rust: ${{ needs.env.outputs.msrv }} | ||
| # s2n-quic-dc-metrics crate' MSRV is 1.88.0, which is higher than the rest |
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 open an issue to clean this up when we are on 1.88.0 for the rest of s2n-quic?
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.
Yes, I have created one: #2888.
* I checked the latest version has very slight calculation errors for recovery::cubic::tests::w_cubic test
04318e1 to
a0fc2ff
Compare
a0fc2ff to
ec9f6a6
Compare
Release Summary:
Implement
s2n-quic-dc-metricscrate to emit metrics for s2n-quic-dc.Resolved issues:
Description of changes:
We implement a highly optimized crate to emit metrics for s2n-quic-dc events.
Call-outs:
as_chunksmethod..clippy.tomlfile for the new crate specifically so that the rust version will be 1.88.0 for the new crate during clippy cheks.Testing:
CI test should include all of s2n-quic-dc-metrics's tests, and they should pass.
Also, I test it with deployment.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.