-
Notifications
You must be signed in to change notification settings - Fork 4
refactor: add qlog factory, move group_id to common_fields #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## Frando/qlog-latest-multipath #218 +/- ##
================================================================
- Coverage 76.01% 75.81% -0.20%
================================================================
Files 83 84 +1
Lines 23019 23053 +34
================================================================
- Hits 17498 17478 -20
- Misses 5521 5575 +54 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Arqu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
Having a look at https://quicwg.org/qlog/draft-ietf-quic-qlog-quic-events.html I could only find the |
|
95198c5 to
94dc880
Compare
94dc880 to
47880d5
Compare
This adds `qlog` support to iroh. Depends on n0-computer/quinn#218.
Description
This PR changes the qlog configuration such that on the endpoint level, users may supply a
QlogFactory, which is invoked for each connection to (optionally) create aQlogConfigfor that connection. This means that the usual behavior is now to write a qlog file per connection instead of a single file per endpoint.This simplifies the operation in quinn_proto: Because a
QlogSinkis now per connection, we don't have to pass thegroup_id(which is theinitial_dst_cid) for each emitted event, and can instead set it in the so-calledCommonFieldsproperty of the qlog trace (which allows to set a field for each event in the trace). This will significantly reduce the file size of traces also, and is the recommended practice by the spec.It will also enable apps to support the
QLOGDIRenvironment variable, so that an endpoint creates a new file for each connection in that directory.Background
Currently,
quinnimpls qlog support such that there is oneQlogSinkper endpoint, shared between all connections. TheQlogSinkforwards to aqlog::QlogStreamer, which writes events in theJSON-SEQformat. While the events have agroup_idfield that allows to disambiguate events from different connections, a qlog file inJSON-SEQformat only has a single qlog trace. And according to the spec, a trace logically maps to a single connection. The file inJSON-SEQformat does not allow for multiple traces. The trace-level metadata includes a so-called "vantage point", which is the observed role, so either client or server - a property that makes only sense per connection, not per endpoint.Breaking Changes
TransportConfig::qlog_sinkTransportConfig::qlog_factoryNotes & open questions
QlogFactorytrait to create a config for such events.Factoryname, but also couldn't come up with better name .