[hidingmyname]

Fixes #4746

Changes

PeriodicExportingMetricReader::Shutdown(timeout) returned only after an in-flight collection had run every remaining meter and observable callback, and the result of that collection could still be exported after the shutdown budget was spent. This follows the direction discussed in #4746:

• MetricCollector::Produce() checks between meters, and ObservableRegistry::Observe() checks between callbacks, whether the reader running the collection has stopped waiting for it. If so, they stop and the collector returns kTimeout with whatever it had already collected.
• PeriodicExportingMetricReader::CollectAndExportOnce() does not call Export() once is_stop_requested_ is set, for any producer status. Before, a partial kTimeout result was dropped only when the collection took longer than export_timeout_millis_ (30 s by default).

The reader passes its is_stop_requested_ flag to the collection code through a small internal helper, sdk/src/metrics/collection_cancellation.{h,cc}. It is a thread-local scope set by the reader's worker thread for the duration of CollectAndExportOnce(). It lives under sdk/src, is not installed, and adds no public API or ABI change. Collections started any other way, such as MetricReader::Collect() from a pull exporter, are unaffected because no scope is active on their thread. For Bazel, sdk/src/metrics/BUILD now lists the headers in that directory as hdrs.

Limits:

• A single callback that is already running still runs to completion; the check runs between callbacks and between meters.
• The untimed join() around Export() mentioned in [BUG] BatchSpanProcessor::Shutdown ignores its timeout when the worker thread is blocked in Export() #4532 is not changed.

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

Tests

Two new tests in sdk/test/metrics/periodic_exporting_metric_reader_test.cc, both using the reader's default options (30 s export timeout):

• ShutdownStopsCollectionBetweenObservableCallbacks: a MeterProvider with four 500 ms observable gauge callbacks across two meters, and Shutdown(100ms) called while the first callback runs. The test checks that only one callback runs, that shutdown returns well before the remaining callbacks would finish, and that nothing is exported.
• ShutdownSkipsExportOfTimedOutCollection: the case from the review. A producer returns kTimeout with one metric after the shutdown budget is spent, and the exporter takes 2 s. The test checks that Export() is not called and that shutdown does not wait for the exporter.

Without the change in sdk/src, both tests fail: in the first, all four callbacks run (about 2000 ms); in the second, the partial result is exported and Shutdown waits for the exporter. With the change, both pass, and pass 20 out of 20 times with --gtest_repeat=20.

ctest --test-dir sdk/test/metrics: 100% of 266 tests pass.

Built with CMake 4.4.5, GCC 15.3.0, Debug, Linux x86_64. Formatted with clang-format 18 using the repository's .clang-format. Not run with Bazel or on Windows/macOS.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.65%. Comparing base (0a5bb0b) to head (cf94acb).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4758      +/-   ##
==========================================
+ Coverage   86.62%   86.65%   +0.04%     
==========================================
  Files         530      531       +1     
  Lines       21169    21190      +21     
==========================================
+ Hits        18335    18360      +25     
+ Misses       2834     2830       -4     

Files with missing lines	Coverage Δ
sdk/src/metrics/collection_cancellation.cc	100.00% <100.00%> (ø)
...metrics/export/periodic_exporting_metric_reader.cc	85.72% <100.00%> (+2.13%)	⬆️
sdk/src/metrics/state/metric_collector.cc	96.97% <100.00%> (+0.20%)	⬆️
sdk/src/metrics/state/observable_registry.cc	97.68% <100.00%> (+0.06%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[mateenali66]

Thanks for the fix.

LGTM, but CI fails on the new test, see comments about clang-tidy and iwyu.

Both new tests fail on main and pass at head 20 times out of 20, and the reader tests are clean under TSan.

See also an optional comment on test coverage for the meter check.

Will approve once CI is green.

[mateenali66]

The three iwyu jobs fail on this file, and they allow no warnings:

periodic_exporting_metric_reader_test.cc should add these lines:
#include <initializer_list>
#include <ratio>

periodic_exporting_metric_reader_test.cc should remove these lines:
- #include <memory>  // lines 10-10

Please add both and remove <memory>, the test still builds and passes that way.

[mateenali66]

Suggested change

	std::chrono::milliseconds duration;
	std::chrono::milliseconds duration{0};

Fixes one of the two new clang-tidy warnings that put both clang-tidy jobs over their limit.

[mateenali66]

Suggested change

	void SlowObservableCallback(opentelemetry::metrics::ObserverResult /* result */, void *state)
	// NOLINTNEXTLINE(performance-unnecessary-value-param)
	void SlowObservableCallback(opentelemetry::metrics::ObserverResult /* result */, void *state)

The callback type requires the by-value ObserverResult, same as async_instruments_test.cc:24.

[mateenali66]

Removing this check leaves both new tests green, because Observe() already skips the callbacks of meter_b.

A MetricCollectorTest that records one counter value, calls Produce() inside a CollectionCancellation::Scope with the flag set, and expects kTimeout and an empty scope_metric_data_, passes at head and fails without this check.

(optional)

[hidingmyname]

Thanks for the review. Pushed the changes you listed: the iwyu include changes, duration{0} and the NOLINTNEXTLINE.

I also added the optional test as MetricCollectorTest.ProduceStopsBeforeNextMeterWhenCancelled. It fails with the meter check removed and passes with it. ctest --test-dir sdk/test/metrics passes 267 of 267 locally (GCC 15.3, Debug). I did not run iwyu or clang-tidy locally, so CI will confirm those.