search

open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
851 stars 403 forks source link

[SDK] Fix forceflush may wait for ever #2584

Closed owent closed 4 months ago

owent commented 6 months ago

Fixes #2574

May also fixes #2583

Changes

For significant contributions please make sure you have completed the following items:

owent commented 5 months ago

@ThomsonTan Is there any problem in this PR? Maybe we can close #2553 if this PR is merged. It fixes the same problems as #2553 do and also fix the simular problems in trace and logs.

ThomsonTan commented 4 months ago

BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.

owent commented 4 months ago

BTW, can you explain in brief that why the sequence notification can break the potential forever wait? Thanks.

When more than one threads call ForceFlush concurrency, is_force_flush_notified may be set true only once in background thread and one of the threads call ForceFlush will wait is_force_flush_notified for ever.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 87.70%. Comparing base (497eaf4) to head (d3df9a4). Report is 65 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584/graphs/tree.svg?width=650&height=150&src=pr&token=FJESTYQ2AD&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ Coverage Diff @@ ## main #2584 +/- ## ========================================== + Coverage 87.12% 87.70% +0.58% ========================================== Files 200 190 -10 Lines 6109 5852 -257 ========================================== - Hits 5322 5132 -190 + Misses 787 720 -67 ``` | [Files](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [...pentelemetry/sdk/logs/batch\_log\_record\_processor.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&filepath=sdk%2Finclude%2Fopentelemetry%2Fsdk%2Flogs%2Fbatch_log_record_processor.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9zZGsvbG9ncy9iYXRjaF9sb2dfcmVjb3JkX3Byb2Nlc3Nvci5o) | `100.00% <100.00%> (ø)` | | | [...ude/opentelemetry/sdk/trace/batch\_span\_processor.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&filepath=sdk%2Finclude%2Fopentelemetry%2Fsdk%2Ftrace%2Fbatch_span_processor.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9zZGsvdHJhY2UvYmF0Y2hfc3Bhbl9wcm9jZXNzb3IuaA==) | `100.00% <100.00%> (ø)` | | | [sdk/src/logs/batch\_log\_record\_processor.cc](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&filepath=sdk%2Fsrc%2Flogs%2Fbatch_log_record_processor.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL3NyYy9sb2dzL2JhdGNoX2xvZ19yZWNvcmRfcHJvY2Vzc29yLmNj) | `89.21% <96.16%> (+1.71%)` | :arrow_up: | | [...metrics/export/periodic\_exporting\_metric\_reader.cc](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&filepath=sdk%2Fsrc%2Fmetrics%2Fexport%2Fperiodic_exporting_metric_reader.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL3NyYy9tZXRyaWNzL2V4cG9ydC9wZXJpb2RpY19leHBvcnRpbmdfbWV0cmljX3JlYWRlci5jYw==) | `81.32% <95.00%> (+1.98%)` | :arrow_up: | | [sdk/src/trace/batch\_span\_processor.cc](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584?src=pr&el=tree&filepath=sdk%2Fsrc%2Ftrace%2Fbatch_span_processor.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL3NyYy90cmFjZS9iYXRjaF9zcGFuX3Byb2Nlc3Nvci5jYw==) | `94.08% <96.16%> (+1.65%)` | :arrow_up: | ... and [57 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2584/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
marcalff commented 4 months ago

Adding ok-to-merge.

Waiting to give @lalitb and @esigo a chance to comment, and planning to merge early next week.