-
Notifications
You must be signed in to change notification settings - Fork 590
feat: add forceflush_timeout configurability #3223
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
base: main
Are you sure you want to change the base?
feat: add forceflush_timeout configurability #3223
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3223 +/- ##
=======================================
- Coverage 80.8% 80.6% -0.2%
=======================================
Files 128 128
Lines 23288 23204 -84
=======================================
- Hits 18821 18725 -96
- Misses 4467 4479 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// Corresponding environment variable: `OTEL_BLRP_FORCE_FLUSH_TIMEOUT`. | ||
| /// | ||
| /// Note: Programmatically setting this will override any value set via the environment variable. | ||
| pub fn with_forceflush_timeout(mut self, forceflush_timeout: Duration) -> Self { |
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.
for shutdown, we allow users to pass timeout in each shutdown call itself. Could we stick with the same for flush also?
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.
Its best to be consistent with shutdown, if at all we are adding timeout to flush.
i.e Add this all the way from provider itself.
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.
You're right, I'll refactor the code to mirror the propagation of the provider's timeout like we do for shutdown_with_timeout.
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.
See https://github.com/open-telemetry/opentelemetry-rust/pull/3223/files#r2495071383
Want to see if there is a reason for this be done differently than Shutdown timeouts.
Changes
Adds configurability of force flushing timeout for batch processors (BatchSpanProcessor, BatchLogProcessor).
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes