quicwg / qlog

The IETF I-D documents for the qlog format
Other
86 stars 13 forks source link

Extensions to events can not be indicated by schema #432

Closed LPardue closed 1 month ago

LPardue commented 4 months ago

Here's a summary of what qlog already permits

For example, the QUIC packet_sent event is extensible. Cloudflare has a custom field called send_at_time that we use (see https://github.com/quicwg/qlog/issues/297 for background). If I want to formally define that extension point with a schema, the current event_schemas mechanism doesn't quite work well. While I could define a new namespace, category ID and event type - that's a lot of overhead for adding just a single field to an existing event. This also makes it hard to compose extensions together.

Proposal: define a new type of schema that is scoped to cover only extensions to existing event types, which is indicated by a value in the URI e.g., where we have urn:ietf:params:qlog:events:gen#loglevel a standardized extension foo could be identified by urn:ietf:params:qlog:event_ext:extension_foo, a private extension could be identified by https://example.com/032024/event_ext/extension_foo.html

rmarx commented 1 month ago

Mulling this over for a long time, I don't see an immediate better/alternative solution than indeed going with an additional type of URN for the event_ext here... (see alternatives I've tried on the bottom of this comment).

However, this does have some annoyingness:

For the "I just want to extend an existing event's Data field with new fields" use case, this works just fine. You'd put the following in :

In `urn:ietf:params:qlog:event_ext:more_packet_sent_fields`:
$$packet-sent-extension //= (
  ? additional_field_b: bool
)

Where the original document `urn:ietf:params:qlog:events:quic#quic` would have:
QUICPacketSent = {
    field_a: uint8

    * $$quic-packetsent-extension
}

HOWEVER, in the case where you actually define new events/data types and then add them to an existing enum/CDDL "type socket", you get an annoying choice:

  1. either split up the definition of the new event into its own URN and the existing type socket extension in another event_ext URN
  2. just keep the type socket extension in the new event URN as well

2 feels more logical, but is asymmetric with the above (i.e., extending event Data fields directly requires a new events_ext URN while adding new events/types to a type socket does NOT). 1 is more symmetric, but seems like annoying busy work. Examples below:

Option 1:

In urn:ietf:params:qlog:events:new_frame#rogerrabbit:

RogerRabbitFrame = {
     type: "Roger"
     ? who: text
}

In urn:ietf:params:qlog:events_ext:rogerrabbit:

$H3Frame /= RogerRabbitFrames

(where $H3Frame is defined in urn:ietf:params:qlog:events:h3#h3 of course)

Option 2 would just have both under the same URN

In urn:ietf:params:qlog:events:new_frame#rogerrabbit:

RogerRabbitFrame = {
     type: "Roger"
     ? who: text
}

$H3Frame /= RogerRabbitFrames

Note that in Option 1, both URNs can of course still be defined in the same document, it's just annoying to have to split them up separately (although, you could make the argument it's a bit cleaner/more explicit that way as well).

Another remark is that above, I've extended just $H3Frame which is NOT an event, but the proposed URNs for extensions is currently event_ext. So I'd bikeshed that to just extensions instead!


Appendix: alternatives considered

A. decoupling event categories from the events URN. This would allow us to just do something like urn:ietf:params:qlog:definitions:MyNamespace and the namespace could contain anything! No need for new URN types or anything! However, since we need categories to be globally unique, having them in the URN somehow seems like a good way to automatically enforce that...

B. Allowing extension of existing categories and URNs. For example, when extending QUICPacketSent above, you'd have something like urn:ietf:params:qlog:events:quic#quic?packet-sent-ext-1. However, this feels wrong for multiple reasons: you'd basically re-use a URN defined in document A as prefix in document B, C, D,.. which makes it hard to track, you'd only be able to do this to add EXTENSIONS to the existing events in a category but NOT add new events to that category (since we disallow that currently). It's all a bit of a mess...

rmarx commented 1 month ago

After my long comment above and thinking about it some more + a long discussion with Lucas, we decided it would make most sense to just remove categories from qlog altogether.

I originally used categories because I thought events would be re-usable across protocols, but in practice, this was a pipe dream anyway. They just made extensibility and naming (and even implementations) more difficult while adding little to no value.

439 removes categories and consequently makes it possible to be more flexible in how we extend qlog, including with new documents that just extend existing events in existing namespaces, and it thus also closes this issue as solved.