-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
qlog: split serializiation and event definitions, remove logging abstraction #5356
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 @@
## master #5356 +/- ##
==========================================
- Coverage 84.17% 83.07% -1.10%
==========================================
Files 158 156 -2
Lines 18730 18743 +13
==========================================
- Hits 15765 15570 -195
- Misses 2346 2555 +209
+ Partials 619 618 -1 ☔ View full report in Codecov by Sentry. |
7966761 to
5a674fd
Compare
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.
Pull Request Overview
This PR implements a comprehensive overhaul of the qlog system by removing the logging abstraction layer and establishing qlog as the primary logging mechanism. The changes introduce a new direct qlog API that replaces the previous wrapped abstraction, enabling more flexible and fine-grained event logging for QUIC connections.
Key changes:
- Removes the
loggingpackage abstraction layer in favor of direct qlog usage - Introduces new qlog event structures and encoding mechanisms
- Updates all transport and server components to use the new qlog API
- Establishes a test utility framework for qlog event recording
Reviewed Changes
Copilot reviewed 86 out of 89 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| transport_test.go | Updated test infrastructure to use new qlog event recording system |
| transport.go | Migrated packet handling and tracing to direct qlog event recording |
| testutils/events/ | Added new test utility framework for qlog event recording and verification |
| server_test.go | Converted server tests from mock-based to event recorder-based testing |
| server.go | Updated server packet handling to use qlog event recording |
| qlogwriter/ | New qlog writer infrastructure for JSON-SEQ format output |
| qlog/ | Completely rewritten qlog types and event structures with direct encoding |
| mtu_discoverer.go | Updated MTU discovery to use qlog event recording |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
220e5c6 to
8840215
Compare
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.
Pull Request Overview
Copilot reviewed 86 out of 89 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
qlogwriter/writer.go:1
- [nitpick] Consider using
jsontext.Uintinstead ofjsontext.Intfor StreamID since protocol.StreamID is an unsigned integer type, maintaining type consistency with the underlying protocol definition.
package qlogwriter
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
d590c24 to
46de50e
Compare
46de50e to
34fbc60
Compare
34fbc60 to
595999e
Compare
|
I don't think there's a lot we can do about code coverage here. We're not really reducing coverage in any existing files. |
595999e to
0047936
Compare
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
0047936 to
e905342
Compare
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.
Pull Request Overview
Copilot reviewed 85 out of 88 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
server_test.go:1
- Corrected spelling of 'adddng' to 'adding'.
package quic
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
c4a2b2c to
b49c9b7
Compare
b49c9b7 to
427fd27
Compare
Fixes #4915.
This is an absolutely massive PR, but I don't see any way to split it up into smaller parts.
This PR:
loggingpackage. Logging is done using qlog. This is QUIC, qlog is (or rather, will soon be) an RFC, so it makes soon to use qlog, instead of wrapping it in another abstraction layer.metricspackage. This is temporary. We should offer some kind of metrics tracer, although I'm currently undecided regarding the exact structure.The new API looks very different from what we had so far. For example, here's how the
packet_sentevent was triggered previously:quic-go/logging/connection_tracer.go
Lines 18 to 19 in e6d5d96
Now we just pass an
qlog.EventtoRecordEvent:This is a lot more flexible, we can just add fields to the
PacketSentstruct, and define how they're marshalled.This new qlog API will allow us, among others: