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

Extend qlog support #4201

Closed
wants to merge 2 commits into from
Closed

Extend qlog support #4201

wants to merge 2 commits into from

Conversation

birneee
Copy link
Contributor

@birneee birneee commented Dec 13, 2023

  • support QLOGDIR environment variable
  • support chosen_alpn qlog event
  • support stream_data_moved event

Covers #4189

- support QLOGDIR environment variable
- support chosen_alpn qlog event
- support stream_data_moved event
Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Can you split this up into 3 separate PRs?

  1. The ALPN event totally makes sense to me.
  2. What's the motivation for the stream_data_moved event? How would you use it?
  3. The QLOGDIR should be optional, i.e. not be implemented in the quic package. Having a separate constructor in the qlog package makes sense to me.

@@ -34,6 +35,8 @@ type ConnectionTracer struct {
LossTimerExpired func(TimerType, EncryptionLevel)
LossTimerCanceled func()
ECNStateUpdated func(state ECNState, trigger ECNStateTrigger)
ChoseAlpn func(protocol string)
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if we could also log server_alpns and client_alpns, but it doesn't seem possible without wrapping various callback on the tls.Config, which we should avoid doing.

Suggested change
ChoseAlpn func(protocol string)
ChoseALPN func(protocol string)

@birneee
Copy link
Contributor Author

birneee commented Dec 13, 2023

Can you split this up into 3 separate PRs?

I can :)

What's the motivation for the stream_data_moved event? How would you use it?

I would like to test/benchmark the "end-to-end"-performance that I can expect on the application layer in some scenarios. From the current qlog it is not obvious how far a stream has progressed, because of loss and reordering. For now I have a Python script that tracks the stream gaps from the qlog stream 😅

The QLOGDIR should be optional

Not sure if I understand you correctly, you mean just a separate NewQlogDirConnectionTracer?

@marten-seemann
Copy link
Member

The QLOGDIR should be optional

Not sure if I understand you correctly, you mean just a separate NewQlogDirConnectionTracer?

Yes, pretty much. Then we can tell users "If you want to record qlogs every time the QLOGDIR is set, set the Config.Tracer to qlog.NewQLOGDirTracer.

@marten-seemann
Copy link
Member

Closing, since we've had support for the ALPN event and the QLOGDIR for a while now. Still not sure what to do about the data moved event.

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.

None yet

2 participants