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

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

Closed
davidbarsky opened this issue Mar 22, 2020 · 6 comments · Fixed by #1015
Closed

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

davidbarsky opened this issue Mar 22, 2020 · 6 comments · Fixed by #1015
Assignees
Labels
crate/core Related to the `tracing-core` crate crate/subscriber Related to the `tracing-subscriber` crate crate/tracing Related to the `tracing` crate kind/rfc A request for comments to discuss future changes meta/breaking This is a breaking change, and should wait until the next breaking release.
Milestone

Comments

@davidbarsky
Copy link
Member

Feature Request

Crates

  • tracing-subscriber

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:

  • Re-export tracing_core::Subscriber as CoreSubscriber in tracing; rename Layer to Subscriber. Optionally, at some later point, break tracing_core and rename Subscriber to CoreSubscriber. In documentation, emphasize that CoreSubscriber is low-level trait and that tracing_subscriber::Subscriber should be preferred whenever possible.
  • Rename Layer to ComposableSubscriber. I'm not a fan of this option; ComposableSubscriber is a bit of a mouthful.

Alternatives

  • Don't do this.
@davidbarsky davidbarsky added crate/subscriber Related to the `tracing-subscriber` crate crate/core Related to the `tracing-core` crate crate/tracing Related to the `tracing` crate kind/rfc A request for comments to discuss future changes labels Mar 22, 2020
@hawkw
Copy link
Member

hawkw commented Mar 22, 2020

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
Copy link
Member Author

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
Copy link
Collaborator

yaahc commented Mar 27, 2020

+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
Copy link
Collaborator

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.

@hawkw hawkw added this to the tracing 0.2 milestone Apr 9, 2020
@hawkw hawkw added the meta/breaking This is a breaking change, and should wait until the next breaking release. label Apr 16, 2020
@samschlegel
Copy link
Contributor

samschlegel commented Apr 26, 2020

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
Copy link
Member Author

davidbarsky commented Aug 13, 2020

@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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/core Related to the `tracing-core` crate crate/subscriber Related to the `tracing-subscriber` crate crate/tracing Related to the `tracing` crate kind/rfc A request for comments to discuss future changes meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants