Comments on QLOG drafts

[Hugo Landau <[e-mail]>]

Hi,

I've completed a review of the QLOG drafts and have some comments. Hope these
are helpful.

Comments on draft-ietf-quic-qlog-main-schema.md
===============================================

    ## Custom fields\
 
Trailing backslash in this section title

Comments on draft-ietf-quic-qlog-quic-events.md
===============================================

connectivity:connection_closed:

  This is missing the frame type field in the QUIC CONNECTION_CLOSE
  frame which is rather useful information.

  connection_code can be a string or a uint32, but QUIC uses a variable
  length integer here and thus this can be up to 62 bits. Same for the
  application_code.

  It is up to the decoder to infer by presence of connection_code or
  application_code to infer whether this was an application or transport
  CONNECTION_CLOSE. But this requires it to log such a value. It would
  be nice if this can be communicated without necessarily knowing the
  actual value. Consider either adding a field to distinguish between
  transport and app errors or replacing the "error" field of "trigger"
  with "app_error" and "transport_error".

connectivity:connection_id_updated:
 
  It would be nice to be able to optionally log CID-associated sequence
  numbers and the type of CID source (ODCID, Initial CID,
  preferred_address transport parameter, NCID frame, etc.)

connectivity:spin_bit_updated:
 
  This is very QUIC-specific, so I question the value of it being defined
  as a generic connectivity event.

connectivity:mtu_updated:
 
  I suppose it is a bit of an asinine remark, but IPv6 jumbograms mean
  theoretically you could do things with datagrams greater than 64k.
  uint32?

quic:version_information:
   
  Maybe clarify that if the client receives a version negotiation packet
  with no viable overlap, it logs a version_information event with no
  chosen_version?

quic:alpn_information:
 
  RFC 7301 states that ALPN protocol strings are byte strings, not text.
  How should ALPN protocol strings which are not valid UTF-8 or not
  valid text be logged?

quic:parameters_set:
 
  As things stand, there is no facility for logging the reception of unknown
  transport parameters by an endpoint. Since transport parameters are used to
  negotiate extensions to QUIC, having logged data about the presence of
  transport parameters sent by a peer but not understood, and the consequential
  ability to get information about the prevalence of any given unsupported
  transport parameter in incoming traffic, seems like it would be quite useful.

  I think this event needs more clarify in terms of the distinction between
  attempting to negotiate a feature and it being enabled. It should be possible
  to log the feature set a local or remote endpoint is requesting and it
  should be possible to log the actually negotiated feature set. Possibly
  this can be resolved via simple clarification. If a client connects to
  a server, does it start by emitting parameters_set (owner=local) with (e.g.)
  max_datagram_frame_size set, and then when the server responds without
  it, emit parameters_set (owner=remote) without it? I guess this allows
  the necessary information to be inferred.

  If the absence of a field indicates it was not negotiated after receiving
  transport parameters from a server, this creates a problem if this event has
  to be used to log other parameters determined at different times (which seems
  plausible for `tls_cipher`, `early_data_enabled`, etc.), as then logging
  those parameters could be misinterpreted as a sign that the transport
  parameters have been received and a feature has not been negotiated.
  Either the cause of a parameters_set event should be clarified (maybe a trigger
  field) or possibly this should be split into different event types.

  I think it would be valuable to have the ability to log information
  such as spin bit policy (e.g. enabled, disabled due to random chance,
  disabled administratively), independent of the actual spin_bit_updated
  event).

quic:packet_sent:

  The meaning of `is_coalesced` should be clarified. I assume this is
  set to true unless a packet is both the first and last packet in a
  datagram. However that is not necessarily obvious when a packet is
  being generated; another option is to define this as being true for
  every packet but the first packet in a datagram.

quic:packet_received:
 
  See comment on is_coalesced above

quic:packet_dropped:
 
  What is the intended trigger category here when an EL is discarded and
  buffered packets are discarded as a consequence? `decryption_failure`
  could match but feels subtly different, as I guess that covers the
  case where a packet is received after an EL is discarded. IMO it would
  be better to have a different trigger category for this, and add
  language clarifying this case.

quic:packet_acked:

  `PacketNumberSpace.application_data`, as this is by far the most prevalent packet
  number space a typical QUIC connection will use.
 
  Editorial note: The usage of the . notation
  `PacketNumberSpace.application_data` seems slightly novel here. Just
  write "application_data"?

quic:datagrams_sent:

  It is a bit minor, but the optional logging of the non-ECN bits in the ToS byte
  (as a separate field) might be useful. Implementations which don't do anything
  special here would omit it, and specifying a field would indicate that the
  implementation has requested a specific ToS from the OS.

  I also think it would be potentially valuable if an implementation could
  note if it has requested 'Don't Fragment' treatment from an OS, but since
  an implementation can be expected to be consistent about this, this doesn't
  seem like something which should be logged on a per-datagram basis, perhaps
  elsewhere.

quic:datagrams_received:

  Comments on ToS above also apply here

quic:stream_state_updated:

  I think this needs to be clarified a little that an event logged here with both
  a `stream_id` field and a `stream_side` field is expressing a state for that
  component of the stream and from the perspective of the specified vantage point
  (also, what about the network vantage point?); whereas an event logged here
  with a `stream_id` field but no `stream_side` field is expressing a state for
  both stream components as a whole.

  If my interpretation here is correct, I think it would also help to note that
  the valid values of StreamState depend on the value *and* presence of
  stream_side.

quic:frames_processed:

    The `frame_processed` event can be used to signal internal state change not
    resulting directly from the actual `parsing` of a frame (e.g., the frame
    could have been parsed, data put into a buffer, then later processed, then
    logged with this event).
 
  Editorial note: use of backticks around parsing seems off here.

     The `packet_received` event can convey all constituent frames. It is not
     expected that the `frames_processed` event will also be used for a
     redundant purpose. Rather, implementations can use this event to avoid
     having to log full packets or to convey extra information about when frames
     are processed (for example, frame processing is deferred for any reason).
 
  Editorial note: add "if" before "frame processing is deferred for any reason"

quic:stream_data_moved:
 
  A brief explanation of each from/to value here and the intended meaning would
  be a big improvement. Also clarify that raw here refers to the referenced
  stream application data (and does not include frame headers etc.).

  Consider having a way to report when the end-of-stream (FIN) condition is
  reported to an application. One option here is to have an optional "fin" field
  here.

  There does not appear to be any way to communicate when a stream reset event is
  delivered to an application in an equivalent way, which would be useful.

quic:datagram_data_moved:
 
  As with stream_data_moved, clarify meaning of the from/to fields here.

security:key_updated:
 
  Clarify use of the generation field here, which is introducing a new concept
  not directly touched on by the QUIC RFCs, which just deal in the Key Phase bit.
  Provide examples for the first few generation numbers (for example, presumably
  the first 1-RTT EL key has a generation of 0). Also clarify how this interacts
  with the temporal behaviour of the key update process (since old keys are
  retained to handle reordered packets). Presumably once a new key exists, it is
  logged with a new generation number and the old key is eventually given a
  key_discarded event with the old generation number. But this should be
  clarified.

security:key_discarded:
 
  As above.

recovery:parameters_set:

    do, for some reason, change these parameters during execution, MAY emit the
    `parameters_set` event twice.
 
  Twice? Seems like this should say "more than once".
  Maximum datagram size could change repeatedly.

  A minor point, but I suppose it may be useful to have a field for a name of a
  congestion control algorithm here, defined as "newreno" / text (with "newreno"
  being the algorithm specified in RFC 9002).

recovery:congestion_state_updated:
 
  It is a little bit inconsistent here for ECN to be uppercase when the
  "ecn" field in other events is lowercase.

recovery:loss_timer_updated:
 
  Probably have a little bit of text clarifying the meaning of
  `timer_type` here.

AckFrame:
 
  It is worth noting that this doesn't allow a QLOG implementation to
  log whether a peer included ECN data in an ACK frame without actually
  including that data. i.e., a QLOG decoder has to infer presence of ECN
  from the presence of the ect1/ect0/ce fields. Perhaps this is too
  minor to address, but I thought I'd point it out.

ResetStreamFrame:

  error_code should be uint64 here.

StopSendingFrame:

  error_code should be uint64 here.

CryptoFrame:
 
  StreamFrame has an optional raw field here, so CryptoFrame should too.

StreamFrame:
 
  This format has ambiguity as to whether explicit length and/or offset
  fields were used. It is potentially useful to know this, or the
  numeric value of the stream type field.

  Being able to know the header length for overhead calculation would
  also be valuable IMO.

  It may be worth explicitly clarifying that it is not possible to use
  this object to log information about a stream frame without logging
  the value of the FIN bit, as the presence or absence of this bit is
  inferred from presence or absence of the FIN field.

ConnectionCloseFrame:

  Is there a reason error_code is limited to uint32 here when
  error_code_value is correctly specified as uint64? Understanding some
  of the rationale behind the encoding of error codes in this
  specification would be helpful.

Yours,
Hugo Landau