Skip to content

Conversation

@boquan-fang
Copy link
Contributor

@boquan-fang boquan-fang commented Nov 5, 2025

Release Summary:

Implement s2n-quic-dc-metrics crate 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:

  • The implementation has some methods that are only stable after rust version 1.88.0. Hence, I made the decision to keep the MSRV for s2n-quic and s2n-quic-dc to be 1.84.0, and make the MSRV for the new s2n-quic-dc-metrics crate to be 1.88.0.
    • Specifically for this:
      --> dc/s2n-quic-dc-metrics/src/counter.rs:29:37
       |
    29 |         let (chunks, tail) = events.as_chunks::<8>();
    
    • Because of that decision, I have to tweak our CI a little bit:
      • Update the nightly toolchain to be nightly-2025-05-29 which will include the as_chunks method.
      • Create a .clippy.toml file for the new crate specifically so that the rust version will be 1.88.0 for the new crate during clippy cheks.
      • Skip the new crate when the rust version is 1.84.0 (s2n-quic's MSRV).

Testing:

CI test should include all of s2n-quic-dc-metrics's tests, and they should pass.

     Running unittests src/lib.rs (target/debug/deps/s2n_quic_dc_metrics-ca2245c14d8becc3)
running 22 tests
test appender::test::flush_on_drop ... ok
test appender::test::test_reuse ... ok
test appender::test::test_rotation ... ok
test appender::test::test_configured_rotation ... ok
test appender::test::test_time_rollback ... ok
test rseq::tests::check_per_cpu ... ok
test rseq::tests::test_send_event_local ... ok
test rseq::tests::check_send_branches ... ok
test rseq::tests::check_send_slow_branches ... ok
test summary::bucket::tests::idx_to_lower_bound ... ok
test summary::bucket::tests::idx_to_upper_bound ... ok
test rseq::tests::test_thread_ctor_dtor ... ok
test summary::bucket::tests::sizes ... ok
test summary::bucket::tests::total_buckets ... ok
test summary::bucket::tests::value_to_idx ... ok
test summary::test::config ... ok
test summary::test::count_correct ... ok
test counter::basic ... ok
test summary::test::maximum ... ok
test rseq::tests::test_send_event_overflow ... ok
test test::counters_for_enum_counts ... ok
test summary::test::visits_all_buckets ... ok
test result: ok. 22 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s

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.

@boquan-fang boquan-fang marked this pull request as ready for review November 5, 2025 21:47
@boquan-fang boquan-fang requested a review from a team as a code owner November 5, 2025 21:47
@Mark-Simulacrum
Copy link
Collaborator

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.

@boquan-fang boquan-fang force-pushed the boquan-fang/impl-metrics branch from a5f216f to d14936d Compare November 6, 2025 23:28
@boquan-fang boquan-fang force-pushed the boquan-fang/impl-metrics branch from ef401c4 to 096f981 Compare November 7, 2025 07:57
# 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
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

@boquan-fang boquan-fang Nov 10, 2025

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
Copy link
Contributor

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?

Copy link
Contributor Author

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:

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.

Copy link
Contributor Author

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.

* update nightly toolchain
* Cargo toml project comments
* update copyright
@boquan-fang boquan-fang force-pushed the boquan-fang/impl-metrics branch from a232e04 to 3920668 Compare November 10, 2025 18:17
env: [default]
include:
- rust: ${{ needs.env.outputs.msrv }}
# s2n-quic-dc-metrics crate' MSRV is 1.88.0, which is higher than the rest
Copy link
Contributor

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?

Copy link
Contributor Author

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
@boquan-fang boquan-fang force-pushed the boquan-fang/impl-metrics branch 2 times, most recently from 04318e1 to a0fc2ff Compare November 10, 2025 21:53
@boquan-fang boquan-fang force-pushed the boquan-fang/impl-metrics branch from a0fc2ff to ec9f6a6 Compare November 10, 2025 21:54
@boquan-fang boquan-fang merged commit 80f7de4 into main Nov 10, 2025
136 checks passed
@boquan-fang boquan-fang deleted the boquan-fang/impl-metrics branch November 10, 2025 22:25
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.

3 participants