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(event): WriteContext::write_ack_frame records events AckRangeSent #1646

Merged
merged 4 commits into from
Mar 2, 2023

Conversation

jon-chuang
Copy link
Contributor

@jon-chuang jon-chuang commented Mar 1, 2023

Resolved issues:

resolves: #1416

Description of changes:

Specialize write_frame for ack frame: write_ack_frame.

Call-outs:

Advantage: essentially zero-cost: write_ack_frame only specializes for Frame::Ack anyway.

Alternatives considered:

  1. expose get_publisher(&self) -> &mut Publisher (requires exposing generic Publisher through WriteContext)
  2. Add a method on_ack_range_sent to WriteContext accepting event::builder::AckRangeSent/AckRanges (It looks slightly ad-hoc, and a lot of data-passing, may as well combine into write_ack_frame).

Testing:

Not sure how to generate appropriate tests - MockContext for WriteContext currently doesn't accept a publisher.

Results

Here is some sample output running the event-framework example with the async-client example.

As you can see, AckRangeSent is recorded after FrameSent {.. frame: Ack {..}, ..} is recorded.

event: ConnectionMeta { endpoint_type: Server, id: 0, timestamp: Timestamp(Timestamp(0:01:21.403762)) } FrameSent { packet_header: OneRtt { number: 7 }, path_id: 0, frame: Ack { ecn_counts: Some(EcnCounts { ect_0_count: 7, ect_1_count: 0, ce_count: 1 }), largest_acknowledged: 7, ack_range_count: 1 } }
event: ConnectionMeta { endpoint_type: Server, id: 0, timestamp: Timestamp(Timestamp(0:01:21.403762)) } AckRangeSent { packet_header: OneRtt { number: 7 }, path_id: 0, ack_range: 3..=7 }
event: ConnectionMeta { endpoint_type: Server, id: 0, timestamp: Timestamp(Timestamp(0:01:21.403762)) } FrameSent { packet_header: OneRtt { number: 7 }, path_id: 0, frame: Padding }
event: ConnectionMeta { endpoint_type: Server, id: 0, timestamp: Timestamp(Timestamp(0:01:21.403762)) } PacketSent { packet_header: OneRtt { number: 7 }, packet_len: 53 }
event: ConnectionMeta { endpoint_type: Server, id: 0, timestamp: Timestamp(Timestamp(0:01:21.403762)) } DatagramSent { len: 53, gso_offset: 0 }

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

@jon-chuang jon-chuang force-pushed the jon/write-context-on_ack_frame_sent branch from 22fc987 to 8288e40 Compare March 1, 2023 04:13
@jon-chuang jon-chuang changed the title feat(event): on_ack_range_sent feat(event): WriteContext::write_ack_frame records events AckRangeSent Mar 1, 2023
Copy link
Contributor

@camshaft camshaft left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! We appreciate the contribution 😃

quic/s2n-quic-transport/src/contexts/mod.rs Outdated Show resolved Hide resolved
@camshaft camshaft enabled auto-merge (squash) March 2, 2023 05:37
@camshaft camshaft merged commit af5f810 into aws:main Mar 2, 2023
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.

event: emit ack range sent event
2 participants