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

Ensure DDTRACE_G(active_stack) is always NULL when the root span is being pushed #2701

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Jun 10, 2024

I have zero idea how this happened, given that I'm unable to reproduce it, but a core dump indicated that the memory it pointed to was from the previous request. While this is not a very proper fix (as this should not be possible in the first place), it mitigates the effects.

So, better a mitigation than a crash... :-)

…eing pushed

I have zero idea _how_ this happened, given that I'm unable to reproduce it, but a core dump indicated that the memory it pointed to was from the previous request.
While this is not a very proper fix (as this should not be possible in the first place), it mitigates the effects.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
@bwoebi bwoebi requested a review from a team as a code owner June 10, 2024 11:02
@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.96%. Comparing base (c5f2dcb) to head (4157bd2).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2701   +/-   ##
=========================================
  Coverage     77.96%   77.96%           
  Complexity     2212     2212           
=========================================
  Files           227      227           
  Lines         26551    26552    +1     
  Branches        988      988           
=========================================
+ Hits          20701    20702    +1     
  Misses         5324     5324           
  Partials        526      526           
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.86% <100.00%> (+<0.01%) ⬆️
tracer-php 80.52% <ø> (ø)

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

Files Coverage Δ
ext/ddtrace.c 74.38% <100.00%> (+0.01%) ⬆️

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 c5f2dcb...4157bd2. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Jun 10, 2024

Benchmarks

Benchmark execution time: 2024-06-10 11:28:42

Comparing candidate commit 4157bd2 in PR branch bob/safety-stack with baseline commit c5f2dcb in branch master.

Found 3 performance improvements and 1 performance regressions! Performance is the same for 174 metrics, 0 unstable metrics.

scenario:LaravelBench/benchLaravelOverhead

  • 🟥 execution_time [+58.800µs; +201.920µs] or [+2.151%; +7.387%]

scenario:PDOBench/benchPDOBaseline

  • 🟩 execution_time [-17.142µs; -14.850µs] or [-9.033%; -7.825%]

scenario:PDOBench/benchPDOOverhead

  • 🟩 execution_time [-18.637µs; -16.546µs] or [-6.388%; -5.671%]

scenario:PDOBench/benchPDOOverheadWithDBM

  • 🟩 execution_time [-17.671µs; -15.962µs] or [-5.585%; -5.045%]

@bwoebi bwoebi merged commit 8686a6f into master Jun 10, 2024
588 of 606 checks passed
@bwoebi bwoebi deleted the bob/safety-stack branch June 10, 2024 11:57
@github-actions github-actions bot added this to the 1.1.0 milestone Jun 10, 2024
morrisonlevi pushed a commit that referenced this pull request Jun 10, 2024
…eing pushed (#2701)

I have zero idea _how_ this happened, given that I'm unable to reproduce it, but a core dump indicated that the memory it pointed to was from the previous request.
While this is not a very proper fix (as this should not be possible in the first place), it mitigates the effects.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
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.

3 participants