search

quicwg / qlog

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

Add clarification on is_coalesced and add it to more events #403

Open rmarx opened 4 months ago

rmarx commented 4 months ago

Fixes #370.

This better explains the intent behind is_coalesced and also adds it to other events that probably should have it as well.

I'm not sure anymore that this is the best design however, since there's a lot of duplication for a feature that isn't THAT important to track...

I wonder if we can get away with something like "If implementations don't want to track individual datagram IDs, but still want to indicate a packet was/could be coalesced, they can use datagram_id = -1 to indicate this" (and then of course we remove is_coalesced and make datagram_id an int32 instead of uint32).

That would be a bit dirty... but reduce the confusion and duplication considerably?

rmarx commented 4 months ago

Another option is to ditch the whole concept of datagram_id and only keep is_coalesced (datagram_id is only used to properly track coalescing anyway)

LPardue commented 4 months ago

I think the new text that clarifies how the datagram_id is used is good. I don't think we benefit from the is_coalesced field, since it's a subset of a subset i.e. if an implementation cares enough about logging coalescing, then it can do the work to add datagram_id support.

rmarx commented 3 months ago

Thanks for the comments on #370 @HLandau and @LPardue.

I agree and have removed is_coalesced in my latests commit. I've also moved all the text related to this to the datagramssent event so it's less spread-out.

PTAL and see if the explanation there around datagram_id is clear enough. Thanks :)

hlandau commented 3 months ago

Looks good. Might want to rename this issue.