fix(sdk): force_flush returns meaningful bool on MetricReader#5085
fix(sdk): force_flush returns meaningful bool on MetricReader#5085ravitheja4531-cell wants to merge 6 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
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks @ravitheja4531-cell.
I've left a couple of small tweaks.
| ) -> bool | None: | ||
| """Called by `MetricReader.collect` when it receives a batch of metrics. | ||
|
|
||
| Subclasses must return ``True`` on success and ``False`` on failure. |
There was a problem hiding this comment.
As there are still some subclasses that may return None, let's soften the requirement.
| Subclasses must return ``True`` on success and ``False`` on failure. | |
| Subclasses should return ``True`` on success and ``False`` on failure. |
| ([#5093](https://github.com/open-telemetry/opentelemetry-python/pull/5093)) | ||
| - `opentelemetry-sdk`: fix YAML structure injection via environment variable substitution in declarative file configuration; values containing newlines are now emitted as quoted YAML scalars per spec requirement | ||
| ([#5091](https://github.com/open-telemetry/opentelemetry-python/pull/5091)) | ||
| - `opentelemetry-sdk`: Fix `force_flush` on `MetricReader` and `PeriodicExportingMetricReader` to return a meaningful `bool` reflecting actual export success/failure instead of always returning `True`. Also fixes `detach(token)` not being called when export raises an exception. ([#5020](https://github.com/open-telemetry/opentelemetry-python/issues/5020)) |
There was a problem hiding this comment.
We use PR numbers in changelog entries, not issue numbers. The issue is link to in the PR description.
| - `opentelemetry-sdk`: Fix `force_flush` on `MetricReader` and `PeriodicExportingMetricReader` to return a meaningful `bool` reflecting actual export success/failure instead of always returning `True`. Also fixes `detach(token)` not being called when export raises an exception. ([#5020](https://github.com/open-telemetry/opentelemetry-python/issues/5020)) | |
| - `opentelemetry-sdk`: Fix `force_flush` on `MetricReader` and `PeriodicExportingMetricReader` to return a meaningful `bool` reflecting actual export success/failure instead of always returning `True`. Also fixes `detach(token)` not being called when export raises an exception. ([#5085](https://github.com/open-telemetry/opentelemetry-python/pull/5085)) |
|
Noted changed "must" to "should" in the docstring and updated the CHANGELOG to use the PR number. Thanks! |
|
can you guys please approve it @herin049 @Asquator @MikeGoldsmith |
| ([#5093](https://github.com/open-telemetry/opentelemetry-python/pull/5093)) | ||
| - `opentelemetry-sdk`: fix YAML structure injection via environment variable substitution in declarative file configuration; values containing newlines are now emitted as quoted YAML scalars per spec requirement | ||
| ([#5091](https://github.com/open-telemetry/opentelemetry-python/pull/5091)) | ||
| - `opentelemetry-sdk`: Fix `force_flush` on `MetricReader` and `PeriodicExportingMetricReader` to return a meaningful `bool` reflecting actual export success/failure instead of always returning `True`. Also fixes `detach(token)` not being called when export raises an exception. ([#5085](https://github.com/open-telemetry/opentelemetry-python/pull/5085)) |
There was a problem hiding this comment.
Also fixes
detach(token)not being called when export raises an exception.
This was never an issue to begin with, why is this here?
4 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: