quicwg / qlog

The IETF I-D documents for the qlog format
Other
84 stars 12 forks source link

Convert H3 setting param types to a real type #383

Closed LPardue closed 3 months ago

LPardue commented 8 months ago

Closes #382

LPardue commented 7 months ago

Instead of a separate raw_type field, we'll just allow the nae to be a union of enum and uint64.

rmarx commented 3 months ago

Heads up that I changed some of this in #417, by adding the name_bytes field (to be consistent with other places where we allow logging raw values), so that would replace the raw_type field proposed here.

LPardue commented 3 months ago

Heads up that I changed some of this in #417, by adding the name_bytes field (to be consistent with other places where we allow logging raw values), so that would replace the raw_type field proposed here.

So I've rebased and resolved the conflict. However, ISTM on a previous editor call we wanted to make this a union instead, so something like

H3Setting = {
    name: H3SettingsName / uint64

    value: uint64
}

and obviously a text update to describe how to pick which one.

Do we still want to do that?

rmarx commented 3 months ago

So, we've gradually moved away from that pattern IIRC (e.g., packet_number is now always a uint64 instead of uint64 / string to deal with IJSON).

Looking through the other events, there are a few events that use this pattern (ResetStreamFrame, StopSendingFrame, ConnectionClosed, IPAddress), e.g., connectionclosed:

    ? connection_code: $TransportError /
                       CryptoError /
                       uint32
    ? application_code: $ApplicationError /
                        uint32

But most events that need something like this, use the

field: string
field_bytes: hexstring

OR

field: string
field_bytes: uint64

It seems like ideally we'd just have a single way of dealing with these. I'm leaning more towards the explicit extra field_bytes pattern, to make things explicit that this is (likely) an "unexpected" value that's likely to cause problems + to prevent tools from having to special case types for fields (at least most of the time).

If you agree, then we should merge this PR as-is (or with my proposed changes in the upcoming review ;)) and then I can make a new PR adding new XYZ_bytes fields to the non-conformant events.

LPardue commented 3 months ago

I'm more than happy to go with this direction right now. Suggestions merged so I think GTG