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

add tracing instrumentation #247

Merged
merged 10 commits into from
Jun 23, 2024
Merged

add tracing instrumentation #247

merged 10 commits into from
Jun 23, 2024

Conversation

Ruben2424
Copy link
Contributor

This adds tracing to most connection relevant functions. This should make it easier to debug.

@Ruben2424 Ruben2424 requested a review from seanmonstar June 21, 2024 19:18
@seanmonstar
Copy link
Member

It'd be nice if tracing could be optional. I was recently reminded of the desire for this in h2: hyperium/h2#713

@Ruben2424
Copy link
Contributor Author

@seanmonstar, I added features for h3 and h3-quinn to enable tracing. What do you think?

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Looks good to me. For what it's worth, you can name it tracing. Under features in the Cargo.toml, it could be tracing = ["dep:tracing"]. I think something like that is cleaner, it doesn't need to include the prefix of the crate (like h3-) in its own features.

@Ruben2424
Copy link
Contributor Author

Looks good to me. For what it's worth, you can name it tracing. Under features in the Cargo.toml, it could be tracing = ["dep:tracing"]. I think something like that is cleaner, it doesn't need to include the prefix of the crate (like h3-) in its own features.

I added the prefix because it confused me to have two crates with the same feature in one repo. When I call cargo test --all --features=tracing does it compile h3 or h3-quinn with the tracing feature. Or both?

@Ruben2424 Ruben2424 merged commit edada98 into hyperium:master Jun 23, 2024
14 checks passed
@Ruben2424 Ruben2424 mentioned this pull request Jun 23, 2024
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