Add PeriodicReaderWithOwnThread #2292
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2292 +/- ##
=======================================
+ Coverage 79.4% 79.6% +0.2%
=======================================
Files 122 123 +1
Lines 20783 21263 +480
=======================================
+ Hits 16504 16940 +436
- Misses 4279 4323 +44 ☔ View full report in Codecov by Sentry. |
opentelemetry-sdk/src/metrics/periodic_reader_with_own_thread.rs
Outdated
Show resolved
Hide resolved
lalitb
left a comment
lalitb
left a comment
There was a problem hiding this comment.
LGTM with nit comments. As this is under feature flag, it can be improved further after release. Thanks for working on it.
| /// attempts. PeriodicReaderWithOwnThread itself does not enforce timeout. | ||
| /// Instead timeout is passed on to the exporter for each export attempt. | ||
| /// | ||
| /// The [collect] method of the returned continues to gather and |
There was a problem hiding this comment.
this looks incomplete. Probably:
| /// The [collect] method of the returned continues to gather and | |
| /// The [collect] method continues to gather and |
| // Alternatively, we could skip the next export | ||
| // and wait for the next interval. | ||
| // Or enforce that export timeout is less than interval. | ||
| // What is the desired behavior? |
There was a problem hiding this comment.
Perhaps hybrid approach can be one option - enforce that the export timeout is configured to be less than the interval. And then, if the delay is slightly more than the interval, we can do next export immediately. However, if the delay is in multiple of intervals, it signifies heavy load on system, and better to skip next cycle - Just a thought, but good to discuss for further PRs.
| // Not a fan of such tests, but this seems to be the only way to test | ||
| // if periodic reader is doing its job. | ||
| // TODO: Decide if this should be ignored in CI | ||
| std::thread::sleep(interval * 5 * 20); |
There was a problem hiding this comment.
yes sleep will slow down the CI. Instead of sleep, we can have a loop that periodically checks this condition as below (not syntactically correct):
while i.load(Ordering::Relaxed) < 5 && elapsed_time < 5*20 ms)
{
}
Or better to skip this test for the CI. Not a blocker for this PR.
| async fn collection_from_tokio_current() { | ||
| collection_triggered_by_interval_helper(); | ||
| collection_triggered_by_flush_helper(); | ||
| collection_triggered_by_shutdown_helper(); |
There was a problem hiding this comment.
nit - These 3 helper methods can be wrapped in a single method and called in all test cases to reduce redundancy
| // Sleep for a duration longer than the interval to ensure at least one collection | ||
| // Not a fan of such tests, but this seems to be the only way to test | ||
| // if periodic reader is doing its job. | ||
| // TODO: Decide if this should be ignored in CI |
There was a problem hiding this comment.
Either skip in CI, or conditional polling. Not a blocker here though - for separate PR.
|
Merging to unblock release. This component is under experimental ff, and no impact on any existing code. |
3 participants
Originally part of https://github.com/open-telemetry/opentelemetry-rust/pull/2142/files, but this one leaves existing PeriodicReader as-is, and instead adds a new one
PeriodicReaderWithOwnThread. While implementing 2142, we discovered several issues related to async runtimes + exporters requiring async runtimes(OTLPExporter) that will take some time to resolve. Hence, this is a separate reader, currently useable with stdoutexporter, etw or user-events exporter, as these exporters don't rely on any particular runtimes. Once the stability is verified, will modify this to allow usage with OTLP Exporters and eventually get merged with PeriodicReader itself.