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

Metrics SDK - can we cleanup after a shutdown? #2442

Open
cijothomas opened this issue Dec 17, 2024 · 5 comments
Open

Metrics SDK - can we cleanup after a shutdown? #2442

cijothomas opened this issue Dec 17, 2024 · 5 comments

Comments

@cijothomas
Copy link
Member

Though shutdown prevents metrics from ever getting collected/exported, they are still kept in the HashMaps/Aggregations. There is no check in hot path to see if the provider is already shutdown (it may be too expensive to do that check). This maybe okay, but opening an issue to discuss more.

Also, given the Counter/Meter from the API use a Arc<dyn> object pointing to something from the SDK, it is not easily possible to do much cleanup in the SDK, unless the user explicitly drops the Counter/Meters themselves.

@cijothomas cijothomas added this to the Metrics SDK Stable milestone Dec 17, 2024
@cijothomas
Copy link
Member Author

@utpilla @fraillt FYI. Something we can discuss.

@fraillt
Copy link
Contributor

fraillt commented Dec 18, 2024

As you have mentioned, I would prefer to avoid having this check in hot path as well.
However, we should probably clear Pipeline (more precisely inner: Mutex<PipelineInner>) during shutdown, so that memory can be cleaned up once user explicitly drops meters themselves.

@utpilla
Copy link
Contributor

utpilla commented Jan 5, 2025

Also, given the Counter/Meter from the API use a Arc<dyn> object pointing to something from the SDK, it is not easily possible to do much cleanup in the SDK

We can use Weak instead of Arc in the instruments to address this issue. I created #2497 as a proof of concept to show that instruments can be updated to respect the MeterProvider shutdown. We can do the same for Meters as well. In the PR, I have only updated Counter to use Weak.

This comes with a performance cost though as the Weak to Arc upgrade happens on the hot path. On my machine, the benchmarks saw a 5% decline and the stress test throughput went from 9M to 7M iterations per second.

unless the user explicitly drops the Counter/Meters themselves.

This would not always be possible. Especially, if the users are using some instrumentation library and have no visibility and access to the instruments and Meters.

@lalitb
Copy link
Member

lalitb commented Jan 6, 2025

This comes with a performance cost though as the Weak to Arc upgrade happens on the hot path. On my machine, the benchmarks saw a 5% decline and the stress test throughput went from 9M to 7M iterations per second.

This is not related, but just to add. The Arc upgrade indeed is perf heavy operation, which was the reason it was removed from Logger/LoggerProvider in #1455.

@TommyCpp
Copy link
Contributor

TommyCpp commented Jan 7, 2025

Based on the prototype @utpilla did I think the cost is not worth the benifit. Most of the MeterProvider should have the same lifecycle as the application

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

No branches or pull requests

5 participants