qlog: add a default tracer that writes to QLOGDIR #4233
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4233 +/- ##
==========================================
- Coverage 84.15% 84.11% -0.04%
==========================================
Files 149 150 +1
Lines 15381 15402 +21
==========================================
+ Hits 12943 12955 +12
- Misses 1941 1947 +6
- Partials 497 500 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thank you for working on this @birneee!
It would be nice to have an integration test as well. In that test, you could assert that
- The directory is created
- It contains qlog files for both the client and server
- These qlog files both contain the same connection ID
|
We can also simplify the qlog instructions in the README: https://github.com/quic-go/quic-go?tab=readme-ov-file#quic-event-logging-using-qlog |
qlog/qlog_dir.go
Outdated
| if QlogDir == "" { | ||
| return nil | ||
| } | ||
| path := fmt.Sprintf("%s/%s_%s.qlog", strings.TrimRight(QlogDir, "/"), connID, label) |
There was a problem hiding this comment.
Should we also include the date like here: https://github.com/libp2p/go-libp2p/blob/v0.32.2/p2p/transport/quicreuse/tracer.go#L47-L53?
It's quite nice to be able to sort the qlogs by file name and have them in chronological order.
There was a problem hiding this comment.
For me, this does not seem to be a big advantage, as one could also sort by file creation time.
I would rather stick with ODCID plus vantage point as recommended by the qlog draft.
| if qlogDir == "" { | ||
| return nil | ||
| } | ||
| if _, err := os.Stat(qlogDir); os.IsNotExist(err) { |
There was a problem hiding this comment.
- Should we handle non-nil errors that are not
os.IsNotExisterrors? - The documentation for
os.IsNotExistsays we should useerrors.Is(err, fs.ErrNotExist).
There was a problem hiding this comment.
I'll do this in a follow-up PR. I might also change the file name a bit.
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
Co-authored-by: Marten Seemann <[email protected]>
| if qlogDir == "" { | ||
| return nil | ||
| } | ||
| if _, err := os.Stat(qlogDir); os.IsNotExist(err) { |
There was a problem hiding this comment.
I'll do this in a follow-up PR. I might also change the file name a bit.
marten-seemann
changed the title
* add qlog default tracer which writes to QLOGDIR * gofumpt * add qlog default tracer which writes to QLOGDIR * fix flaky tests * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> --------- Co-authored-by: Marten Seemann <[email protected]>
* add qlog default tracer which writes to QLOGDIR * gofumpt * add qlog default tracer which writes to QLOGDIR * fix flaky tests * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> * Update README.md Co-authored-by: Marten Seemann <[email protected]> --------- Co-authored-by: Marten Seemann <[email protected]>
Issue #4189
Split from #4201