fix(sdk): force_flush returns meaningful bool on MetricReader#5085
fix(sdk): force_flush returns meaningful bool on MetricReader#5085ravitheja4531-cell wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
|
could someone from @jd take a look when you have a moment? |
|
I personally like this pretty much. After it's merged, we'll have to open a new issue to indicate failure reason. Will look at the code with more attention soon. Btw, are you planning to add unit tests for the new behavior? |
| **kwargs, | ||
| ) -> None: | ||
| """Called by `MetricReader.collect` when it receives a batch of metrics""" | ||
| ) -> bool: |
There was a problem hiding this comment.
| ) -> bool: | |
| ) -> bool | None: |
|
|
||
| @final | ||
| def collect(self, timeout_millis: float = 10_000) -> None: | ||
| def collect(self, timeout_millis: float = 10_000) -> Optional[bool]: |
There was a problem hiding this comment.
| def collect(self, timeout_millis: float = 10_000) -> Optional[bool]: | |
| def collect(self, timeout_millis: float = 10_000) -> bool | None: |
github-project-automation
bot
moved this to Reviewed PRs that need fixes
in Python PR digest
| collect_ok = super().force_flush(timeout_millis=timeout_millis) | ||
| exporter_ok = self._exporter.force_flush(timeout_millis=timeout_millis) | ||
| return collect_ok and exporter_ok |
There was a problem hiding this comment.
- Should we test this logic?
- Should we even call the exporter's
force_flushif the general one fails?
There was a problem hiding this comment.
Addressed all review feedback:
Changed Optional[bool] → bool | None on collect and _receive_metrics (per @herin049)
Removed Optional from imports entirely
Fixed force_flush short-circuit: exporter.force_flush is now skipped if super().force_flush fails (per @Asquator)
Added 4 unit tests covering success, failure, short-circuit, and detach always running in finally
Ready for re-review. Thanks!
Fixes open-telemetry#5020 Signed-off-by: Ravi Theja <ravitheja4531@gmail.com>
ravitheja4531-cell
force-pushed
the
fix/force-flush-return-meaningful-bool
branch
from
60da606 to
5680779
Compare
3 participants
Fixes #5020
Description
force_flushonMetricReaderandPeriodicExportingMetricReaderalwaysreturned
Trueregardless of whether the export succeeded or failed, violatingthe OTel specification:
This PR threads the actual export result up through
_receive_metrics→collect→force_flushso callers get a meaningful bool.Also fixes a pre-existing bug where
detach(token)inPeriodicExportingMetricReader._receive_metricswas only called on the happypath — moved to
finallyso it always runs.Type of change
How Has This Been Tested?
Ran the full existing metrics test suite locally: