-
Notifications
You must be signed in to change notification settings - Fork 598
fix: handle shutdown in logs exporter #3255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle shutdown in logs exporter #3255
Conversation
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3255 +/- ##
=======================================
- Coverage 80.8% 80.7% -0.1%
=======================================
Files 129 129
Lines 23203 23212 +9
=======================================
Hits 18750 18750
- Misses 4453 4462 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scottgerring
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
| } | ||
| None => Err(tonic::Status::failed_precondition( | ||
| "exporter is already shut down", | ||
| "metrics exporter is already shut down", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is helpful; I don't think the user would necessarily see which failed otherwise
Co-authored-by: Scott Gerring <[email protected]>
|
@stefanobaghino that was a quick turnaround :D |
|
I'd be happy to take any additional feedback on this. If this is not a satisfactory approach, I also have nothing against closing this. Please let me know how to progress. Thanks. |
Hey @stefanobaghino should be fine; let's just wait on @cijothomas 's feedback! He ultimately holds the merge approval. |
cijothomas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
Fixes #2777
Changes
This follows the approach suggested here by @scottgerring:
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes