Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make use of the sidecar thread safe #2671

Closed
wants to merge 1 commit into from
Closed

Conversation

iamluc
Copy link
Contributor

@iamluc iamluc commented May 22, 2024

Description

The recreation of the sidecar transport in case of error (closed socket, sidecar crash, ...) was not thread safe.
This PR should fix that.

NOTE: because this PR needs a change in libdatadog (DataDog/libdatadog#440), the generated header files also contain some changes of #2639

Reviewer checklist

  • Test coverage seems ok.
  • Appropriate labels assigned.

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

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

Project coverage is 66.77%. Comparing base (1aaebd8) to head (10b9acf).
Report is 3 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2671       +/-   ##
=============================================
- Coverage     77.72%   66.77%   -10.95%     
  Complexity     2212     2212               
=============================================
  Files           225      199       -26     
  Lines         26109    21953     -4156     
  Branches        988        0      -988     
=============================================
- Hits          20292    14659     -5633     
- Misses         5291     7294     +2003     
+ Partials        526        0      -526     
Flag Coverage Δ
appsec-extension ?
tracer-extension 78.54% <83.87%> (-0.02%) ⬇️
tracer-php 50.44% <ø> (-29.86%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ext/sidecar.c 90.32% <83.87%> (+7.56%) ⬆️

... and 59 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1aaebd8...10b9acf. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented May 22, 2024

Benchmarks

Benchmark execution time: 2024-05-22 10:23:05

Comparing candidate commit 10b9acf in PR branch luc/sidecar-thread-safety with baseline commit 1aaebd8 in branch master.

Found 14 performance improvements and 2 performance regressions! Performance is the same for 162 metrics, 0 unstable metrics.

scenario:ComposerTelemetryBench/benchTelemetryParsing

  • 🟥 execution_time [+2.503µs; +4.497µs] or [+4.422%; +7.946%]

scenario:EmptyFileBench/benchEmptyFileBaseline

  • 🟩 execution_time [-369.502µs; -232.558µs] or [-13.339%; -8.395%]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

  • 🟩 execution_time [-435.457µs; -298.943µs] or [-15.016%; -10.308%]

scenario:EmptyFileBench/benchEmptyFileOverhead

  • 🟩 execution_time [-532.619µs; -358.301µs] or [-14.805%; -9.959%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

  • 🟩 execution_time [-627.622µs; -492.798µs] or [-16.844%; -13.226%]

scenario:HookBench/benchHookOverheadInstallHookOnFunction

  • 🟥 execution_time [+20.089µs; +37.867µs] or [+2.475%; +4.666%]

scenario:LaravelBench/benchLaravelBaseline

  • 🟩 execution_time [-354.663µs; -200.017µs] or [-12.213%; -6.888%]

scenario:LaravelBench/benchLaravelBaseline-opcache

  • 🟩 execution_time [-483.706µs; -335.034µs] or [-15.836%; -10.969%]

scenario:LaravelBench/benchLaravelOverhead

  • 🟩 execution_time [-559.448µs; -375.372µs] or [-14.769%; -9.909%]

scenario:LaravelBench/benchLaravelOverhead-opcache

  • 🟩 execution_time [-591.415µs; -416.825µs] or [-15.261%; -10.756%]

scenario:SymfonyBench/benchSymfonyBaseline

  • 🟩 execution_time [-515.681µs; -444.659µs] or [-8.115%; -6.997%]

scenario:SymfonyBench/benchSymfonyBaseline-opcache

  • 🟩 execution_time [-527.111µs; -478.689µs] or [-8.151%; -7.402%]

scenario:SymfonyBench/benchSymfonyOverhead

  • 🟩 execution_time [-947.724µs; -892.476µs] or [-11.944%; -11.248%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

  • 🟩 execution_time [-924.632µs; -840.408µs] or [-11.519%; -10.470%]

scenario:WordPressBench/benchWordPressOverhead

  • 🟩 execution_time [-10.230ms; -8.324ms] or [-4.654%; -3.787%]

scenario:WordPressBench/benchWordPressOverhead-opcache

  • 🟩 execution_time [-9.471ms; -7.807ms] or [-4.280%; -3.528%]

@iamluc iamluc force-pushed the luc/sidecar-thread-safety branch from 16e3d8b to f47047e Compare May 22, 2024 09:11
components-rs/sidecar.rs Outdated Show resolved Hide resolved
@iamluc iamluc force-pushed the luc/sidecar-thread-safety branch from f47047e to f0231a8 Compare May 22, 2024 09:49
@iamluc iamluc force-pushed the luc/sidecar-thread-safety branch from f0231a8 to 10b9acf Compare May 22, 2024 09:55
@iamluc
Copy link
Contributor Author

iamluc commented May 27, 2024

Superseded by #2672

@iamluc iamluc closed this May 27, 2024
@iamluc iamluc deleted the luc/sidecar-thread-safety branch May 27, 2024 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants