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 and Export metric data before PeriodicMetricReader shutdown. #1860

Merged
merged 10 commits into from
Jan 4, 2023

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Dec 14, 2022

Fixes #1858

Changes

As discussed in this issue, PeriodicMetricReader should ensure the metric collection and export is done once before shutdown, for all the data points be uploaded. This is also supported in Java , and discussed in specs - open-telemetry/opentelemetry-specification#2983

The shutdown sequence is:
MeterProvider::Shutdown() -> MeterContext::Shutdown() -> MetricCollector() -> Shutdown() -> MetricReader::Shutdown() -> PeriodicMetricReader::OnShutdown()
-> [[[Perform Collect and Shutdown ]]] -> MetricExporter::Shutdown()

This ensures that the MericExporter instance remains valid while MetricReader is doing collect-and-export during it's shutdown. Once the collect-and-export is completed, the MetricExporter is shutdown.

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

@lalitb lalitb requested a review from a team December 14, 2022 00:09
@lalitb lalitb changed the title Metric reader shutdown Collect and Exporte metric data before PeriodicMetricReader shutdown. Dec 14, 2022
@lalitb lalitb changed the title Collect and Exporte metric data before PeriodicMetricReader shutdown. Collect and Export metric data before PeriodicMetricReader shutdown. Dec 14, 2022
@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #1860 (ae8e0e4) into main (61bc860) will increase coverage by 0.01%.
The diff coverage is 71.43%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
+ Coverage   85.78%   85.78%   +0.01%     
==========================================
  Files         171      171              
  Lines        5243     5252       +9     
==========================================
+ Hits         4497     4505       +8     
- Misses        746      747       +1     
Impacted Files Coverage Δ
sdk/src/metrics/metric_reader.cc 75.76% <50.00%> (+0.76%) ⬆️
...metrics/export/periodic_exporting_metric_reader.cc 74.08% <73.08%> (+0.17%) ⬆️
sdk/src/trace/batch_span_processor.cc 91.48% <0.00%> (+0.78%) ⬆️

@lalitb lalitb requested a review from esigo January 3, 2023 18:59
@esigo esigo self-assigned this Jan 4, 2023
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 for the PR :)

@lalitb lalitb enabled auto-merge (squash) January 4, 2023 21:03
@lalitb lalitb merged commit d557191 into open-telemetry:main Jan 4, 2023
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.

Collect in PeriodicExportingMetricReader ForceFlush before exporting
2 participants