tokio-rs / tracing

Application level tracing for Rust.
https://tracing.rs
MIT License
5.51k stars 721 forks source link

Rename `tracing_subscriber::layer::Layer` to something closer to `Subscriber`. #641

Closed davidbarsky closed 4 years ago

davidbarsky commented 4 years ago

Feature Request

Crates

Motivation

"Layer" is a somewhat abstract name for describing what it is actually: a composable subscriber. Given the centrality of the Subscriber trait and our direction to end-customers to use the Layer trait as much as possible, I think we should attempt to communicate the "subscriber"-ness of Layer to end-users as much as possible. It would also align the Layer trait closer to the crate's name, tracing-subscriber.

Proposal

(The process for renaming is relatively simple, so most of this proposal will focus on non-tracing-core breaking changes.) Here are a few options:

Alternatives

hawkw commented 4 years ago

I'm definitely +1 for renaming Layer to Subscriber. What I'm not totally sure about is whether we should make that change independently of renaming tracing-core's Subscriber — on one hand, the re-export means we don't have to wait until we have other breaking changes in core to make, but on the other, I'm not super excited about having two different traits named Subscriber in two different crates, which seems like it could cause some confusion.

Additionally, if we reexport tracing_core::Subscriber as CoreSubscriber, we'd have to do a deprecation cycle for the current re-export, unless we have other breaking changes to make in tracing. If we do decide to rename tracing_core::Subscriber to something else (I like Collector) later on, we would have to do a second deprecation. Unless there's a breaking change to tracing in between, that would mean that tracing would expose a deprecated Subscriber trait, a deprecated CoreSubscriber trait, and a Collector trait...and all of them would be the same trait.

davidbarsky commented 4 years ago

I'm definitely +1 for renaming Layer to Subscriber. What I'm not totally sure about is whether we should make that change independently of renaming tracing-core's Subscriber — on one hand, the re-export means we don't have to wait until we have other breaking changes in core to make, but on the other, I'm not super excited about having two different traits named Subscriber in two different crates, which seems like it could cause some confusion.

I think this can be mitigated through loud documentation telling users to be aware of the distinction in tracing, tracing-core, and tracing-subscriber and a short deprecation cycle. I think that means we should consider what else we want to break in tracing-core relatively soon. If I recall correctly, you want to:

REMOVE THE 'static LIFETIME FROM enabled AND ONLY PASS STATIC META INTO register_callsite

Given that, I think we should probably thinking about what we want to break in tracing-core?

Additionally, if we reexport tracing_core::Subscriber as CoreSubscriber, we'd have to do a deprecation cycle for the current re-export, unless we have other breaking changes to make in tracing. If we do decide to rename tracing_core::Subscriber to something else (I like Collector) later on, we would have to do a second deprecation. Unless there's a breaking change to tracing in between, that would mean that tracing would expose a deprecated Subscriber trait, a deprecated CoreSubscriber trait, and a Collector trait...and all of them would be the same trait.

That's a solid point; I didn't consider the impact of those different aliases. For what its worth, I'm happy to move straight over to Collector—I think I like that more than CoreSubscriber anyways and it communicates its actual purpose a bit more clearly than CoreSubscriber despite it being a larger departure. In that situation, we'd have a situation where Collector and Subscriber point to the same trait, but that feels less confusing than the situation you laid out.


If we're renaming/deprecating anything, I think setting a budget of a single deprecated alias per entity, per breaking change cycle is a good goal. We'd need to be really confident that the new name is something we'd like to stick with prior to cutting a release to crates.io with that change.

yaahc commented 4 years ago

+1 for Subscriber/Collector, though I'm less of a fan of Collector, to me Registry makes more sense, but also y'all are the experts, so tioli.

jtescher commented 4 years ago

Subscriber/Collector seem like a change in the right direction. Naming the core trait is tricky as it does a few things, but a Registry being a Collector + LookupSpan seems to make sense currently.

samschlegel commented 4 years ago

I like Collector, as the word "collect" is used very frequently in the docs though to describe what a Subscriber does. My mind does first jump to Registry, but I do think I would expect a Registry to also have LookupSpan, or something more than the current interface of Subscriber

That being said, something still doesn't sit right with me. A true Subscriber shouldn't affect upstreams like how Layers currently do, so that rename doesn't feel right to me. I think the Layer/Subscriber naming and split is actually good. I just think that it feels Subscribers currently do too much. I'd want to be able to register multiple Subscribers, but as designed one has to be authoritative and global for Id generation. I'd also like to not have to care about maintaining the current span. I also think it makes sense to not have to re-implement common filtering logic in every subscriber, hence why Layers are important.

tl;dr cause it's sunday and my brain is tired: I do think Layers currently feel like what Subscribers should feel like, but I don't think the design of having to stack Layers makes sense for what I'd want from a Subscriber, since Subscribers should be indenpendent. But I'd also still want something like Layers for things like EnvFilter, so maybe Filter instead of Layer?

edit: maybe we could also shamelessly copy https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/sdk.md naming 👀

davidbarsky commented 4 years ago

@samschlegel Thanks for commenting and sorry for the delay in responding! I think we'll eventually have a dedicated Filter trait that can be composed with Collectors and Subscribers1 that describes the behavior of EnvFilter. I think that should address your misgivings. For compositional reasons, I'd expect that the hypothetical Filter trait must also be aware of Collectors and Subscribers, but it'll be less in-your-face than the current arrangement.


1 Assuming we rename tracing_core::Subscriber to Collector and tracing_subscriber::layer::Layer to be Subscriber, which I'm personally leaning in favor of...