fix(sdk): force_flush returns meaningful bool on MetricReader - #5085
fix(sdk): force_flush returns meaningful bool on MetricReader#5085ravitheja4531-cell wants to merge 10 commits into
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? |
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!
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.
|
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 |
|
In the original code, detach(token) was called at the end of the method body but not inside a finally block. If an uncaught exception escaped the except Exception handler (e.g. a BaseException like KeyboardInterrupt), detach would never be called, leaking the context token. Moving it to finally ensures it always runs. Happy to remove the mention from the CHANGELOG if you'd prefer to keep the entry focused on the force_flush behavior. |
|
The handlng of All depends on maintainer preferences and how pedantic the library is about separating changes. Anyway, if it's left here, the merge commit message should be more generic/include both changes. |
MikeGoldsmith
left a comment
MikeGoldsmith
left a comment
There was a problem hiding this comment.
This now looks good to me, pending @herin049's feedback.
|
Hi @MikeGoldsmith @Asquator please approve so that the checks run |
|
I already approved many times. We need a maintainer to approve the workflows. |
Is I want to merge soon, please help me know if anything else needs to be done. |
|
I see the feature itself as done. Maybe there are some caveats with the .md files indeed, I don't know much about that. |
Can someone help to move faster and merge this PR please @IndiTheo @herin049 @MikeGoldsmith |
Hello Team, @MikeGoldsmith @herin049 how can we advance from here ? Need assistance to merge this asap. |
Hello @MikeGoldsmith @herin049: Any insights on this PR ? Requesting assistance to merge this sooner. |
|
@MikeGoldsmith @IndiTheo @emdneto @herin049: Please help me respond and lets move forward asap please. |
emdneto
left a comment
emdneto
left a comment
There was a problem hiding this comment.
Missing changelog. can you add?
|
Will be doing it shortly! |
|
Hello, can we merge this? |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
Pull request dashboard statusWaiting on the author · refreshed 2026-08-25 21:33 UTC Two things need attention:
Status above doesn't look right?
|
Hi thanks for this. Please could you resolve the current merge conflicts |
|
Hi @ravitheja4531-cell — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
|
Merge conflict in export/init.py is now resolved and the branch is up to date with main. This is ready for another look. Could a maintainer approve the workflow run so CI can execute? Also, there's still one "changes requested" review on record — @herin049 , if your earlier feedback is addressed, would you mind re-reviewing to clear it? Thanks all. |
|
@IndiTheo @MikeGoldsmith @emdneto @lzchen can you please run the checks |
8 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: