From de8b3f030421a204af72711d664bfbb0ae3b4c0b Mon Sep 17 00:00:00 2001 From: Ruben Van Assche Date: Wed, 20 May 2026 12:40:43 +0200 Subject: [PATCH] Let the sampler vote on traces inherited from a traceparent Previously, when a trace was continued from a W3C traceparent (or an explicit traceId/spanId/sample tuple), the sampling decision was taken directly from the parent and the configured sampler was bypassed. That made DynamicSampler rules like forJob effectively dormant in practice, since jobs almost always run with an inherited traceparent. Trace continuity (the traceId and parent spanId) is now decoupled from the sampling decision. The sampler is always called, and receives the parent's sampling flag as a hint via a new optional parameter. - Sampler::shouldSample gains ?bool \$parentSampled = null - AlwaysSampler and NeverSampler ignore the hint - RateSampler honors the hint when set, otherwise rolls the rate - DynamicSampler evaluates rules first (a match overrides the parent), then falls back to the parent decision, then to base_rate - Tracer::startTrace drops the ?bool \$sample parameter; startFromTraceparent and startFromDefined are inlined Breaking change, acceptable pre-v3. --- UPGRADING.md | 14 ++++- src/Sampling/AlwaysSampler.php | 2 +- src/Sampling/DynamicSampler.php | 10 ++- src/Sampling/NeverSampler.php | 2 +- src/Sampling/RateSampler.php | 6 +- src/Sampling/Sampler.php | 2 +- src/Tracer.php | 70 ++++++--------------- tests/Sampling/DynamicSamplerTest.php | 87 +++++++++++++++++++++++++++ tests/Sampling/RateSamplerTest.php | 25 ++++++++ tests/Support/LifecylceTest.php | 2 +- tests/TracerTest.php | 10 +-- 11 files changed, 162 insertions(+), 68 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index ef51c3a..334209e 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -157,21 +157,29 @@ Both are now managed internally by `Lifecycle`. Drop any direct calls. #### Custom `Sampler` signature changed -`Sampler::shouldSample()` now receives an `EntryPoint` instead of an array. +`Sampler::shouldSample()` now receives an `EntryPoint` and an optional `?bool $parentSampled` that carries the sampling decision from an inherited W3C traceparent (or `null` when there is no parent). ```php // Before public function shouldSample(array $context): bool; // After -public function shouldSample(EntryPoint $entryPoint): bool; +public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool; ``` +Each built-in sampler handles the parent decision differently: + +- `AlwaysSampler` and `NeverSampler` ignore `$parentSampled` (their name is the contract). +- `RateSampler` honors `$parentSampled` when set, otherwise rolls the configured rate. +- `DynamicSampler` evaluates rules first. A matching rule wins (overriding the parent decision). If no rule matches, the parent's decision is honored, falling back to `base_rate` only when there is no parent. + +Custom samplers can ignore the new parameter to preserve existing behavior, but consider whether honoring the parent decision is the right default for your sampler. + The `SamplingType` enum was also removed. Use the boolean `$tracer->sampling` (or `isSampling()`) instead. #### Changes in `Tracer` -- `startTrace()` signature changed. +- `startTrace()` signature changed. The `?bool $sample` parameter was dropped. Trace continuity (the traceId and parent spanId) is now decoupled from the sampling decision: explicit `traceId`/`spanId` arguments or a `traceParent` set the IDs, then the configured sampler is always called (with the parent's sampled flag when available). - `addRawSpan()` was renamed to `addSpan()`. - `startTraceWithSpan()`, `setCurrentSpanId()`, and `trashCurrentTrace()` were removed without replacement. They're managed internally now. diff --git a/src/Sampling/AlwaysSampler.php b/src/Sampling/AlwaysSampler.php index c250678..6f92dd5 100644 --- a/src/Sampling/AlwaysSampler.php +++ b/src/Sampling/AlwaysSampler.php @@ -6,7 +6,7 @@ class AlwaysSampler implements Sampler { - public function shouldSample(EntryPoint $entryPoint): bool + public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool { return true; } diff --git a/src/Sampling/DynamicSampler.php b/src/Sampling/DynamicSampler.php index 22eacb0..935c8bd 100644 --- a/src/Sampling/DynamicSampler.php +++ b/src/Sampling/DynamicSampler.php @@ -11,6 +11,8 @@ class DynamicSampler extends RateSampler implements DeferrableSampler protected bool $pending = false; + protected ?bool $parentSampled = null; + public function __construct(array $config) { parent::__construct(['rate' => $config['base_rate'] ?? null]); @@ -23,9 +25,10 @@ public function __construct(array $config) ); } - public function shouldSample(EntryPoint $entryPoint): bool + public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool { $this->pending = false; + $this->parentSampled = $parentSampled; foreach ($this->rules as $rule) { if (! $rule->type()->appliesTo($entryPoint->type)) { @@ -49,7 +52,7 @@ public function shouldSample(EntryPoint $entryPoint): bool return true; } - return parent::shouldSample($entryPoint); + return $parentSampled ?? parent::shouldSample($entryPoint, null); } public function isPending(): bool @@ -77,11 +80,12 @@ public function reevaluate(EntryPoint $entryPoint): bool } } - return parent::shouldSample($entryPoint); + return $this->parentSampled ?? parent::shouldSample($entryPoint, null); } public function reset(): void { $this->pending = false; + $this->parentSampled = null; } } diff --git a/src/Sampling/NeverSampler.php b/src/Sampling/NeverSampler.php index 002ffe4..f4a62ee 100644 --- a/src/Sampling/NeverSampler.php +++ b/src/Sampling/NeverSampler.php @@ -6,7 +6,7 @@ class NeverSampler implements Sampler { - public function shouldSample(EntryPoint $entryPoint): bool + public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool { return false; } diff --git a/src/Sampling/RateSampler.php b/src/Sampling/RateSampler.php index 476c70a..6cf283f 100644 --- a/src/Sampling/RateSampler.php +++ b/src/Sampling/RateSampler.php @@ -20,8 +20,12 @@ public function __construct( $this->rate = $rate; } - public function shouldSample(EntryPoint $entryPoint): bool + public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool { + if ($parentSampled !== null) { + return $parentSampled; + } + return $this->decide($this->rate); } diff --git a/src/Sampling/Sampler.php b/src/Sampling/Sampler.php index 31406c1..6b67737 100644 --- a/src/Sampling/Sampler.php +++ b/src/Sampling/Sampler.php @@ -6,5 +6,5 @@ interface Sampler { - public function shouldSample(EntryPoint $entryPoint): bool; + public function shouldSample(EntryPoint $entryPoint, ?bool $parentSampled = null): bool; } diff --git a/src/Tracer.php b/src/Tracer.php index b49cd63..96c945d 100644 --- a/src/Tracer.php +++ b/src/Tracer.php @@ -79,8 +79,7 @@ public function __construct( public function startTrace( ?string $traceId = null, ?string $spanId = null, - ?bool $sample = null, - ?string $traceParent = null + ?string $traceParent = null, ): bool { if ($this->disabled === true) { return false; @@ -90,64 +89,35 @@ public function startTrace( return $this->sampling; } - if ($traceParent) { - return $this->startFromTraceparent($traceParent); - } + $parentSampled = null; - if ($traceId && $spanId && $sample !== null) { - return $this->startFromDefined( - sample: $sample, - traceId: $traceId, - spanId: $spanId, - currentSpanIdAvailable: false, - ); - } + if ($traceParent !== null) { + $parsed = $this->ids->parseTraceparent($traceParent); - if ($traceId || $spanId || $sample !== null) { - throw new Exception("If one of traceId, spanId or sample is provided, all three must be provided."); + if ($parsed !== null) { + $traceId = $parsed['traceId']; + $spanId = $parsed['parentSpanId']; + $parentSampled = $parsed['sampling']; + } } - return $this->sampling = $this->sampler->shouldSample($this->entryPointResolver->get()); - } - - protected function startFromTraceparent( - string $traceParent, - ): bool { - $parsedTraceparent = $this->ids->parseTraceparent($traceParent); - - if ($parsedTraceparent === null) { - return $this->startTrace(); + if (($traceId === null) !== ($spanId === null)) { + throw new Exception('If one of traceId or spanId is provided, both must be provided.'); } - [ - 'traceId' => $traceId, - 'parentSpanId' => $parentSpanId, - 'sampling' => $sampling, - ] = $parsedTraceparent; + if ($traceId !== null && $spanId !== null) { + $this->currentTraceId = $traceId; + $this->currentSpanId = $spanId; + $this->currentSpanIdAvailable = false; + $this->spans = []; + } - return $this->startFromDefined( - sample: $sampling, - traceId: $traceId, - spanId: $parentSpanId, - currentSpanIdAvailable: false + return $this->sampling = $this->sampler->shouldSample( + $this->entryPointResolver->get(), + $parentSampled, ); } - protected function startFromDefined( - bool $sample, - string $traceId, - string $spanId, - bool $currentSpanIdAvailable, - ): bool { - $this->currentTraceId = $traceId; - $this->currentSpanId = $spanId; - $this->currentSpanIdAvailable = $currentSpanIdAvailable; - - $this->spans = []; - - return $this->sampling = $sample; - } - public function endTrace(): void { if ($this->sampler instanceof DeferrableSampler) { diff --git a/tests/Sampling/DynamicSamplerTest.php b/tests/Sampling/DynamicSamplerTest.php index ac98363..196627b 100644 --- a/tests/Sampling/DynamicSamplerTest.php +++ b/tests/Sampling/DynamicSamplerTest.php @@ -302,3 +302,90 @@ expect($sampler->reevaluate($entryPoint))->toBeTrue(); }); + +it('honors parentSampled true when no rule matches and base rate is zero', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 0, + 'rules' => [ + SamplingRule::forPath('/admin/*', 1.0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/public'); + + expect($sampler->shouldSample($entryPoint, parentSampled: true))->toBeTrue(); +}); + +it('honors parentSampled false when no rule matches and base rate is one', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 1, + 'rules' => [ + SamplingRule::forPath('/admin/*', 1.0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/public'); + + expect($sampler->shouldSample($entryPoint, parentSampled: false))->toBeFalse(); +}); + +it('lets a matching rule override parentSampled', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 0, + 'rules' => [ + SamplingRule::forPath('/admin/*', 1.0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/admin/users'); + + expect($sampler->shouldSample($entryPoint, parentSampled: false))->toBeTrue(); +}); + +it('lets a matching rule with rate zero override parentSampled true', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 0, + 'rules' => [ + SamplingRule::forPath('/admin/*', 0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/admin/users'); + + expect($sampler->shouldSample($entryPoint, parentSampled: true))->toBeFalse(); +}); + +it('falls back to stored parentSampled on reevaluate when no rule matches', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 0, + 'rules' => [ + SamplingRule::forRoute('/admin/*', 1.0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/public'); + + $sampler->shouldSample($entryPoint, parentSampled: true); + + $entryPoint->setHandler('GET /public', 'PublicController', 'php_request'); + + expect($sampler->reevaluate($entryPoint))->toBeTrue(); +}); + +it('reset clears stored parentSampled so reevaluate falls back to base rate', function () { + $sampler = new DynamicSampler([ + 'base_rate' => 0, + 'rules' => [ + SamplingRule::forRoute('/admin/*', 1.0), + ], + ]); + + $entryPoint = new EntryPoint(EntryPointType::Web, 'https://example.com/public'); + + $sampler->shouldSample($entryPoint, parentSampled: true); + $sampler->reset(); + + $entryPoint->setHandler('GET /public', 'PublicController', 'php_request'); + + expect($sampler->reevaluate($entryPoint))->toBeFalse(); +}); diff --git a/tests/Sampling/RateSamplerTest.php b/tests/Sampling/RateSamplerTest.php index 128f634..e1619bd 100644 --- a/tests/Sampling/RateSamplerTest.php +++ b/tests/Sampling/RateSamplerTest.php @@ -67,3 +67,28 @@ it('cannot define a sample rate above 1', function () { new RateSampler(['rate' => 1.5]); })->throws(InvalidArgumentException::class, 'Rate must be between 0 and 1'); + +it('honors parentSampled true regardless of rate', function () { + $sampler = new RateSampler(['rate' => 0]); + $entryPoint = new EntryPoint(EntryPointType::Web, 'http://localhost'); + + for ($i = 0; $i < 100; $i++) { + expect($sampler->shouldSample($entryPoint, true))->toBeTrue(); + } +}); + +it('honors parentSampled false regardless of rate', function () { + $sampler = new RateSampler(['rate' => 1]); + $entryPoint = new EntryPoint(EntryPointType::Web, 'http://localhost'); + + for ($i = 0; $i < 100; $i++) { + expect($sampler->shouldSample($entryPoint, false))->toBeFalse(); + } +}); + +it('rolls the rate when parentSampled is null', function () { + $sampler = new RateSampler(['rate' => 0]); + $entryPoint = new EntryPoint(EntryPointType::Web, 'http://localhost'); + + expect($sampler->shouldSample($entryPoint, null))->toBeFalse(); +}); diff --git a/tests/Support/LifecylceTest.php b/tests/Support/LifecylceTest.php index 7a97c52..0b3ede3 100644 --- a/tests/Support/LifecylceTest.php +++ b/tests/Support/LifecylceTest.php @@ -33,7 +33,7 @@ }); it('can continue a trace with a traceparent', function () { - $flare = setupFlare(fn (FlareConfig $config) => $config->neverSampleTraces()); + $flare = setupFlare(fn (FlareConfig $config) => $config->sampleRate(0)); $traceParent = $flare->ids->traceParent( $traceId = $flare->ids->trace(), diff --git a/tests/TracerTest.php b/tests/TracerTest.php index 258f41b..da163fb 100644 --- a/tests/TracerTest.php +++ b/tests/TracerTest.php @@ -84,25 +84,21 @@ expect($tracer->currentTraceId())->not()->toBeNull(); }); -it('can potentially resume a trace', function () { +it('can resume a trace with explicit ids and lets the sampler decide', function () { $tracer = setupFlare(fn (FlareConfig $config) => $config->sampleRate(1))->tracer; - $tracer->startTrace(traceId: 'traceid', spanId: 'spanid', sample: true); + $tracer->startTrace(traceId: 'traceid', spanId: 'spanid'); expect($tracer->isSampling())->toBeTrue(); expect($tracer->currentTraceId())->toEqual('traceid'); expect($tracer->currentSpanId())->toEqual('spanid'); }); -it('requires all three parameters to resume a possible trace', function () { +it('requires both traceId and spanId when one is provided', function () { $tracer = setupFlare(fn (FlareConfig $config) => $config->sampleRate(1))->tracer; expect(fn () => $tracer->startTrace(traceId: 'traceid'))->toThrow(Exception::class); expect(fn () => $tracer->startTrace(spanId: 'span_id'))->toThrow(Exception::class); - expect(fn () => $tracer->startTrace(sample: true))->toThrow(Exception::class); - expect(fn () => $tracer->startTrace(traceId: 'traceid', spanId: 'spanid'))->toThrow(Exception::class); - expect(fn () => $tracer->startTrace(traceId: 'traceid', sample: true))->toThrow(Exception::class); - expect(fn () => $tracer->startTrace(spanId: 'spanid', sample: true))->toThrow(Exception::class); }); it('can potentially resume a trace with traceparent', function () {