Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2550 +/- ##
============================================
- Coverage 77.08% 75.91% -1.18%
+ Complexity 2574 2563 -11
============================================
Files 214 241 +27
Lines 23057 27020 +3963
Branches 0 976 +976
============================================
+ Hits 17773 20511 +2738
- Misses 5284 5989 +705
- Partials 0 520 +520
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 26 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
BenchmarksBenchmark execution time: 2024-03-05 14:22:30 Comparing candidate commit 4f2ee25 in PR branch Found 0 performance improvements and 4 performance regressions! Performance is the same for 178 metrics, 0 unstable metrics. scenario:PDOBench/benchPDOBaseline-opcache
scenario:PDOBench/benchPDOOverhead-opcache
scenario:PDOBench/benchPDOOverheadWithDBM-opcache
|
|
|
||
| class Common | ||
| { | ||
| // When modifying this method, please also pay attention to the tests/ext/orphans.phpt test and update it accordingly if needed. |
There was a problem hiding this comment.
Is there a reason why we cannot test that as part of an Integration test (isolated test testing Common::handleOrphan() directly is fine)? I'd rather avoid duplicating logic like this.
It also doesn't really fit in tests/ext as it's not testing the extension either, but testing how this particular code here interacts with functionality the extension provides.
There was a problem hiding this comment.
I tried reaching out to the /next-response endpoint of the Request Replayer from TracerTestTrait, but it wouldn't work for w/e reason. I'm tempted to just use the capabilities offered by the test agent instead tbh
There was a problem hiding this comment.
Feel free to use that if it works :-)
PROFeNoM
force-pushed
the
alex/fix/orphans
branch
from
7087742 to
cf159da
Compare
PROFeNoM
force-pushed
the
alex/fix/orphans
branch
from
cf159da to
4f2ee25
Compare
bwoebi
left a comment
bwoebi
left a comment
There was a problem hiding this comment.
looks good to me now, thanks :-)
3 participants
Description
🚨 Requires #2556 🚨
What's the issue?
While doing some tests locally on my Laravel App, some orphan spans wouldn't get sampled away while they were in previous tracer versions.
What's happening?
In real life (i.e., not in our tests), the default fallback rule
(service: , env: )would get evaluated. This rule exists because of the default agent sampling rate of 10 traces per second. Most notably, when the rate per service would become0, then\DDTrace\get_priority_sampling()would evaluate to zero, and we wouldn't set the priority sampling to auto reject again.What's the fix?
The fix adds the check
|| $prioritySampling == DD_TRACE_PRIORITY_SAMPLING_AUTO_REJECTto the rejection condition.While still not entirely realistic, the tests aim to assess that the code used to handle orphans works across values of agent sampling that would be evaluated through the default fallback rule.
Additionally, there were two additional
mysqli_queryspans in one test from this PR. Let's just test what we want actually to test.Reviewer checklist