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

Collect in PeriodicExportingMetricReader ForceFlush before exporting #1858

Closed
johanpel opened this issue Dec 12, 2022 · 4 comments · Fixed by #1860
Closed

Collect in PeriodicExportingMetricReader ForceFlush before exporting #1858

johanpel opened this issue Dec 12, 2022 · 4 comments · Fixed by #1860
Assignees

Comments

@johanpel
Copy link
Contributor

johanpel commented Dec 12, 2022

Is your feature request related to a problem?

Consider the scenario where some executable emitting metrics through the PeriodicExportingMetricReader is about to exit, but desires to collect and export the last value of an ObservableGauge one more time without waiting for the periodic collect and export.

Describe the solution you'd like

When using the PeriodicExportingMetricReader, it would be nice if ForceFlush() collects before exporting.

Describe alternatives you've considered
I have a metric that could be a Counter, but it gets updated very often and emits too many metric points. Therefore I really want to just collect and export it periodically using an ObservableGauge. I could work around it by using a Counter and periodically updating that myself, forcing one final update and flush upon exit. However, it seems the behavior I desire is common among other language SDKs as well, so I think it may be desired by others for ForceFlush to follow the behavior seen elsewhere.

Additional context

I hope I am not missing anything, but if I understand correctly, in the current implementation, an export is performed but no metrics are collected before exporting.
https://github.com/open-telemetry/opentelemetry-cpp/blob/main/sdk/src/metrics/export/periodic_exporting_metric_reader.cc#L85-L88

Other language SDKs appear to collect before exporting on ForceFlush:

Java: https://javadoc.io/static/io.opentelemetry/opentelemetry-sdk-metrics/1.21.0/io/opentelemetry/sdk/metrics/export/PeriodicMetricReader.html#forceFlush()
Python (see force_flush at the bottom, the superclass force_flush collects first): https://opentelemetry-python.readthedocs.io/en/latest/_modules/opentelemetry/sdk/metrics/_internal/export.html#PeriodicExportingMetricReader.force_flush

@lalitb lalitb self-assigned this Dec 12, 2022
@lalitb lalitb added this to the Metrics post GA release milestone Dec 12, 2022
@lalitb
Copy link
Member

lalitb commented Dec 12, 2022

I hope I am not missing anything, but if I understand correctly, in the current implementation, an export is performed but no metrics are collected before exporting.

Yes, this is correct. Thanks for raising the issue.

Related issues on specs:
open-telemetry/opentelemetry-specification#2924
open-telemetry/opentelemetry-specification#2983

@lalitb
Copy link
Member

lalitb commented Dec 13, 2022

The MetricReader::ForceFlush() method needs more clarity from specification prospective. As of now, python and java does collection while JS and C++ doesn't.
Regarding the scenario described in this issue, it would make more sense for PeriodicExportingMetricReader::Shutdown() to do one last collection to capture all metrics before exiting ?

@johanpel
Copy link
Contributor Author

The MetricReader::ForceFlush() method needs more clarity from specification prospective

Agreed, thanks for linking the related issues.

PeriodicExportingMetricReader::Shutdown() to do one last collection to capture all metrics before exiting

Yes, that would cover the scenario perfectly.

@sfc-gh-sili
Copy link

sfc-gh-sili commented Jan 28, 2024

@lalitb @johanpel Hello! Is there any reason PeriodicExportingMetricReader don't collect another round of data on Shutdown anymore? ref:

bool PeriodicExportingMetricReader::CollectAndExportOnce()

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 a pull request may close this issue.

3 participants