search

eaze / tracing-honeycomb

Fork of https://github.com/inanna-malick/tracing-honeycomb
MIT License
17 stars 19 forks source link

Update dependencies to tracing-subscriber 0.3 #12

Closed Infrasonics closed 2 years ago

Infrasonics commented 2 years ago

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.

Infrasonics commented 2 years ago

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

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

ericsampson commented 2 years ago

@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 commented 2 years ago

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

jfhbrook-at-work commented 2 years ago

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

ericsampson commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I made a branch to iterate on these changes with a new PR: https://github.com/eaze/tracing-honeycomb/pull/14

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