> Hello Hugo, > > Thank you for this very thorough review of the QLOG documents! > > Some of your remarks were clear (editorial) errors and I have changed them > in a single commit here: > https://github.com/quicwg/qlog/commit/21a3de1ec3dbcef1c636d91d841fa597f683c8f9 > > Others also indicate valid problems, but will require additional discussion > and/or work before being addressed in a PR. > I have opened individual issues for them in the github repo at: > https://github.com/quicwg/qlog/issues. Hopefully you're willing to continue > the discussion there. Thanks. Yeah, I'll definitely keep an eye on these.
> Finally, for a few remarks, I'll offer my comments here, since I personally > don't think they should be addressed (but might warrant further discussion > in the wg through this mailing list): > > 1. For connectivity:spin_bit_updated's category: while I agree this is > somewhat QUIC specific, the same could be set for the connection_id_changed > event. This feels more like bikeshedding, so unless others have strong > opinions, I'd keep it at connectivity. Yep, this was extremely minor. Let's avoid bikeshedding and keep it as is. > 2. For quic:datagrams_sent and logging other ToS fields: I think ECN is > special here since it has a clear (potential) impact on transport-level > congestion control. I don't agree that other IP-level fields should be > logged here, and rather should get an IP-specific qlog event somewhere down > the line. Seems fair. > Similarly, IIUC QUIC implementations should ALWAYS request the "don't > fragment treatment" from an OS? If not, then I also agree this should not > be part of the datagrams_* events but rather part of a more general > "configuration" field (which we removed a while back due to no concrete use > cases ;)) Yeah, implementations are supposed to always se it. RFC 9000: In IPv4 [IPv4], the Don't Fragment (DF) bit MUST be set if possible, to prevent fragmentation on the path. There is a subtle out here where an implementation can avoid setting it if there's no way for it to do so. e.g. if an OS just doesn't provide the required API. So my feeling was it might be helpful if an implementation can note if it is setting this. But it is an extremely minor thing. > 3. For recovery:congestion_state_updated and uppercase-ness of the ECN > trigger value: ecn is consistently lowercase as field _name_ (as are all > field names in qlog), but we have other instances where ECN-related > _values_ are uppercase (and this is also a _value_, so I'd keep it at ECN) No objections, just thought I'd point it out. > 4. For recovery:loss_timer_updated and clarifying the "timer_type": we > currently have a comment pointing to RFC 9002 there; I would assume this is > enough? So I thought I understood the meaning of this field but I just followed the reference to RFC 9002 A.9 and actually got more confused. I think the reference should be A.8 not A.9, since that is the section which references updating the timer. My feeling is that while A.9 does briefly talk about "mode" in a single line of text the concept is not really developed properly in favour of the psuedocode which doesn't really talk about "mode". My interpretation of the "timer_type" field as things stand is that "ack" is supposed to refer to the case where SetLossDetectionTimer() executes the "// Time threshold loss detection." branch and "pto" where the "GetPtoTimeAndSpace" branch is executed, but I do feel this needs clarifying. The name "ack" here feels off also since it is really about the time threshold. Would definitely be nice to see some clarification here. > 5. For AckFrame and not being able to log the receipt of ECN data without > also logging the ECN values: I personally don't think this is sensitive > enough data to need a way to log this without revealing the values. > Additionally, one can arguably infer this through > recovery:ecn_state_updated that they "should be there" (if not scrubbed by > the network). That's one perspective - that the major reason why one wouldn't want to log the ECN values is the sensitivity of data. I raised this because I don't necessarily think that that's the only reason to avoid logging something - just my gut feeling. I agree this is a very minor issue though and am not too concerned either way. > Thank you again for your time on this. > It's very good to get input from a "new implementer" on the docs now that > they've progressed so much from the initial points where most people > implemented the format. No problem. If anyone is curious, this is my implementation, though note that it doesn't implement the current draft as QVIS doesn't support it yet: https://github.com/openssl/openssl/pull/22037
