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 () {