-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
Comments
Currently, yes this is the case.
Is there a particular problem you are having with this? All this feature does is cause events to be delegated to the [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 |
I'm using
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. |
Sorry if this is too direct, but:
Why do you want to decide this? Not saying it is wrong, I just want to understand. |
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! |
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?
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. |
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. |
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. 🤷 |
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 So here's the plan: the Unfortunately I'm not sure of any other way of allowing an application to control whether Isahc emits logs via This has to wait until 2.0 since changing the default feature set is a breaking change. |
Currently the "log" feature of the "tracing" dependency is enabled (ref):
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?
The text was updated successfully, but these errors were encountered: