Skip to content

[METRICS SDK] last collection of metrics before shutdown - #4550

Merged
marcalff merged 11 commits into
open-telemetry:mainfrom
nikhilbhatia08:fix/last_collect_before_shutdown
Sep 13, 2026
Merged

marcalff merged 11 commits into
open-telemetry:mainfrom
nikhilbhatia08:fix/last_collect_before_shutdown

Conversation

@nikhilbhatia08

@nikhilbhatia08 nikhilbhatia08 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #4542

Changes

This fixes the dropping of metrics since last periodic tick appears. The fix calls CollectAndExportOnce() one final time before shutdown of exporter.

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

@nikhilbhatia08
nikhilbhatia08 requested a review from a team as a code owner September 10, 2026 02:47
@codecov

codecov Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.50%. Comparing base (e0e0458) to head (235f8af).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4550      +/-   ##
==========================================
+ Coverage   86.45%   86.50%   +0.05%     
==========================================
  Files         525      525              
  Lines       20450    20465      +15     
==========================================
+ Hits        17679    17701      +22     
+ Misses       2771     2764       -7     
Files with missing lines Coverage Δ
...metrics/export/periodic_exporting_metric_reader.cc 83.60% <100.00%> (+2.56%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mateenali66

Copy link
Copy Markdown
Member

built it. the ordering works, OnShutDown joins the worker before exporter_->Shutdown() (periodic_exporting_metric_reader.cc:289-300), so the final cycle reaches a live exporter. two things come with it.

every clean shutdown now logs a warning. the final CollectAndExportOnce() runs only after the loop exits, which requires IsShutdown(), and MetricReader::Collect warns on exactly that (metric_reader.cc:31-35). it shows in this PR's own test:

[Warning] metric_reader.cc:35 MetricReader::Collect invoked while Shutdown in progress!
[       OK ] PeriodicExportingMetricReader.ShutdownPerformsFinalCollectAndExport (205 ms)

and Shutdown(timeout) now blocks for a whole export. against a MockPushMetricExporter sleeping 300ms in Export(), asking for 1ms:

main this PR
Shutdown(1ms) blocked 0 ms 304 ms

that is #4532 on this class. the join() at :297 is untimed, so an export inside the joined thread widens the gap between the requested timeout and the real one.

#4542 asks for the effects of ForceFlush, and OnForceFlush already does the timeout accounting, decrementing timeout_steady and passing the remainder to exporter_->ForceFlush (:236-272). one catch, its break_condition returns true as soon as IsShutdown() (:215), so a flush would have to run before shutdown_.store(true) in MetricReader::Shutdown.

CI green. comment, not an approval.

Comment thread sdk/src/metrics/export/periodic_exporting_metric_reader.cc Outdated
Comment thread sdk/src/metrics/export/periodic_exporting_metric_reader.cc Outdated
Comment thread sdk/src/metrics/metric_reader.cc Outdated
Comment thread sdk/src/metrics/export/periodic_exporting_metric_reader.cc Outdated
@lalitb

lalitb commented Sep 12, 2026

Copy link
Copy Markdown
Member

Thanks for fixing earlier issues. I have one blocking comment posted here which should be fixed.

Also, CI is crashing in the gRPC functional test after TLS certificate errors. Could we check the backtrace to see why? This looks separate from the metric reader change, but a failed connection should not crash the process. If transient, good to add a follow-up issue for that.

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

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.

@marcalff
marcalff merged commit 1143e55 into open-telemetry:main Sep 13, 2026
77 checks passed
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.

PeriodicExportingMetricReader::Shutdown should include effects of ForceFlush

4 participants