qlog icon indicating copy to clipboard operation
qlog copied to clipboard

Add raw_ack_delay field to AckFrame

Open rmarx opened this issue 1 year ago • 3 comments

As requested by Damien Neil on the QUIC wg mailing list on November 9, this adds an option to log the raw ack_delay value seen in the AckFrame, as opposed to the scaled value (for which external "global" state is needed, that is not always present in all code paths).

rmarx avatar Nov 10 '23 08:11 rmarx

@marten-seemann thank you for the feedback. My reply to the list is pending moderation (no clue why), just so you know I also had some explanation in there, so we'll see what others say. I don't feel strongly this HAS to be in there, but I do understand the ask here.

rmarx avatar Nov 10 '23 08:11 rmarx

for which external "global" state is needed, that is not always present in all code paths

I don't think this is a reasonable ask. We discussed this a few months ago on the list, and the next QUIC RFC might need to add some clarifying text that ACK delay isn't used in the Initial and Handshake PN space. This is completely independent of qlog, and conflating those two would be bad design.

In the end, a QUIC stack will need to decode the value at some point, there's nothing it can do with an unscaled ACK delay. The only advantage to introducing the unscaled_ack_delay is that a QUIC stack can apply the scaling after qlogging the frame. I don't think this strikes the right balance (as I've argued on list). Among all the qlog implementations around today, no other QUIC stack ran into this problem, so this seems like we're adding logging in order to work around an architectural problem in one particular single stack here.

marten-seemann avatar Nov 10 '23 09:11 marten-seemann

FWIW Its easier to track feature requests as issues rather than PRs (I'm guilty of this too).

Propose closing this with no action.

LPardue avatar Nov 17 '23 21:11 LPardue

Agreeing with @marten-seemann here. Also have seen no pushback on the list since, so closing without action. If still needed, please open an issue.

rmarx avatar Jun 27 '24 18:06 rmarx