Skip to content

refactor(s2n-events): Reduce generated diffs when changing metric definitions#2993

Merged
Mark-Simulacrum merged 2 commits into
aws:mainfrom
Mark-Simulacrum:reduce-metrics-diffs
Mar 11, 2026
Merged

refactor(s2n-events): Reduce generated diffs when changing metric definitions#2993
Mark-Simulacrum merged 2 commits into
aws:mainfrom
Mark-Simulacrum:reduce-metrics-diffs

Conversation

@Mark-Simulacrum

@Mark-Simulacrum Mark-Simulacrum commented Feb 27, 2026

Copy link
Copy Markdown
Collaborator

Release Summary:

  • feat(s2n-quic-dc): Instrument connect_tls with latency/error event tracking

Resolved issues:

n/a

Description of changes:

Previously the raw integers were embedded in multiple places, and inserting into the middle of the list caused all integers to change. This meant a huge diff for a tiny change, and also increased the risk of merge conflicts. This updates the generator to have a list of constants each with value (previous + 1), which means that any change should only touch ~1 adjacent constant to any newly added metric at most.

This shouldn't have any change in runtime behavior or performance since all offsets/indices remain computed at compile-time.

Call-outs:

n/a

Testing:

There's some unit/integration tests and I've poked through a couple of example events and things look reasonable.

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

@Mark-Simulacrum Mark-Simulacrum marked this pull request as ready for review February 27, 2026 18:54
@Mark-Simulacrum Mark-Simulacrum requested a review from a team as a code owner February 27, 2026 18:54
@Mark-Simulacrum Mark-Simulacrum force-pushed the reduce-metrics-diffs branch 2 times, most recently from a4a9950 to 843f433 Compare March 4, 2026 15:56
@Mark-Simulacrum

Copy link
Copy Markdown
Collaborator Author

I'll rebase this once the generator is approved since it needs rebasing after any event-touching PR.

@boquan-fang boquan-fang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a diff that you can show for this PR? It might be good to have a example of it to show how the generated diff is reduced.

@Mark-Simulacrum

Mark-Simulacrum commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator Author

I pushed a second commit adding an event around s2n-quic-dc TLS stream connects for an example diff. It would've produced a much larger diff (standard ~3k lines changes, see e.g. 4ca17fa for previous example of that kind of change).

Previously the raw integers were embedded in multiple places, and
inserting into the middle of the list caused all integers to change.
This meant a huge diff for a tiny change, and also increased the risk of
merge conflicts. This updates the generator to have a list of constants
each with value (previous + 1), which means that any change should only
touch ~1 adjacent constant to any newly added metric at most.

This shouldn't have any change in runtime behavior or performance since
all offsets/indices remain computed at compile-time.
boquan-fang
boquan-fang previously approved these changes Mar 11, 2026
@Mark-Simulacrum Mark-Simulacrum merged commit 59bb815 into aws:main Mar 11, 2026
165 of 166 checks passed
@Mark-Simulacrum Mark-Simulacrum deleted the reduce-metrics-diffs branch March 11, 2026 23:44
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