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

Update dependencies to tracing-subscriber 0.3 #12

Closed
wants to merge 3 commits into from

Conversation

Infrasonics
Copy link
Contributor

I ran into a problem when using the current version of tracing-appender with a custom telemetry layer built on tracing-distributed since they renamed the associated function new_span.

Tests for both crates with default config pass with this update, but I'm not a user of honeycomb, so I have not tested further than that.

Feedback is appreciated.

This is necessary for compatibility with `trait Layer<S>` in tracing-subscriber v0.3.3.
Update dependency to tracing_subscriber 0.3
@Infrasonics
Copy link
Contributor Author

The last CI-run failed due to an internal compiler error in nightly as described in rust-lang/rust#91663. They recommend running cargo clean or wait until rust-lang/rust#91715 has been merged.

Therefore I'm not changing this PR and ask for a rerun of the pipeline sometime.

@Fishrock123 Fishrock123 requested review from ericsampson, jfhbrook-at-work and chrisdickinson and removed request for ericsampson December 10, 2021 21:29
@ericsampson
Copy link

@jfhbrook-at-work you're taking this one right?
I think the PR needs to add a minor version bump to the library version, unless I just need more coffee.

@Infrasonics
Copy link
Contributor Author

Good point! Definitely for tracing-distributed; does tracing-honeycomb's patch number need +1 too?

@jfhbrook-at-work
Copy link
Contributor

@ericsampson Yeah, I'm gonna try to look at this in the next few days!

@jfhbrook-at-work jfhbrook-at-work self-assigned this Dec 13, 2021
@ericsampson
Copy link

Good point! Definitely for tracing-distributed; does tracing-honeycomb's patch number need +1 too?

I'd think so. Does this change have any effect on the public API surface or behavior for consumers of tracing-honeycomb?
We're just taking over maintenance of this library from a coworker, so apologies that we're still coming up to speed :)

@Infrasonics
Copy link
Contributor Author

The public API of tracing honeycomb is not influenced from what I understand.

The only change I had to make, in order to make tracing-distributed compatible with the current version of tracing-subscriber, is to rename the new_span function to on_new_span. This impacts implementations of tracing_subscriber::Layer<S>. Since TelemetryLayer implements Layer it's most probably a breaking change for users of tracing-distributed because they use the old API in tracing-subscriber as a dependency.

Nice to hear you'll be maintaining this cool project^^. From my experience of using tracing-distributed directly so far I'll have some more remarks/questions in the near future.

@ericsampson
Copy link

ericsampson commented Dec 14, 2021

From my experience of using tracing-distributed directly so far I'll have some more remarks/questions in the near future.

Sounds great!!!

@jfhbrook-at-work
Copy link
Contributor

I made a branch to iterate on these changes with a new PR: #14

I'm going to close this one in favor of #14, which I plan on releasing during the break!

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.

3 participants