-
Notifications
You must be signed in to change notification settings - Fork 508
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
feat: Add shutdown with timeout for metric exporter #2854
base: main
Are you sure you want to change the base?
feat: Add shutdown with timeout for metric exporter #2854
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2854 +/- ##
=======================================
- Coverage 81.1% 81.0% -0.1%
=======================================
Files 124 125 +1
Lines 23927 23945 +18
=======================================
- Hits 19410 19409 -1
- Misses 4517 4536 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -163,6 +164,10 @@ impl PushMetricExporter for MetricExporter { | |||
} | |||
|
|||
fn shutdown(&self) -> OTelSdkResult { | |||
self.shutdown_with_timeout(Duration::from_secs(5)) |
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.
Is it from spec? If so please link the document here nvm I think we have it already, maybe worth following up to see if the value is reasonable
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.
The 5 second is used in other functions as default timeout. like:
forceflush_timeout: Duration::from_secs(5), // TODO: make this configurable |
I don't know why we choose 5 seconds. As it is written in ToDo, I think we need to read it from configuration.
I think this is not related to issue of the PR. we can work on it at another PR.
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.
There is no spec on what should be the default, so 5 seconds is our invention. We should recommend users to use the shutdown_with_timeout passing a Duration of their choice, but we also do shutdown() from Drop, where we need to pick some default. 5 secs seems reasonable to me as a default.
@@ -26,6 +28,9 @@ pub trait PushMetricExporter: Send + Sync + 'static { | |||
/// | |||
/// After Shutdown is called, calls to Export will perform no operation and | |||
/// instead will return an error indicating the shutdown state. | |||
fn shutdown_with_timeout(&self, timeout: Duration) -> OTelSdkResult; |
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.
now that we have both, lets give a default implementation for the shutdown(), invoking the new one passing 5 seconds.
Also please update changelog to reflect this - this is breaking change for custom MetricExporter author.
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.
Done
#2574 Part 1: Adding shutdown_with_timeout for metric exporter