Add clarification on is_coalesced and add it to more events #403
Conversation
|
Another option is to ditch the whole concept of |
|
I think the new text that clarifies how the datagram_id is used is good. I don't think we benefit from the |
…nly in datagramssent
| datagram-level qlog events SHOULD thus use the local and qlog-specific concept | ||
| of a "datagram identifier" in the `datagram_id` field. Selecting specific and | ||
| locally unique `datagram_id` values is left to qlog implementations, but in | ||
| general multiple packet-level events sharing the same `datagram_id` SHOULD |
There was a problem hiding this comment.
This second SHOULD kudt seems to be repeating the requirement above?
There was a problem hiding this comment.
It is; I wasn't aware that's a problem. Would you prefer to change the first SHOULD to a should or the second one here?
| Implementations willing to track coalescing across packet-level and | ||
| datagram-level qlog events SHOULD thus use the local and qlog-specific concept | ||
| of a "datagram identifier" in the `datagram_id` field. Selecting specific and | ||
| locally unique `datagram_id` values is left to qlog implementations, but in | ||
| general multiple packet-level events sharing the same `datagram_id` SHOULD | ||
| indicate they were coalesced in the same UDP datagram. |
There was a problem hiding this comment.
So I might be misreading what you intend but with current text but I could even go further with something like
| Implementations willing to track coalescing across packet-level and | |
| datagram-level qlog events SHOULD thus use the local and qlog-specific concept | |
| of a "datagram identifier" in the `datagram_id` field. Selecting specific and | |
| locally unique `datagram_id` values is left to qlog implementations, but in | |
| general multiple packet-level events sharing the same `datagram_id` SHOULD | |
| indicate they were coalesced in the same UDP datagram. | |
| qlog defines it's own mechanism for tracking coalescing across packet-level and | |
| datagram-level qlog events, a "datagram identifier" carried in `datagram_id` fields. | |
| qlog implementations that want to track coalescing can use this mechanism, the | |
| selectiion of specific and locally-unique `datagram_id` values is an implementation | |
| choice. |
There was a problem hiding this comment.
I feel that loses a (potentially obvious, but still good to call out) aspect in how to use the datagram_id though... so maybe something like this then:
| Implementations willing to track coalescing across packet-level and | |
| datagram-level qlog events SHOULD thus use the local and qlog-specific concept | |
| of a "datagram identifier" in the `datagram_id` field. Selecting specific and | |
| locally unique `datagram_id` values is left to qlog implementations, but in | |
| general multiple packet-level events sharing the same `datagram_id` SHOULD | |
| indicate they were coalesced in the same UDP datagram. | |
| qlog defines its own mechanism for tracking coalescing across packet-level and | |
| datagram-level qlog events, a "datagram identifier" carried in `datagram_id` | |
| fields. qlog implementations that want to track coalescing can use this | |
| mechanism, where multiple packet-level events sharing the same `datagram_id` | |
| indicate they were coalesced in the same UDP datagram. The selection of specific | |
| and locally-unique `datagram_id` values is an implementation choice. |
|
Looks good. Might want to rename this issue. |
Fixes #370.
This better explains the intent behind
is_coalescedand 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_coalescedand makedatagram_idanint32instead ofuint32).That would be a bit dirty... but reduce the confusion and duplication considerably?