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

core: remove Collect default impls for 0.2 #1262

Open
hawkw opened this issue Feb 23, 2021 · 1 comment
Open

core: remove Collect default impls for 0.2 #1262

hawkw opened this issue Feb 23, 2021 · 1 comment
Labels
crate/core Related to the `tracing-core` crate good first issue Good for newcomers kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.

Comments

@hawkw
Copy link
Member

hawkw commented Feb 23, 2021

Feature Request

Crates

  • tracing-core

Motivation

The Collect trait currently has several methods with default implementations that return an Option or some other "empty" or "default" type. In many cases, this isn't because it makes sense for those methods to be optional, but because they were added after v0.1.0 was released and making them mandatory would have been a breaking change. This can sometimes cause issues when implementations forget to override the default implementations and miss out on important behavior.

Proposal

For 0.2, we should consider making some of these methods mandatory so that implementations can't overlook them. In particular:

  • Collect::max_level_hint probably shouldn't have a default None return value (and we should maybe get rid of the Option? is it actually worth differentiating between "may enable up to TRACE" and "just doesn't care about levels at all"?)
  • Collect::current_span defaults to returning span::Current::unknown. Saying "I don't know what the current span is" should be an explicit decision on behalf of the collector in 0.2
@hawkw hawkw added kind/feature New feature or request good first issue Good for newcomers crate/core Related to the `tracing-core` crate meta/breaking This is a breaking change, and should wait until the next breaking release. labels Feb 23, 2021
@hawkw hawkw added this to the tracing-core 0.2 milestone Feb 23, 2021
@lenaschoenburg
Copy link
Contributor

If we remove the default implementation of Collect::current_span we would need to make tracing_core::span::Current::unknown() public so that all implementers of Collect that relied on the default implementation have access to it.
This would also mean new imports of tracing_core in a bunch of places. Is this okay or should we re-export tracing_core::span::Current as tracing::span::Current?

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 good first issue Good for newcomers kind/feature New feature or request meta/breaking This is a breaking change, and should wait until the next breaking release.
Projects
None yet
Development

No branches or pull requests

2 participants