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

Move collecting and flushing telemetry to after final request flush #2555

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Mar 4, 2024

Moving things around, basically between request flush and request memory free.

Reduces the request latency, even though it doesn't change anything about CPU time used.

Note: This does not affect the cli-server sapi.

@bwoebi bwoebi requested a review from a team as a code owner March 4, 2024 23:37
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

Merging #2555 (bc54d0e) into master (2e1ed12) will decrease coverage by 0.04%.
The diff coverage is 70.96%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2555      +/-   ##
============================================
- Coverage     75.91%   75.88%   -0.04%     
  Complexity     2574     2574              
============================================
  Files           240      240              
  Lines         27029    27069      +40     
  Branches        976      976              
============================================
+ Hits          20519    20541      +22     
- Misses         5990     6008      +18     
  Partials        520      520              
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.62% <70.96%> (-0.08%) ⬇️
tracer-php 75.08% <ø> (ø)

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

Files Coverage Δ
ext/ddtrace.h 62.50% <ø> (ø)
ext/telemetry.c 96.92% <89.47%> (-3.08%) ⬇️
ext/ddtrace.c 72.61% <62.79%> (-0.53%) ⬇️

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 2e1ed12...bc54d0e. Read the comment docs.

@bwoebi bwoebi force-pushed the bob/remove-telemetry-from-request branch from d0a2ed6 to 2bc3062 Compare March 5, 2024 00:01
@pr-commenter
Copy link

pr-commenter bot commented Mar 5, 2024

Benchmarks

Benchmark execution time: 2024-03-06 12:34:48

Comparing candidate commit bc54d0e in PR branch bob/remove-telemetry-from-request with baseline commit 2e1ed12 in branch master.

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

scenario:SamplingRuleMatchingBench/benchRegexMatching1

  • 🟥 execution_time [+187.168ns; +375.032ns] or [+3.064%; +6.139%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3-opcache

  • 🟥 execution_time [+143.538ns; +284.462ns] or [+2.387%; +4.731%]

@bwoebi bwoebi force-pushed the bob/remove-telemetry-from-request branch 7 times, most recently from 27ac0fe to 23fda2b Compare March 6, 2024 12:00
@bwoebi bwoebi force-pushed the bob/remove-telemetry-from-request branch from 23fda2b to bc54d0e Compare March 6, 2024 12:03
@bwoebi bwoebi marked this pull request as draft March 28, 2024 19:16
@bwoebi
Copy link
Collaborator Author

bwoebi commented Apr 19, 2024

This approach is problematic because of php/php-src#10335 (comment). Maybe we can use it once it's somehow fixed upstream?

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.

2 participants