Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
LPardue opened this issue Jan 22, 2024 · 8 comments · May be fixed by #417
Open

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

LPardue opened this issue Jan 22, 2024 · 8 comments · May be fixed by #417
Assignees

Comments

@LPardue
Copy link
Member

LPardue commented Jan 22, 2024

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?

@rmarx
Copy link
Contributor

rmarx commented Jan 30, 2024

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).

@LPardue
Copy link
Member Author

LPardue commented Jan 30, 2024

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.

@rmarx
Copy link
Contributor

rmarx commented Feb 5, 2024

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 rmarx self-assigned this Feb 5, 2024
@rmarx
Copy link
Contributor

rmarx commented Feb 29, 2024

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 :)

@LPardue
Copy link
Member Author

LPardue commented Mar 2, 2024

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.

@rmarx
Copy link
Contributor

rmarx commented Mar 4, 2024

@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).

@LPardue
Copy link
Member Author

LPardue commented Mar 4, 2024

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
)
~~~~~~~~

@rmarx
Copy link
Contributor

rmarx commented Mar 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants