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 API] [Breaking] Enforce meter name, version schema_url to be static string slices #2112

Merged

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Sep 12, 2024

Context

  • MeterProvider trait is not object safe because the methods of MeterProvider trait have generic parameters
  • Since it's not object safe, we cannot use it as it is to switch from a no-op provider to an SDK provider
  • We have a public struct GlobalMeterProvider and a trait ObjectSafeMeterProvider to work around this
  • Meter name, version and schema_url should be static string slices. Doing so would also make MeterProvider object safe.

Changes

  • Make MeterProvider trait object safe by updating its method signatures to accept &'static str types instead of the generic impl Into<Cow<'static, str>>
  • Remove the public struct GlobalMeterProvider and ObjectSafeMeterProvider trait as they are no longer necessary

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)

@utpilla utpilla requested a review from a team September 12, 2024 21:31
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 78.3%. Comparing base (d0a4fd0) to head (a85340c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry/src/global/metrics.rs 50.0% 4 Missing ⚠️
opentelemetry/src/metrics/noop.rs 0.0% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2112     +/-   ##
=======================================
- Coverage   78.3%   78.3%   -0.1%     
=======================================
  Files        121     121             
  Lines      20815   20767     -48     
=======================================
- Hits       16309   16269     -40     
+ Misses      4506    4498      -8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@TommyCpp TommyCpp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the concern here is object safety. I wonder if we can ask the implementation of MeterProvider to be Sized. Something like this https://rust-lang.github.io/api-guidelines/flexibility.html#traits-are-object-safe-if-they-may-be-useful-as-a-trait-object-c-object

@TommyCpp
Copy link
Contributor

One less public API to maintain

I don't think it's a huge overhead. We basically only repeating the MeterProvider trait implementation here, which has been pretty stable. In generally I want to use generic parameters as much as possible. I think in this case the maintance overhead is not worth the regression on readbitliy or ergonomics

@utpilla
Copy link
Contributor Author

utpilla commented Sep 12, 2024

If the concern here is object safety. I wonder if we can ask the implementation of MeterProvider to be Sized. Something like this https://rust-lang.github.io/api-guidelines/flexibility.html#traits-are-object-safe-if-they-may-be-useful-as-a-trait-object-c-object

@TommyCpp The concern here is the extra bit of code (GlobalMeterProvider + ObjectSafeMeterProvider) which exists ONLY to get around MeterProvider not being object safe. I think we should get rid of this code as it seems unnecessary.

The Self:Sized approach won't help as it would exclude the methods from the trait object which means we wouldn't be able to call meter() or versioned_meter() on the MeterProvider trait object.

@utpilla
Copy link
Contributor Author

utpilla commented Sep 12, 2024

One less public API to maintain

I don't think it's a huge overhead. We basically only repeating the MeterProvider trait implementation here, which has been pretty stable. In generally I want to use generic parameters as much as possible. I think in this case the maintance overhead is not worth the regression on readbitliy or ergonomics

Apart from reducing the maintenance overhead, removing the public API also allows us to change things in the global metrics setup more freely if required in future. We won't be tying the setup process to any type other than MeterProvider.

There is no regression on readability/ergonomics, if the user is using global::meter() to create the meter. I believe our documentation also showcases this as the approach to create Meter. The breaking change will affect users only if they create meters using SdkMeterProvider.meter(). If we feel that there would be a lot of users who might be calling meter() on SdkMeterProvider instead of global we could consider not making this change.

@TommyCpp
Copy link
Contributor

TommyCpp commented Sep 12, 2024

removing the public API also allows us to change things in the global metrics setup more freely if required in future

Generally I'd agree. But in this case GlobalMeterProvider is just implementing the MeterProvider, which is public anyways. Thus I see little risk GlobalMeterProvider will prevent us from making any breaking changes. Do you have any example that GlobalMeterProvider would be a problem in terms of blocking changes?

@TommyCpp
Copy link
Contributor

There is no regression on readability/ergonomics, if the user is using global::meter() to create the meter

True. I am thinking about the regression on readability/ergonomics for users that don't use global module. I feel even if they are probably not the majority, the reward we get from this PR doesn't warrant making their life harder. But it's pretty subjective

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good cleanup, but lets do it by making 'static str for MeterNames, as discussed here : #2112 (comment)

@@ -56,9 +51,9 @@ pub trait MeterProvider {
/// default name will be used instead.
fn versioned_meter(
&self,
name: impl Into<Cow<'static, str>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stormshield-fabs FYI. #1527 is still to be added for Metrics, which will remove versioned_meter in favor of builder pattern.

@utpilla utpilla changed the title Proposal: Remove ObjectSafeMeterProvider and GlobalMeterProvider Remove ObjectSafeMeterProvider and GlobalMeterProvider Sep 16, 2024
@utpilla utpilla changed the title Remove ObjectSafeMeterProvider and GlobalMeterProvider [Breaking] Remove ObjectSafeMeterProvider and GlobalMeterProvider by enforcing meter name,version,schema_url to be static string slices Sep 16, 2024
@utpilla utpilla changed the title [Breaking] Remove ObjectSafeMeterProvider and GlobalMeterProvider by enforcing meter name,version,schema_url to be static string slices [Breaking] Enforce meter name, version schema_url to be static string slices Sep 16, 2024
Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Please follow up with a breaking change entry to changelog for this (and previous 2 PRs that changed public api)

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Based on the following assumptions:

  • Meter names are generally recommended to be static strings defined at compile time.
  • There will be no need to make object unsafe addition to MeterProvider trait in future.

@cijothomas
Copy link
Member

@TommyCpp merging now to unblock progress on api. We can follow up if there are additional feedback to be addressed.

@cijothomas cijothomas merged commit 3976f3d into open-telemetry:main Sep 17, 2024
24 of 25 checks passed
@utpilla utpilla changed the title [Breaking] Enforce meter name, version schema_url to be static string slices [Metrics API] [Breaking] Enforce meter name, version schema_url to be static string slices Oct 15, 2024
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.

4 participants