-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
So this is the intent of the So, arguably, the extra mention in I'm not a big fan of explicitly adding that to all events (too much clutter). |
I don't think that works for my needs from a composition perspective. I want to extend |
Discussed in meeting. |
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 I have a more extensive example at #400, but that's only for 2 events ( 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:
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:
Note that this can also be used for other things than events (e.g., 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 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 :) |
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 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 |
The proposal on #400 would allow me to support the https://github.com/cloudflare/quiche/blob/master/qlog/src/events/quic.rs#L649 I think it would like like this
|
Discussed during editors meeting: |
In H3 parameters_set , we have a
if I grok CDDL correctly, this allows us to have any number of pairs that would serialize like
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?
The text was updated successfully, but these errors were encountered: