qlog icon indicating copy to clipboard operation
qlog copied to clipboard

packet_sent doesn't allow logging of IP address (for packets sent outside of a connection)

Open marten-seemann opened this issue 1 year ago • 2 comments

This would be interesting for Version Negotiation, Retry, CONNECTION_CLOSE etc. sent by a server.

marten-seemann avatar Feb 02 '24 10:02 marten-seemann

Discussed during meeting. Robin: not a big fan of this; use datagrams_* instead or group_id/path_id or custom field or separate trace Lucas: not super-strong opinion there Marten: imo server log sending out retries should log everything in a single event, not spilt out Robin: will think about it some more; get back to you

see also #211

rmarx avatar Feb 05 '24 09:02 rmarx

So, still not a big fan of having this separately from path_id (see #336) or conceptually group_id.

I get that logging a separate path_assigned event is annoying for this type of thing, but there you could also choose to just log the IP addresses as the pathID as that's a string (see for example slide 5 here: https://datatracker.ietf.org/meeting/118/materials/slides-118-quic-qlog-00).

I'm mostly afraid that if you add ip fields to all these events, people will use them for "normal" logs as well, not just for the use cases you describe, which is something I -really- want to discourage.

rmarx avatar Mar 01 '24 15:03 rmarx

@marten-seemann now that https://github.com/quicwg/qlog/pull/336 has been merged, and with my comments above, do you think we can close this issue?

rmarx avatar Jun 10 '24 09:06 rmarx

Discussed during editor's meeting.

Marten: really useful to log events outside connections for debugging Lucas: true, but in practice we don't do this at scale because we get so much bad stuff. Have whole DDoS protection layers to protect against this. Also annoying to link outside connection events to actual connections (e.g., VNEG/RETRY).

Marten: proposed adding separate events to explicitly log this stuff Robin: seems too much work/overhead just to add ip address info. Feel like the pathID "hack" proposed above allows for this use case. Marten: I can work, but it's dirty. Paths aren't necessarily 4-tuples; abusing fact that pathID is a string Robin: true... but if you look at how PathEndpointInfo is defined, that's basically also just a 4-tuple? You would just skip the path_assigned step by serializing it yourself. Marten: don't want to make it more complicated than necessary. I don't like the pathID solution, really don't, but creating new events/category seems like a lot of overhead.

Robin: ok. For now then just closing this issue without action; use pathID for this use case. Open to revisiting later if more feedback from the field comes in.

rmarx avatar Jun 10 '24 09:06 rmarx