split the qlog package into a logging and a qlog package, use a tracer interface in the quic.Config #2638

marten-seemann · 2020-06-29T06:35:49Z

This is the first step towards a more general logging approach, that will ultimately allow us to run both qlog and a metrics collector.

marten-seemann · 2020-06-29T06:38:48Z

logging/interface.go

+	"github.com/lucas-clemente/quic-go/internal/wire"
+)
+
+type Tracer interface {


Any idea for a better name?

Not really. Logger maybe, but that's already taken.

marten-seemann · 2020-06-29T06:40:24Z

logging/interface.go

+type Tracer interface {
+	TracerForServer(odcid protocol.ConnectionID) ConnectionTracer
+	TracerForClient(odcid protocol.ConnectionID) ConnectionTracer
+}


We'll need to add another method here for logging events not related to any particular connection (e.g. stateless resets sent, Version Negotiation packets sent, packets that get dropped before they are sent to a connection, etc.).
Any suggestion for a name?

Why a single method, and not one per event, like in the ConnectionTracer?

That makes sense. Good thing about it, we don't need to come up with another name (NonConnectionTracker?).

marten-seemann · 2020-06-29T06:46:42Z

logging/interface.go

+}
+
+// A ConnectionTracer records events.
+type ConnectionTracer interface {


To make this useful, we'll have to make sure that all function parameters don't use any internal types. For most types, we can get away with defining type alias, e.g.

type MaxDataFrame = wire.MaxDataFrame

Not sure if the fact the MaxDataFrame uses a protocol.ByteCount (which is also an internal type) will cause any problems here.

For some types, we might want to redefine the type. For example, we don't want to pass the contents of STREAM and CRYPTO frames to the logger:

type StreamFrame struct { Offset protocol.ByteCount StreamID protocol.StreamID Length int }

Suggestion:
We'll create a new package logging/internal/to_logging, that defines conversion for these types:

func ConvertFrame(*wire.Frame) *logging.Frame

SGTM. Maybe we can reuse this conversion in the LogFrame function?

Actually I wonder whether the Logger can eventually be yet another tracer… Probably not?

That would be an option. We could then define a String() string method on the logging.Frame, and get rid of the massive switch statement in LogFrame.

Maybe you don't even need that String() and can just use the fmt print?

That would print the contents of STREAM and CRYPTO frames.

But the LogFrame wouldn't include those, and instead have useful metadata (len, etc)? I guess the downside would be that we don't get to format fields usefully (hex vs dec e.g.), but not sure whether that's worth the complexity?

codecov · 2020-06-30T15:36:48Z

Codecov Report

Merging #2638 into master will decrease coverage by 0.06%.
The diff coverage is 69.52%.

@@            Coverage Diff             @@
##           master    #2638      +/-   ##
==========================================
- Coverage   86.39%   86.32%   -0.06%     
==========================================
  Files         120      122       +2     
  Lines        9748     9754       +6     
==========================================
- Hits         8421     8420       -1     
- Misses        991      999       +8     
+ Partials      336      335       -1

Impacted Files	Coverage Δ
internal/ackhandler/ackhandler.go	`0.00% <0.00%> (ø)`
internal/ackhandler/sent_packet_handler.go	`73.04% <6.67%> (ø)`
internal/handshake/crypto_setup.go	`66.95% <22.22%> (ø)`
internal/handshake/updatable_aead.go	`91.34% <33.33%> (ø)`
server.go	`84.57% <50.00%> (+0.25%)`	⬆️
logging/types.go	`74.29% <74.29%> (ø)`
qlog/qlog.go	`93.73% <83.78%> (-2.21%)`	⬇️
session.go	`76.60% <86.57%> (+0.18%)`	⬆️
client.go	`73.05% <100.00%> (-0.19%)`	⬇️
config.go	`100.00% <100.00%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c42d79...ac60622. Read the comment docs.

lucas-clemente · 2020-07-01T07:08:26Z

logging/interface.go

+type Tracer interface {
+	TracerForServer(odcid protocol.ConnectionID) ConnectionTracer
+	TracerForClient(odcid protocol.ConnectionID) ConnectionTracer
+}


Why a single method, and not one per event, like in the ConnectionTracer?

lucas-clemente · 2020-07-01T07:08:56Z

logging/interface.go

+	"github.com/lucas-clemente/quic-go/internal/wire"
+)
+
+type Tracer interface {


Not really. Logger maybe, but that's already taken.

lucas-clemente · 2020-07-01T07:10:04Z

logging/interface.go

+}
+
+// A ConnectionTracer records events.
+type ConnectionTracer interface {


Actually I wonder whether the Logger can eventually be yet another tracer… Probably not?

marten-seemann requested a review from lucas-clemente June 29, 2020 06:35

marten-seemann force-pushed the logging-tracer branch from 6b43b84 to 27cedc4 Compare June 29, 2020 06:36

marten-seemann commented Jun 29, 2020

View reviewed changes

lucas-clemente approved these changes Jul 1, 2020

View reviewed changes

marten-seemann added 5 commits July 1, 2020 16:49

split the qlog package into a logging and a qlog package

78ba619

rename qlogger (member) variables to tracer

7fdace4

replace the logging.Export() method by a Close() method

7f6eca8

rename the logging.Tracer to ConnectionTracer

4121ea8

replace the GetLogWriter quic.Config option by a Tracer interface

ac60622

marten-seemann force-pushed the logging-tracer branch from 27cedc4 to ac60622 Compare July 1, 2020 09:57

marten-seemann merged commit 00f8028 into master Jul 1, 2020

marten-seemann deleted the logging-tracer branch July 1, 2020 10:10

marten-seemann mentioned this pull request Jul 6, 2020

fix crash when the qlog callbacks returns a nil io.WriteCloser #2648

Merged

aschmahmann mentioned this pull request Sep 22, 2020

Release 0.7.0 ipfs/kubo#7560

Closed

72 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

split the qlog package into a logging and a qlog package, use a tracer interface in the quic.Config #2638

split the qlog package into a logging and a qlog package, use a tracer interface in the quic.Config #2638

marten-seemann commented Jun 29, 2020

marten-seemann Jun 29, 2020

lucas-clemente Jul 1, 2020

marten-seemann Jun 29, 2020

lucas-clemente Jul 1, 2020

marten-seemann Jul 1, 2020

marten-seemann Jun 29, 2020

lucas-clemente Jul 1, 2020

lucas-clemente Jul 1, 2020

marten-seemann Jul 1, 2020

lucas-clemente Jul 1, 2020

marten-seemann Jul 1, 2020

lucas-clemente Jul 1, 2020

codecov bot commented Jun 30, 2020 •

edited

Loading

lucas-clemente Jul 1, 2020

lucas-clemente Jul 1, 2020

lucas-clemente Jul 1, 2020

split the qlog package into a logging and a qlog package, use a tracer interface in the quic.Config #2638

split the qlog package into a logging and a qlog package, use a tracer interface in the quic.Config #2638

Conversation

marten-seemann commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 30, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 30, 2020 •

edited

Loading