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 #2184

Closed
marcalff opened this issue Jun 8, 2023 · 4 comments · Fixed by #2205
Closed

[API] Remove Meters #2184

marcalff opened this issue Jun 8, 2023 · 4 comments · Fixed by #2205
Assignees
Labels
bug Something isn't working priority:p0 Issues which require immediate action to stop bleeding

Comments

@marcalff
Copy link
Member

marcalff commented Jun 8, 2023

Related to:

Context

Using opentelemetry-cpp 1.9.1
Using metrics, with asynchronous instruments.
Using callbacks located in a shared library.

Use case:

  • Create a Meter
  • Create an asynchounous instrument
  • add a callback to the instrument

Measurements are taken invoking the callback, and are exported.

Then:

  • remove the callback from the instrument
  • release the last instrument reference
  • release the last meter reference

From this point, no new measurements are called.

The OTLP HTTP exporter however, still exports data about the metrics no longer measured, as seen in the opentelemetry-collector logs in the endpoint.

In my understanding, this is due to the aggregations used:

  • sum aggregation of async counters continues, reporting a constant value
  • probably likewise for gauges and the last value (not tested)
  • etc

Please provide:

  • in the API/SDK, an operation to remove a synchronous / asynchronous instrument from a Meter,
  • in the API/SDK, an operation to remove a Meter from a MeterProvider.

so that exporters no longer report anything about defunct Meter and Metrics.

This is blocking: in practice, using Metrics with shared libraries is not possible currently with opentelemetry-cpp.

@marcalff marcalff added the bug Something isn't working label Jun 8, 2023
@lalitb
Copy link
Member

lalitb commented Jun 8, 2023

Also related - #2168. This is related to the aggregation, which probably doesn't get cleaned-up.
@marcalff - Thanks for raising the issue. If you don't have plan to work on it, should I assign it to myself. I think I will have some bandwidth during next week to fix it.

@marcalff
Copy link
Member Author

marcalff commented Jun 9, 2023

Thanks @lalitb .

I am working on Factory::Create() methods for metrics, to get me started on this part of the code base.

Assigning to you then. I can help with testing and review.

@marcalff marcalff changed the title [SDK] Remove Instruments and Meters [API/SDK] Remove Instruments and Meters Jun 12, 2023
@marcalff
Copy link
Member Author

Adding the blocking tag, as using opentelemetry-cpp for metrics is not possible without this fix, when instrumenting meters in shared libraries (that can disappear upon unload)

@marcalff marcalff added the issue:blocking This issue is preventing other fixes label Jun 14, 2023
@lalitb lalitb added this to the Metrics post GA release - 1 milestone Jun 19, 2023
marcalff added a commit to marcalff/opentelemetry-cpp that referenced this issue Jun 27, 2023
@marcalff marcalff mentioned this issue Jun 27, 2023
3 tasks
@marcalff marcalff changed the title [API/SDK] Remove Instruments and Meters [API] Remove Instruments and Meters Jul 3, 2023
@marcalff
Copy link
Member Author

marcalff commented Jul 3, 2023

After additional analysis, there is no real use case for removing instruments individually.

The entry point for an instrumented library is MeterProvider::GetMeter(), and an instrumented library will define its own meters.

As a result, removing a meter, which imply to remove all the associated instruments, at once, is sufficient.

@marcalff marcalff changed the title [API] Remove Instruments and Meters [API] Remove Meters Jul 3, 2023
@marcalff marcalff added priority:p0 Issues which require immediate action to stop bleeding and removed issue:blocking This issue is preventing other fixes labels Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p0 Issues which require immediate action to stop bleeding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants