Don't enable tracing features by default

[njam]

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]

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]

I'm using isahc through opentelemetry-jaeger's "isahc_collector_client" feature flag, so I can't set default-features = false (see https://github.com/rust-lang/cargo/issues/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]

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]

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]

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]

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]

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]

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.