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

Expose a function for retrieving stats about a connection #4

Merged
merged 2 commits into from
Dec 22, 2022

Conversation

andrewvy
Copy link
Contributor

@andrewvy andrewvy commented Dec 22, 2022

I wanted a way for exposing ConnectionStats and came up with a quick POC of storing the internal Quinn connection when the successful connection is made. This lets you call .stats() on it later on, which is useful for grabbing the RTT / net stats for observability.

I'm not sure if this is the best way about going about it, so I'm up for amending this in whichever way you see fit! (I'm not sure if we want to just re-export the ConnectionStats struct from quinn-proto, it seems quinn itself doesn't re-export this for some reason.. )

@Henauxg
Copy link
Owner

Henauxg commented Dec 22, 2022

Thank you for this PR !

I took the liberty to make a few changes :

  • Store the internal Quinn connection inside the connection's Connected state
  • I finally went with the re-export of quinn-proto ConnectionStats. I did hesitate a lot and still am not 100% sure about this too. A few reasons that made me decide:
    • this feels simpler than maintaining a duplicate of quinn-proto stats structs
    • for now, Quinn is the only back-end, so exposing some of it is not a problem
    • this also has the benefit that all the existing information are available right away, we won't have to add more and more to fulfill possible future needs
    • this is not a critical part of the API, we can still change it later if need be

If this all seems good to you, I can go ahead and merge it

@andrewvy andrewvy marked this pull request as ready for review December 22, 2022 16:38
@andrewvy
Copy link
Contributor Author

@Henauxg Yep, works for me!

@Henauxg Henauxg merged commit 3e886b4 into Henauxg:main Dec 22, 2022
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