Skip to content
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

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

Merged
merged 5 commits into from
Jul 1, 2020

Conversation

marten-seemann
Copy link
Member

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

"github.com/lucas-clemente/quic-go/internal/wire"
)

type Tracer interface {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea for a better name?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

type Tracer interface {
TracerForServer(odcid protocol.ConnectionID) ConnectionTracer
TracerForClient(odcid protocol.ConnectionID) ConnectionTracer
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// A ConnectionTracer records events.
type ConnectionTracer interface {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would print the contents of STREAM and CRYPTO frames.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codecov bot commented Jun 30, 2020

Codecov Report

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

Impacted file tree graph

@@            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.

type Tracer interface {
TracerForServer(odcid protocol.ConnectionID) ConnectionTracer
TracerForClient(odcid protocol.ConnectionID) ConnectionTracer
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"github.com/lucas-clemente/quic-go/internal/wire"
)

type Tracer interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// A ConnectionTracer records events.
type ConnectionTracer interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants