qlog icon indicating copy to clipboard operation
qlog copied to clipboard

Convert H3 setting param types to a real type

Open LPardue opened this issue 1 year ago • 1 comments

Closes #382

LPardue avatar Jan 28 '24 18:01 LPardue

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

LPardue avatar Feb 19 '24 09:02 LPardue

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.

rmarx avatar Jun 24 '24 13:06 rmarx

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?

LPardue avatar Jun 26 '24 18:06 LPardue

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.

rmarx avatar Jun 27 '24 12:06 rmarx

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

LPardue avatar Jun 27 '24 16:06 LPardue