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

Don't enable tracing features by default #293

Open
njam opened this issue Jan 19, 2021 · 8 comments · May be fixed by #374
Open

Don't enable tracing features by default #293

njam opened this issue Jan 19, 2021 · 8 comments · May be fixed by #374
Milestone

Comments

@njam
Copy link

njam commented Jan 19, 2021

Currently the "log" feature of the "tracing" dependency is enabled (ref):

[dependencies.tracing]
version = "0.1.17"
features = ["log"]

If I understand correctly, this means that any application using "isahc" will have the tracing "log" feature enabled, with no way to disable it (because cargo features are additive). This will then not only apply to "isahc" but to all other usages of "tracing".

If that is correct, shouldn't this be disabled by default, and only enabled if the application decides to do so?

@sagebind
Copy link
Owner

Currently, yes this is the case.

If that is correct, shouldn't this be disabled by default, and only enabled if the application decides to do so?

Is there a particular problem you are having with this? All this feature does is cause events to be delegated to the log crate if no tracing subscriber is registered, which itself will also do nothing unless a logger is registered. I see no downside to enabling this like we do now, but the downside for conditionally enabling this is no logs for people using log and not tracing who disable default features. For example, if I depend on Isahc like this:

[dependencies.isahc]
version = "1.0"
default-features = false
features = ["json"]

As a consumer I might be surprised to discover that no logs are emitted when using a log logger, but logs are emitted when using a tracing subscriber.

@njam
Copy link
Author

njam commented Jan 19, 2021

I'm using isahc through opentelemetry-jaeger's "isahc_collector_client" feature flag, so I can't set default-features = false (see rust-lang/cargo#2823). I guess the same could be the case for other libraries.

As a consumer I might be surprised to discover that no logs are emitted when using a log logger, but logs are emitted when using a tracing subscriber.

I agree completely for isahc. But unfortunately the decision whether traces should be sent to logging seems to be a global one. So if "isahc" decides to do that, then it decides it for any other libraries in the current project, and the project itself too. So once I have "isahc" in my dependencies I cannot decide anymore if I want the "log" feature in my application or not.

@sagebind
Copy link
Owner

sagebind commented Jan 19, 2021

Sorry if this is too direct, but:

I cannot decide anymore if I want the "log" feature in my application or not.

Why do you want to decide this? Not saying it is wrong, I just want to understand.

@njam
Copy link
Author

njam commented Jan 19, 2021

I agree it seems like tracing's "log" could be a sane default :D But I have the feeling if "tracing" decides to make this feature optional, then "isahc" should not enable it if it doesn't explicitly require it.

In this case my application is using tracing and logging for different use cases, and I don't want to mix the two, so I never want traces sent to the logger.

Feel free to close this ticket if you disagree!
I can set the "max_level_off" feature flag to disable tracing completely, instead of not adding a subscriber, then I won't have logs either.

@sagebind
Copy link
Owner

In this case my application is using tracing and logging for different use cases, and I don't want to mix the two, so I never want traces sent to the logger.

Interesting, I don't think I've heard that use-case before. In this scenario, would you want Isahc to send records to tracing, or to logs? Or neither?

Feel free to close this ticket if you disagree!

I feel like I don't have enough information yet to agree or disagree, I'd probably have to think about it more deeply. That said, changing this would be a backwards-incompatible change regardless and would likely need to be postponed to a version 2.0, whenever that happens.

@njam
Copy link
Author

njam commented Jan 21, 2021

In this scenario, would you want Isahc to send records to tracing, or to logs?

Probably to logs.

I guess the more common use case is to use "log" and "tracing" for the same purpose, and then indeed the current behaviour makes sense.

Maybe a good solution would be a macro that can be used to produce logs and tracing events at the same time. Then each producer of tracing events can decide if they want to also produce logs or not, and it's not a global flag. But that's probably something that should be discussed on the "tracing" repo.

@fkrull
Copy link

fkrull commented Feb 12, 2022

As a datapoint: the "log" feature has a bug where if you call one of the tracing macros in an async context in a particular way, the entire future becomes non-Send. This is particularly insidious if the problematic tracing call is somewhere deep within a dependency and the "log" feature is enabled by another dependency like isahc without your knowledge.

I'm not sure this means the onus should be on isahc to not enable the feature, given this is clearly a bug in tracing and they even advertise the "log" feature as suitable for libraries. But it is an aspect of the situation as it exists right now. 🤷

@sagebind sagebind added this to the 2.0.0 milestone Feb 15, 2022
@sagebind
Copy link
Owner

I've added this to the 2.0 milestone, I think I agree that this is an issue and should be re-thought. It seems like the intent of the log feature in tracing was for libraries to enable it by default, but in practice it does cause issues.

So here's the plan: the tracing dependency will become an optional dependency entirely, and not enabled by default. When enabled, Isahc will emit log entries as tracing events instead. The log feature will never be enabled and it will be up to users to enable it in their application if they want.

Unfortunately I'm not sure of any other way of allowing an application to control whether Isahc emits logs via log or via tracing when Isahc is a transitive dependency; if there were then I'd be open to it.

This has to wait until 2.0 since changing the default feature set is a breaking change.

@sagebind sagebind linked a pull request Feb 15, 2022 that will close this issue
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 a pull request may close this issue.

3 participants