qlog icon indicating copy to clipboard operation
qlog copied to clipboard

Should we use "* text => uint64" anywhere that a type can contain extension fields?

Open LPardue opened this issue 1 year ago • 8 comments

In H3 parameters_set , we have a

; additional settings for grease and extensions
    * text => uint64

if I grok CDDL correctly, this allows us to have any number of pairs that would serialize like

"my_sekret_extension": 123,
"9999": 0

It got me thinking a bit. This isn't explained super clearly and people might overlook it. Maybe there's more use for this in some other types. Leading to, maybe we should define this as a generic tuple type in the base spec along with some text, then we can reuse it across various events?

LPardue avatar Jan 22 '24 23:01 LPardue

So this is the intent of the * text => any mention in the main schema here: https://www.ietf.org/archive/id/draft-ietf-quic-qlog-main-schema-07.html#name-events

So, arguably, the extra mention in parameters_set should be removed/replaced with a reference to that instead.

I'm not a big fan of explicitly adding that to all events (too much clutter).

rmarx avatar Jan 30 '24 10:01 rmarx

I don't think that works for my needs from a composition perspective. I want to extend $ProtocolEventBody so that the extension is specific to a single type, not extend the entire event.

LPardue avatar Jan 30 '24 11:01 LPardue

Discussed in meeting. Lucas: main intent is to formally allow any event to be extensible (instead of having to copy-paste all event parameters to a new event name). Currently possible in code of course, but not in proper CDDL schema. Robin: probably not possible without making everything a socket, but need to double check with CDDL defs.

rmarx avatar Feb 05 '24 09:02 rmarx

So, turns out this is possible in a relatively simple way.

I thought we would have to make every event their own "type socket" (like $protocolEventBody or $quicFrame), but it turns out we can make do with "group sockets" (they use $$ instead of $ and don't require all the boilerplate the other sockets need). Both are described here: https://datatracker.ietf.org/doc/html/rfc8610#section-3.9

I have a more extensive example at #400, but that's only for 2 events (parameters_set and parameters_restored).

If we want to allow this for all events, this is possible, but would require adding something like this on the bottom of each event definition:

* $$quic-eventname-extension

That's basically all that's needed... (which is much less than what's needed for the "type sockets").

With that, you can specify new fields on a per-event basis by extending one specific extension point, which is IIUC what @LPardue asked here.

So say there's a new extension that adds 2 loss bits and you want to add those to the packet_sent event:

$$quic-packetsent-extension //= (
    q_bit: bool
    ? l_bit: bool
)

Note that this can also be used for other things than events (e.g., H3Parameters). It won't work for extending things like ENUMs (or e.g., H3Setting), but for that we can still use the more verbose "type sockets" that we've been doing before if needed.


The main downside is that all the new fields are by definition optional when regarded in the complete schema, because for now, we of course don't have any extensions yet, so we have to do * $$quic-eventname-extension (there will be zero or more) or ? $$quic-eventname-extension (they are optional) in our current definition. So even if later extensions mandate a new field (as the example above, q_bit is mandatory), this wouldn't be enforced in the combined schema.

There are of course workarounds for this (e.g., script that gathers CDDL from all docs can remove the * or ? before the used $$ fields so they do properly track this), but it's good to be aware of this.

AFAICT, there is no way to prevent this without additional boilerplate (e.g., we could not use * or ?, but then we'd have to define an (empty) extension for each of these in the current documents... annoying).


I propose we track this general issue here going forward (instead of related #261, #176, #170, #124, #192, #297).

I could use some opinions on this here @LPardue @marten-seemann @lnicco :)

rmarx avatar Feb 29 '24 15:02 rmarx

Nice. The caveats sound fine to me (but my understanding isn't as comprehensive as yours). It does sound to me that we'll probably want to have some support in place for future extensions to help get validated but that is outside the scope of the qlog documents themselves.

LPardue avatar Mar 02 '24 02:03 LPardue

@LPardue yes, it's definitely possible to get future extensions to be properly validated by the CDDL tooling. The moment they have a concrete extension in place, it suffices to change * $$quic-eventname-extension to $$quic-eventname-extension (remove the *) so the field will no longer be seen as "zero or more" but instead "one" and the tooling will properly track optional/required fields in the extension as expected (tested this with the ruby gem last week).

rmarx avatar Mar 04 '24 09:03 rmarx

The proposal on #400 would allow me to support the send_at_time custom field we have in quiche 👍

https://github.com/cloudflare/quiche/blob/master/qlog/src/events/quic.rs#L649

I think it would like like this

~~~~~~~~
$$quic-packetsent-extension //= (
  ? sent_at_time: float
)
~~~~~~~~

LPardue avatar Mar 04 '24 09:03 LPardue

Discussed during editors meeting: Merge #400 now Robin: Create new PR that adds extension sockets to all events (and other types where needed, like H3Setting), discuss during Brisbane meeting

rmarx avatar Mar 04 '24 09:03 rmarx