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

Issues with new logging model #2697

Open
Sushisource opened this issue Feb 21, 2025 · 6 comments
Open

Issues with new logging model #2697

Sushisource opened this issue Feb 21, 2025 · 6 comments
Labels
A-common Area:common issues that not related to specific pillar

Comments

@Sushisource
Copy link
Contributor

Sushisource commented Feb 21, 2025

I recently upgraded from 0.26 -> 0.28 and found that the global error handler has been removed: #2260 in favor of Tokio tracing logging.

That's great, however there are at least a few pain points:

Various things logged at info level that don't need to be: Ex here and here (and many other places). I would argue there is next to no reason for a library like this to log at info level. This example is maybe appropriate, but pretty much any consumer is not going to be interested in lifecycle events like the others.

The new thread for metrics export here will never log properly unless the global tracing subscriber is set. I am a library author consuming this library. I can't set a global subscriber, because my users might want to. However, I do want the subscriber my users tell me to use to be set as the subscriber my dependencies, like otel use. Normally this isn't a problem because I make sure all threads I spawn set the configured subscriber, but I can't control the subscriber for this (or any other) thread spawned internally by otel. It would be great if there was a way to configure the subscriber for exporters, so that it would be used by any spawned threads.

Lastly, I had a test in my code that was ensuring users would see internal ERROR level logs from otel about things they are very likely to care about (namely, failures to export metrics). AFAICT, even when I set a global trace subscriber, I'm seeing no such errors any more. This is the test as it functioned in 0.26, saw this one in particular, and I never see that now.

@cijothomas
Copy link
Member

Agree that some INFO level ones could be revisited to downgrade to DEBUG level. Would you mind sending a PR with your proposal, so we can review and adjust?

@cijothomas
Copy link
Member

The new thread for metrics export here will never log properly unless the global tracing subscriber is set.

True. Do you have a proposal in mind to help ease this? (It's totaly okay if you don't have!) Let us keep this issue open, and look for solutions...

@Sushisource
Copy link
Contributor Author

Sushisource commented Feb 21, 2025

The new thread for metrics export here will never log properly unless the global tracing subscriber is set.

True. Do you have a proposal in mind to help ease this? (It's totaly okay if you don't have!) Let us keep this issue open, and look for solutions...

This was essentially it:

It would be great if there was a way to configure the subscriber for exporters, so that it would be used by any spawned threads.

If PeriodicReaderBuilder accepted an option for a subscriber, as well as the options for any other similar constructs which spawn a thread, that would solve the issue.

@Sushisource
Copy link
Contributor Author

Agree that some INFO level ones could be revisited to downgrade to DEBUG level. Would you mind sending a PR with your proposal, so we can review and adjust?

Sure, I can do this tomorrow.

@cijothomas cijothomas added the A-common Area:common issues that not related to specific pillar label Feb 21, 2025
@cijothomas
Copy link
Member

The new thread for metrics export here will never log properly unless the global tracing subscriber is set.

True. Do you have a proposal in mind to help ease this? (It's totaly okay if you don't have!) Let us keep this issue open, and look for solutions...

This was essentially it:

It would be great if there was a way to configure the subscriber for exporters, so that it would be used by any spawned threads.

If PeriodicReaderBuilder accepted an option for a subscriber, as well as the options for any other similar constructs which spawn a thread, that would solve the issue.

It'll require a lot of work. We depend only on tracing and don't take a dependency on the tracing::subscriber crate intentionally, and leave it up to end users. Accepting a tracing::subscriber also ties internal logging to tracing. While it is the only implementation today, @lalitb has avoided direct usage by exposing otel_info! etc. In future, we could change the implementation from tracing to something else (like firing a callback like before etc.), should there be a need.

Modifying all structs that can spin up a thread to accept subscriber would not be easy in this case. Additionally, we expect these components to offer tokio/async-std (or bring-you-own) mechanisms instead of own thread, which can further complicate this.

I wonder how does other libraries like reqwest etc. handle this issue? Have read that they also spun up own thread and uses tracing for internal logs....

(Am I overthinking 😟 ?)

@Sushisource
Copy link
Contributor Author

Sushisource commented Feb 22, 2025

If the issue is avoiding a dep (which makes sense), you can always accept an on-thread-start hook like Tokio does for its runtime: https://docs.rs/tokio/latest/tokio/runtime/struct.Builder.html#method.on_thread_start

In the case of something like reqwest they're just delegating to the async runtime's functionality there. In the case of the blocking client they don't have a hook. But... they're also not using tracing, they're using log which is a slightly different story and would require a globally set log-to-trace setup anyway.

Looks like they do have a related issue seanmonstar/reqwest#1506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar
Projects
None yet
Development

No branches or pull requests

2 participants