feat(s2n-quic-dc): add map events #2362
Conversation
camshaft
force-pushed
the
camshaft/dc-map-events
branch
from
68ddcd7
to
1bb46ff
Compare
| let key = entry.control_secret(); | ||
|
|
||
| let Some(packet) = packet.authenticate(&key) else { | ||
| self.subscriber().on_stale_key_packet_rejected( |
There was a problem hiding this comment.
It seems like we ought to have a variant of rejected for failed auth and for just unknown?
There was a problem hiding this comment.
yeah i went back and forth on that. i currently don't have one for unknown - we don't emit anything right now. but probably worth doing.
dc/s2n-quic-dc/events/map.rs
Outdated
| capacity: usize, | ||
|
|
||
| /// The port that the path secret is listening on | ||
| control_socket_port: u16, |
There was a problem hiding this comment.
I don't think we're listening on this port? Or at least we never read from the control socket -- it's purely used for sending. We listen/read control packets from the s2n-quic handshake socket.
It would be nice to have a global counter here for # of maps in existence -- probably this is the right place to emit that counter? (tracked via a global atomic static under the hood)
There was a problem hiding this comment.
I don't think we're listening on this port? Or at least we never read from the control socket -- it's purely used for sending. We listen/read control packets from the s2n-quic handshake socket.
Do you think I should just remove it? I guess if we're not listening it's pretty useless.
It would be nice to have a global counter here for # of maps in existence -- probably this is the right place to emit that counter? (tracked via a global atomic static under the hood)
I'm thinking the subscriber would be the one that tracks the number. Usually we try to keep the event emissions decoupled from the aggregation.
There was a problem hiding this comment.
Yeah I'd just drop the control socket port.
I'm thinking the subscriber would be the one that tracks the number. Usually we try to keep the event emissions decoupled from the aggregation.
How can the subscriber keep track of a global number? We inherently need a global in the map if we want an actually global number.
There was a problem hiding this comment.
Couldn't it just take the difference between the number of init and deinit events?
There was a problem hiding this comment.
No, because I might not always configure the subscriber. The goal of the global static is to ensure that if at least one subscriber is used I can get an accurate count.
Maybe there's other ways to achieve that though.
camshaft
force-pushed
the
camshaft/dc-map-events
branch
from
1bb46ff
to
a682947
Compare
Description of changes:
This change adds a bunch of new events to the dc map implementation. These events should provide the following:
Call-outs:
There's a few more things I still need to include in a follow-up:
containscallsTesting:
I've currently only manually tested that the events are being emitted. As a follow-up PR (since this one is already quite large), I will add several event snapshot tests to show it all working and prevent regressions in events.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.