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

[API] Remove Meters #2205

Merged
merged 12 commits into from
Jul 10, 2023
Merged

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jun 27, 2023

Fixes #2184

Changes

Changes:

  • CMake: Add an experimental option, WITH_REMOVE_METER_PREVIEW.
  • API: expose the new method MeterProvider::RemoveMeter().
  • SDK: implementation for MeterProvider::RemoveMeter().
  • Cleanup misc usage of the opentelemetry::v1 namespace, to make the implementation independent of the ABI version exposed.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Merging #2205 (f7c5320) into main (8b61318) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

❗ Current head f7c5320 differs from pull request most recent head c894d5b. Consider uploading reports for the commit c894d5b to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2205      +/-   ##
==========================================
- Coverage   87.49%   87.31%   -0.18%     
==========================================
  Files         169      169              
  Lines        4891     4900       +9     
==========================================
- Hits         4279     4278       -1     
- Misses        612      622      +10     
Impacted Files Coverage Δ
api/include/opentelemetry/metrics/meter_provider.h 100.00% <ø> (ø)
api/include/opentelemetry/metrics/noop.h 37.26% <ø> (ø)
sdk/src/metrics/meter_context.cc 77.78% <0.00%> (-11.11%) ⬇️
sdk/src/metrics/meter_provider.cc 86.12% <ø> (ø)

... and 1 file with indirect coverage changes

@marcalff marcalff marked this pull request as ready for review June 30, 2023 12:56
@marcalff marcalff requested a review from a team June 30, 2023 12:56
@marcalff marcalff changed the title [RFC][API/SDK] Remove Instruments and Meters [API/SDK] Remove Meters Jun 30, 2023
@lalitb
Copy link
Member

lalitb commented Jun 30, 2023

Thanks for the PR. Few questions before reviewing it :)

  • Just curious why is this issue blocker for the shared libraries. Or do we mean it is a blocker for scenarios when applications dynamically load (dlopen()) and unload (dlclose()) shared libraries, and the Meter should be removed once the shared library is unloaded?

  • Do we need to increment the ABI version for this change, as the new API is still not part of the specs (and mayn't be eventually)? Instead, as of now, we can introduce it under macro say - OPENTELEMETRY_REMOVE_METERS. Once this is part of the specification, we can increment ABI version for this.

  • Looks like this PR is also introducing maintaining multiple ABI versions - maintaining all older versions would add complexity to the code, and need some discussion (refer - Prepare release, include fixes with abi breaking changes #1983 (comment)). As of now, should be keep it separate from this PR ?

@marcalff
Copy link
Member Author

marcalff commented Jul 3, 2023

@lalitb

Thanks for the PR. Few questions before reviewing it :)

* Just curious why is this issue blocker for the shared libraries. Or do we mean it is a blocker for scenarios when applications dynamically load (dlopen()) and unload (dlclose()) shared libraries, and the Meter should be removed once the shared library is unloaded?

This is a blocker for scenarios when applications use dlopen() and dlclose() during normal operations (i.e., not just on the application startup and shutdown, but during the application workload).

Disclosure:

I happen to be instrumenting such an application, and in our case, the issue is so severe that we just can not use opentelemetry for metrics, at all, if this is not resolved.

The proper label can be either "blocking", or "priority:p0", no strong opinion here, as long as it gets addressed, hence the PR.

* Do we need to increment the ABI version for this change, as the new API is still not part of the specs (and mayn't be eventually)?  Instead, as of now, we can introduce it under macro say - OPENTELEMETRY_REMOVE_METERS. Once this is part of the specification, we can increment ABI version for this.

This is a two-fold question:

  • RemoveMeter() is not part of ABI v1, so when this gets added to the ABI, it must be under ABI v2 in any case, as v1 can not change.
  • This API is not (yet) part of the spec either, which raise the question if it will ever be there, and if so, if the spec will use the prototype proposed, or something similar but different. I agree that we need a feature flag to clearly identify apis that are not in the spec, to convey the notion that the api is experimental and subject to change, independently of the ABI version used.
* Looks like this PR is also introducing maintaining multiple ABI versions - maintaining all older versions would add complexity to the code, and need some discussion (refer - [Prepare release, include fixes with abi breaking changes #1983 (comment)](https://github.com/open-telemetry/opentelemetry-cpp/issues/1983#issuecomment-1440755915)). As of now, should be keep it separate from this PR ?

I will split this PR to keep only the fix itself, and move the ABI changes with #1983, for clarity, and to merge this PR sooner.

The discussion on ABI still needs to happen, as this is blocking many fixes, but sure, let's do that separately.

@marcalff marcalff changed the title [API/SDK] Remove Meters [API] Remove Meters Jul 3, 2023
@marcalff
Copy link
Member Author

marcalff commented Jul 3, 2023

Now ready for review.

@marcalff marcalff added the priority:p0 Issues which require immediate action to stop bleeding label Jul 7, 2023
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. Good to add a unit test. Also, should we enable this macro in the maintainers CI?

Copy link
Member

@esigo esigo left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks :)

@esigo esigo added the ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) label Jul 9, 2023
@marcalff
Copy link
Member Author

LGTM. Good to add a unit test. Also, should we enable this macro in the maintainers CI?

Thanks for the review.

Added unit test, and coverage in maintainers CI.

@marcalff marcalff merged commit 1111f34 into open-telemetry:main Jul 10, 2023
41 checks passed
@marcalff marcalff deleted the fix_remove_meter_2184 branch September 5, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-merge The PR is ok to merge (has two approves or raised by a maintainer/approver and has one approve) priority:p0 Issues which require immediate action to stop bleeding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] Remove Meters
3 participants