opentelemetry-cpp icon indicating copy to clipboard operation
opentelemetry-cpp copied to clipboard

[SDK] Fix forceflush may wait for ever

Open owent opened this issue 1 year ago • 5 comments

Fixes #2574

May also fixes #2583

Changes

  • Using a sequence to check if a complete notification includes the datas when call ForceFlush.

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

  • [ ] CHANGELOG.md updated for non-trivial changes
  • [x] Unit tests have been added
  • [x] Changes in public API reviewed

owent avatar Mar 10 '24 09:03 owent

@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.

owent avatar Apr 30 '24 10:04 owent

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

ThomsonTan avatar May 07 '24 22:05 ThomsonTan

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.

owent avatar May 08 '24 07:05 owent

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

@@            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 Coverage Δ
...pentelemetry/sdk/logs/batch_log_record_processor.h 100.00% <100.00%> (ø)
...ude/opentelemetry/sdk/trace/batch_span_processor.h 100.00% <100.00%> (ø)
sdk/src/logs/batch_log_record_processor.cc 89.21% <96.16%> (+1.71%) :arrow_up:
...metrics/export/periodic_exporting_metric_reader.cc 81.32% <95.00%> (+1.98%) :arrow_up:
sdk/src/trace/batch_span_processor.cc 94.08% <96.16%> (+1.65%) :arrow_up:

... and 57 files with indirect coverage changes

codecov[bot] avatar May 08 '24 07:05 codecov[bot]

Adding ok-to-merge.

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

marcalff avatar May 23 '24 19:05 marcalff