Skip to content

Refactor SdkMeterProvider with Inner Structure for Better Lifecycle Control #1663

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

Merged
merged 7 commits into from
Apr 19, 2024

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Apr 16, 2024

Fixes #1661
Design discussion issue (if applicable) #1661

Changes

  • Refactor SdkMeterProvider by encapsulating its core functionality in a new SdkMeterProviderInner structure.
  • Drop trait now on SdkMeterProviderInner to ensure cleanup only occurs after the last reference is dropped.
  • Added unit test to validate that shutdown through Drop trait is triggered only when all SdkMeterProvider clones are dropped.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team April 16, 2024 07:15
@TommyCpp
Copy link
Contributor

It may not directly related to this PR but what's our general guideline on shutdown behavior when users call shutdown explicitly on cloned singal providers.

I see two possibility here:

  1. If shutdown is called on any clone of the signal provider, we should shutdown the provider(but not drop resource, as the underlying resource lifetime hasn't ended)
  2. shutdown only works when the singal provider gets dropped(essentially make it part of the Drop of the resource)

I want to make sure we aligned on 1

@lalitb
Copy link
Member Author

lalitb commented Apr 17, 2024

I want to make sure we aligned on 1

Yes, agree on this in case of explicit shutdown invoked on any of the cloned signal provider. ( and #1643 handles this for logs).

In the case when shutdown is not invoked, the shutdown will be done when the last cloned signal provider is dropped. I believe this is the existing behavior with Logs and Traces, and the current PR ensures metrics also aligns to that.

@bwalk-at-ibm
Copy link

Maybe I don't fully understand the semantics here, does that mean that all cloned instances of a signal provider give access to the same singleton resource? If so, then why implement Clone for signal providers at all? What would be the usecase for a cloned signal provider except sharing which can be done via reference?

@lalitb
Copy link
Member Author

lalitb commented Apr 18, 2024

What would be the usecase for a cloned signal provider except sharing which can be done via reference?

Good question, and thanks for bringing this up.

  • For logs and traces, the logger, and tracer structs maintain the clones (internally Arc) to their providers. These clones are used for accessing processors while emitting logs/spans. Reference to providers can't be used as these loggers/tracers can outlive the original scope where the provider was created. And, setting global singleton is optional, and application may or mayn't do it.
  • For Metrics, the only use-case for cloning is when (optionally) setting the global singleton meter provider, and then application may needs the handle to this provider for shutdown/flush scenario.
       let provider = super::SdkMeterProvider::builder()
           .with_reader(reader.clone())
           .build();
       global::set_meter_provider(provider.clone());
        ....
        // and later
        provider.flush()
       // and eventually
        provider.shutdown()

we can possibly use reference to the global meter provider (as this is the static instance), and thus restrict Clone, but kept it for consistency with other signals. And since this additional Arc redirection is not used in the hot path of measurement recording, I didn't see much concern in having it. However, open to any suggestions to simplify this :)

@TommyCpp TommyCpp merged commit 8f27e26 into open-telemetry:main Apr 19, 2024
15 checks passed
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 this pull request may close these issues.

[Bug]: Global metrics provider gets shutdown when local provider is dropped
4 participants