[SDK] Optimize PeriodicExportingMetricReader Thread Usage #3383
Conversation
|
|
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
ColeVanOphem
changed the title
| } while (IsShutdown() != true); | ||
| if(IsShutdown()) | ||
| { | ||
| return; |
There was a problem hiding this comment.
Don't we need call worker_thread_instrumentation_->OnEnd() below?
There was a problem hiding this comment.
+1, probably we can just break from here instead of return.
Replaced early returns with breaks in PeriodicExportingMetricReader::DoBackgroundWork to fix worker_thread_instrumentation_ behavior.
marcalff
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3383 +/- ##
==========================================
+ Coverage 89.91% 89.98% +0.08%
==========================================
Files 211 211
Lines 6832 6812 -20
==========================================
- Hits 6142 6129 -13
+ Misses 690 683 -7
🚀 New features to boost your workflow:
|
Fixed formatting so that it aligns with the clangformat Removed unnecessary future includes to address errors raised by include-what-you-use
| @@ -151,8 +141,11 @@ void PeriodicExportingMetricReader::DoBackgroundWork() | |||
| worker_thread_instrumentation_->AfterWait(); | |||
| } | |||
| #endif /* ENABLE_THREAD_INSTRUMENTATION_PREVIEW */ | |||
|
|
|||
| } while (IsShutdown() != true); | |||
| if (IsShutdown()) | |||
There was a problem hiding this comment.
With the return changed back to break, is it the same as the original do...while loop? If so, just restore it to the do...while loop which seems more concise?
Improved scoping of the unique lock that is used in PeriodicExportingMetricReader::DoBackgroundWork Changed while loop logic from while(true)to be similar to original code (do-while)
There was a problem hiding this comment.
Thanks for the contribution @ColeVanOphem @pr1nceray. The change looks good. Please add a changelog entry and consider adding test cases/checks for the PeriodicExportingMetricReader if missing or impacted by this PR.
| << this->export_timeout_millis_.count() << " ms, and timed out"); | ||
| return false; | ||
| } | ||
| this->exporter_->Export(metric_data); |
There was a problem hiding this comment.
Note to self - the timeout interval export_timeout_millis_ is checked at best effort basis between collect and export. This means, if the collect and/or export takes indefinite time, we can be in blocked state. This is specifically relevant to export method, as its execution time is not in the control of the SDK. The export operation runs to completion once started, regardless of how long it takes.
Nothing for this PR, as this is the existing behavior, but something to discuss further.
|
@ColeVanOphem please let us know if could add an entry to CHANGELOG as @dbarker mentioned, then the PR is ready for merging. |
[SDK] Optimize PeriodicExportingMetricReader Thread Usage (open-telemetry#3383)
Fixes #3230
Changes
Optimizes thread usage in
PeriodicExportingMetricReaderby:CollectAndExportOnce. Collection and export now run synchronously in the worker thread.std::future-based mechanism.std::condition_variable.For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes