open-telemetry / opentelemetry-cpp

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

[EXPORTER] OTLP file: use thread-safe file/io #2675

Closed owent closed 3 weeks ago

owent commented 1 month ago

Fixes #2674

Changes

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

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.67%. Comparing base (497eaf4) to head (93ec062). Report is 77 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2675/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/2675?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 #2675 +/- ## ========================================== + Coverage 87.12% 87.67% +0.55% ========================================== Files 200 190 -10 Lines 6109 5851 -258 ========================================== - Hits 5322 5129 -193 + Misses 787 722 -65 ``` | [Files](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2675?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [api/include/opentelemetry/nostd/string\_view.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2675?src=pr&el=tree&filepath=api%2Finclude%2Fopentelemetry%2Fnostd%2Fstring_view.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-YXBpL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9ub3N0ZC9zdHJpbmdfdmlldy5o) | `97.96% <100.00%> (ø)` | | ... and [75 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2675/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
owent commented 1 month ago
  • file iostream operations are not thread safe when multiple threads uses the same stream

file iostream operations are not thread safe when multiple threads uses the same stream(we got data race from std::basic_streambuf<CharT,Traits>::xsputn when using tsan). But fwrite and fflush are thread safe when multiple threads uses the same FILE handle.

You are right, fwrite(data.data(), 1, data.size(), out.get()); and fputc('\n', out.get()); still have concurrency problems here. I have move the codes to append EOL in the caller.

marcalff commented 3 weeks ago

@esigo Hi Ehsan. Any comments for the code review ?