diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index e4ce16ef5..7f2eb603b 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -260,11 +260,6 @@ parameters: count: 1 path: src/Options.php - - - message: "#^Method Sentry\\\\Options\\:\\:isStrictTracePropagationEnabled\\(\\) should return bool but returns mixed\\.$#" - count: 1 - path: src/Options.php - - message: "#^Method Sentry\\\\Options\\:\\:shouldAttachMetricCodeLocations\\(\\) should return bool but returns mixed\\.$#" count: 1 diff --git a/src/Options.php b/src/Options.php index 3f75ad897..5138b6489 100644 --- a/src/Options.php +++ b/src/Options.php @@ -61,6 +61,12 @@ public function __construct(array $options = []) $this->configureOptions($this->resolver); + // Migrate `strict_trace_propagation` over to `strict_trace_continuation` if not set. + // If both are set, then `strict_trace_continuation` will take precedence. + if (isset($options['strict_trace_propagation']) && !isset($options['strict_trace_continuation'])) { + $options['strict_trace_continuation'] = $options['strict_trace_propagation']; + } + $this->options = $this->resolver->resolve($options); if ($this->options['enable_tracing'] === true && $this->options['traces_sample_rate'] === null) { @@ -775,25 +781,50 @@ public function setTracePropagationTargets(array $tracePropagationTargets): self } /** - * Returns whether strict trace propagation is enabled or not. + * Returns whether strict trace continuation is enabled or not. */ - public function isStrictTracePropagationEnabled(): bool + public function isStrictTraceContinuationEnabled(): bool { - return $this->options['strict_trace_propagation']; + /** + * @var bool $result + */ + $result = $this->options['strict_trace_continuation']; + + return $result; } /** - * Sets if strict trace propagation should be enabled or not. + * Sets if strict trace continuation should be enabled or not. */ - public function enableStrictTracePropagation(bool $strictTracePropagation): self + public function enableStrictTraceContinuation(bool $strictTraceContinuation): self { - $options = array_merge($this->options, ['strict_trace_propagation' => $strictTracePropagation]); + $options = array_merge($this->options, ['strict_trace_continuation' => $strictTraceContinuation]); $this->options = $this->resolver->resolve($options); return $this; } + /** + * Returns whether strict trace propagation is enabled or not. + * + * @deprecated since version 4.21. To be removed in version 5.0. Use `isStrictTraceContinuationEnabled` instead. + */ + public function isStrictTracePropagationEnabled(): bool + { + return $this->isStrictTraceContinuationEnabled(); + } + + /** + * Sets if strict trace propagation should be enabled or not. + * + * @deprecated since version 4.21. To be removed in version 5.0. Use `enableStrictTraceContinuation` instead. + */ + public function enableStrictTracePropagation(bool $strictTracePropagation): self + { + return $this->enableStrictTraceContinuation($strictTracePropagation); + } + /** * Gets a list of default tags for events. * @@ -1352,6 +1383,7 @@ private function configureOptions(OptionsResolver $resolver): void return $metric; }, 'trace_propagation_targets' => null, + 'strict_trace_continuation' => false, 'strict_trace_propagation' => false, 'tags' => [], 'error_types' => null, @@ -1406,6 +1438,7 @@ private function configureOptions(OptionsResolver $resolver): void $resolver->setAllowedTypes('ignore_exceptions', 'string[]'); $resolver->setAllowedTypes('ignore_transactions', 'string[]'); $resolver->setAllowedTypes('trace_propagation_targets', ['null', 'string[]']); + $resolver->setAllowedTypes('strict_trace_continuation', 'bool'); $resolver->setAllowedTypes('strict_trace_propagation', 'bool'); $resolver->setAllowedTypes('tags', 'string[]'); $resolver->setAllowedTypes('error_types', ['null', 'int']); diff --git a/src/Tracing/DynamicSamplingContext.php b/src/Tracing/DynamicSamplingContext.php index 0304504ce..abea31a83 100644 --- a/src/Tracing/DynamicSamplingContext.php +++ b/src/Tracing/DynamicSamplingContext.php @@ -167,22 +167,7 @@ public static function fromTransaction(Transaction $transaction, HubInterface $h $client = $hub->getClient(); if ($client !== null) { - $options = $client->getOptions(); - - if ($options->getDsn() !== null && $options->getDsn()->getPublicKey() !== null) { - $samplingContext->set('public_key', $options->getDsn()->getPublicKey()); - } - if ($options->getDsn() !== null && $options->getDsn()->getOrgId() !== null) { - $samplingContext->set('org_id', (string) $options->getDsn()->getOrgId()); - } - - if ($options->getRelease() !== null) { - $samplingContext->set('release', $options->getRelease()); - } - - if ($options->getEnvironment() !== null) { - $samplingContext->set('environment', $options->getEnvironment()); - } + self::setOrgOptions($client->getOptions(), $samplingContext); } if ($transaction->getSampled() !== null) { @@ -208,11 +193,22 @@ public static function fromOptions(Options $options, Scope $scope): self $samplingContext->set('sample_rate', (string) $options->getTracesSampleRate()); } + self::setOrgOptions($options, $samplingContext); + + $samplingContext->freeze(); + + return $samplingContext; + } + + private static function setOrgOptions(Options $options, DynamicSamplingContext $samplingContext): void + { if ($options->getDsn() !== null && $options->getDsn()->getPublicKey() !== null) { $samplingContext->set('public_key', $options->getDsn()->getPublicKey()); } - if ($options->getDsn() !== null && $options->getDsn()->getOrgId() !== null) { + if ($options->getOrgId() !== null) { + $samplingContext->set('org_id', (string) $options->getOrgId()); + } elseif ($options->getDsn() !== null && $options->getDsn()->getOrgId() !== null) { $samplingContext->set('org_id', (string) $options->getDsn()->getOrgId()); } @@ -223,10 +219,6 @@ public static function fromOptions(Options $options, Scope $scope): self if ($options->getEnvironment() !== null) { $samplingContext->set('environment', $options->getEnvironment()); } - - $samplingContext->freeze(); - - return $samplingContext; } /** diff --git a/src/Tracing/Traits/TraceHeaderParserTrait.php b/src/Tracing/Traits/TraceHeaderParserTrait.php index dd30c29a5..1a5c0f7c0 100644 --- a/src/Tracing/Traits/TraceHeaderParserTrait.php +++ b/src/Tracing/Traits/TraceHeaderParserTrait.php @@ -4,6 +4,7 @@ namespace Sentry\Tracing\Traits; +use Sentry\SentrySdk; use Sentry\Tracing\DynamicSamplingContext; use Sentry\Tracing\SpanId; use Sentry\Tracing\TraceId; @@ -65,6 +66,14 @@ protected static function parseTraceAndBaggageHeaders(string $sentryTrace, strin $samplingContext = DynamicSamplingContext::fromHeader($baggage); + if ($hasSentryTrace && !self::shouldContinueTrace($samplingContext)) { + $result['traceId'] = null; + $result['parentSpanId'] = null; + $result['parentSampled'] = null; + + return $result; + } + if ($hasSentryTrace) { // The request comes from an old SDK which does not support Dynamic Sampling. // Propagate the Dynamic Sampling Context as is, but frozen, even without sentry-* entries. @@ -102,4 +111,51 @@ protected static function parseTraceAndBaggageHeaders(string $sentryTrace, strin return $result; } + + private static function shouldContinueTrace(DynamicSamplingContext $samplingContext): bool + { + $hub = SentrySdk::getCurrentHub(); + $client = $hub->getClient(); + + if ($client === null) { + return true; + } + + $options = $client->getOptions(); + $clientOrgId = $options->getOrgId(); + if ($clientOrgId === null && $options->getDsn() !== null) { + $clientOrgId = $options->getDsn()->getOrgId(); + } + + $baggageOrgId = $samplingContext->get('org_id'); + $logger = $options->getLoggerOrNullLogger(); + + // both org IDs are set but are not equals + if ($clientOrgId !== null && $baggageOrgId !== null && ((string) $clientOrgId !== $baggageOrgId)) { + $logger->debug( + \sprintf( + "Starting a new trace because org IDs don't match (incoming baggage org_id: %s, SDK org_id: %s)", + $baggageOrgId, + $clientOrgId + ) + ); + + return false; + } + + // One org ID is not set and strict trace continuation is enabled + if ($options->isStrictTraceContinuationEnabled() && ($clientOrgId === null) !== ($baggageOrgId === null)) { + $logger->debug( + \sprintf( + 'Starting a new trace because strict trace continuation is enabled and one org ID is missing (incoming baggage org_id: %s, SDK org_id: %s)', + $baggageOrgId !== null ? $baggageOrgId : 'none', + $clientOrgId !== null ? (string) $clientOrgId : 'none' + ) + ); + + return false; + } + + return true; + } } diff --git a/src/functions.php b/src/functions.php index 02b32c430..f146aa44c 100644 --- a/src/functions.php +++ b/src/functions.php @@ -62,7 +62,7 @@ * server_name?: string, * spotlight?: bool, * spotlight_url?: string, - * strict_trace_propagation?: bool, + * strict_trace_continuation?: bool, * tags?: array, * trace_propagation_targets?: array|null, * traces_sample_rate?: float|int|null, @@ -389,13 +389,32 @@ function getBaggage(): string */ function continueTrace(string $sentryTrace, string $baggage): TransactionContext { + // With the new `strict_trace_continuation`, it's possible that we start two new + // traces if we parse the TransactionContext and PropagationContext from the same + // headers. To make sure the trace is the same, we will create one transaction + // context from headers and copy relevant information over. + $transactionContext = TransactionContext::fromHeaders($sentryTrace, $baggage); + $propagationContext = PropagationContext::fromDefaults(); + $metadata = $transactionContext->getMetadata(); + + $traceId = $transactionContext->getTraceId() ?? $propagationContext->getTraceId(); + $transactionContext->setTraceId($traceId); + $propagationContext->setTraceId($traceId); + + $propagationContext->setParentSpanId($transactionContext->getParentSpanId()); + $propagationContext->setSampleRand($metadata->getSampleRand()); + + $dynamicSamplingContext = $metadata->getDynamicSamplingContext(); + if ($dynamicSamplingContext !== null) { + $propagationContext->setDynamicSamplingContext($dynamicSamplingContext); + } + $hub = SentrySdk::getCurrentHub(); - $hub->configureScope(static function (Scope $scope) use ($sentryTrace, $baggage) { - $propagationContext = PropagationContext::fromHeaders($sentryTrace, $baggage); + $hub->configureScope(static function (Scope $scope) use ($propagationContext): void { $scope->setPropagationContext($propagationContext); }); - return TransactionContext::fromHeaders($sentryTrace, $baggage); + return $transactionContext; } /** diff --git a/tests/FunctionsTest.php b/tests/FunctionsTest.php index 060b89d7c..b92926d2b 100644 --- a/tests/FunctionsTest.php +++ b/tests/FunctionsTest.php @@ -637,4 +637,77 @@ public function testContinueTrace(): void $this->assertTrue($dynamicSamplingContext->isFrozen()); }); } + + public function testContinueTraceWhenOrgMismatch(): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options([ + 'strict_trace_continuation' => true, + 'org_id' => 1, + ])); + + $hub = new Hub($client); + SentrySdk::setCurrentHub($hub); + + $transactionContext = continueTrace( + '566e3688a61d4bc888951642d6f14a19-566e3688a61d4bc8-1', + 'sentry-org_id=2' + ); + + $newTraceId = (string) $transactionContext->getTraceId(); + $newSampleRand = $transactionContext->getMetadata()->getSampleRand(); + + $this->assertNotSame('566e3688a61d4bc888951642d6f14a19', $newTraceId); + $this->assertNotEmpty($newTraceId); + $this->assertNull($transactionContext->getParentSpanId()); + $this->assertNull($transactionContext->getParentSampled()); + $this->assertNull($transactionContext->getMetadata()->getDynamicSamplingContext()); + $this->assertNotNull($newSampleRand); + + configureScope(function (Scope $scope) use ($newTraceId, $newSampleRand): void { + $propagationContext = $scope->getPropagationContext(); + + $this->assertSame($newTraceId, (string) $propagationContext->getTraceId()); + $this->assertNull($propagationContext->getParentSpanId()); + $this->assertNull($propagationContext->getDynamicSamplingContext()); + $this->assertSame($newSampleRand, $propagationContext->getSampleRand()); + }); + } + + public function testContinueTraceWhenOrgMatch(): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options([ + 'strict_trace_continuation' => true, + 'org_id' => 1, + ])); + + $hub = new Hub($client); + SentrySdk::setCurrentHub($hub); + + $transactionContext = continueTrace( + '566e3688a61d4bc888951642d6f14a19-566e3688a61d4bc8-1', + 'sentry-org_id=1' + ); + + $this->assertSame('566e3688a61d4bc888951642d6f14a19', (string) $transactionContext->getTraceId()); + $this->assertSame('566e3688a61d4bc8', (string) $transactionContext->getParentSpanId()); + $this->assertTrue($transactionContext->getParentSampled()); + + configureScope(function (Scope $scope): void { + $propagationContext = $scope->getPropagationContext(); + + $this->assertSame('566e3688a61d4bc888951642d6f14a19', (string) $propagationContext->getTraceId()); + $this->assertSame('566e3688a61d4bc8', (string) $propagationContext->getParentSpanId()); + + $dynamicSamplingContext = $propagationContext->getDynamicSamplingContext(); + + $this->assertNotNull($dynamicSamplingContext); + $this->assertSame('1', $dynamicSamplingContext->get('org_id')); + }); + } } diff --git a/tests/OptionsTest.php b/tests/OptionsTest.php index 860b88d5a..5c9e49822 100644 --- a/tests/OptionsTest.php +++ b/tests/OptionsTest.php @@ -289,6 +289,13 @@ static function (): void {}, 'setTracePropagationTargets', ]; + yield [ + 'strict_trace_continuation', + true, + 'isStrictTraceContinuationEnabled', + 'enableStrictTraceContinuation', + ]; + yield [ 'strict_trace_propagation', true, diff --git a/tests/Tracing/DynamicSamplingContextTest.php b/tests/Tracing/DynamicSamplingContextTest.php index 90ded1369..ebbb05080 100644 --- a/tests/Tracing/DynamicSamplingContextTest.php +++ b/tests/Tracing/DynamicSamplingContextTest.php @@ -153,6 +153,64 @@ public function testFromOptions(): void $this->assertTrue($samplingContext->isFrozen()); } + public function testFromOptionsUsesConfiguredOrgIdOverDsnOrgId(): void + { + $options = new Options([ + 'dsn' => 'http://public@o1.example.com/1', + 'org_id' => 2, + ]); + + $scope = new Scope(); + $samplingContext = DynamicSamplingContext::fromOptions($options, $scope); + + $this->assertSame('2', $samplingContext->get('org_id')); + } + + public function testFromOptionsFallsBackToDsnOrgId(): void + { + $options = new Options([ + 'dsn' => 'http://public@o1.example.com/1', + ]); + + $scope = new Scope(); + $samplingContext = DynamicSamplingContext::fromOptions($options, $scope); + + $this->assertSame('1', $samplingContext->get('org_id')); + } + + public function testFromTransactionUsesConfiguredOrgIdOverDsnOrgId(): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options([ + 'dsn' => 'http://public@o1.example.com/1', + 'org_id' => 2, + ])); + + $hub = new Hub($client); + $transaction = new Transaction(new TransactionContext(), $hub); + $samplingContext = DynamicSamplingContext::fromTransaction($transaction, $hub); + + $this->assertSame('2', $samplingContext->get('org_id')); + } + + public function testFromTransactionFallsBackToDsnOrgId(): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->once()) + ->method('getOptions') + ->willReturn(new Options([ + 'dsn' => 'http://public@o1.example.com/1', + ])); + + $hub = new Hub($client); + $transaction = new Transaction(new TransactionContext(), $hub); + $samplingContext = DynamicSamplingContext::fromTransaction($transaction, $hub); + + $this->assertSame('1', $samplingContext->get('org_id')); + } + /** * @dataProvider getEntriesDataProvider */ diff --git a/tests/Tracing/StrictTraceContinuationTest.php b/tests/Tracing/StrictTraceContinuationTest.php new file mode 100644 index 000000000..a92a1eab7 --- /dev/null +++ b/tests/Tracing/StrictTraceContinuationTest.php @@ -0,0 +1,214 @@ +createMock(ClientInterface::class); + $client->expects($this->exactly(2)) + ->method('getOptions') + ->willReturn($options); + + SentrySdk::setCurrentHub(new Hub($client)); + + $contexts = [ + PropagationContext::fromHeaders(self::SENTRY_TRACE_HEADER, $baggage), + PropagationContext::fromEnvironment(self::SENTRY_TRACE_HEADER, $baggage), + ]; + + foreach ($contexts as $context) { + if ($expectedContinueTrace) { + $this->assertSame('566e3688a61d4bc888951642d6f14a19', (string) $context->getTraceId()); + $this->assertSame('566e3688a61d4bc8', (string) $context->getParentSpanId()); + } else { + $this->assertNotSame('566e3688a61d4bc888951642d6f14a19', (string) $context->getTraceId()); + $this->assertNotEmpty((string) $context->getTraceId()); + $this->assertNull($context->getParentSpanId()); + $this->assertNull($context->getDynamicSamplingContext()); + } + } + } + + /** + * @dataProvider strictTraceContinuationDataProvider + */ + public function testTransactionContext(Options $options, string $baggage, bool $expectedContinueTrace): void + { + $client = $this->createMock(ClientInterface::class); + $client->expects($this->exactly(2)) + ->method('getOptions') + ->willReturn($options); + + SentrySdk::setCurrentHub(new Hub($client)); + + $contexts = [ + TransactionContext::fromHeaders(self::SENTRY_TRACE_HEADER, $baggage), + TransactionContext::fromEnvironment(self::SENTRY_TRACE_HEADER, $baggage), + ]; + + foreach ($contexts as $context) { + if ($expectedContinueTrace) { + $this->assertSame('566e3688a61d4bc888951642d6f14a19', (string) $context->getTraceId()); + $this->assertSame('566e3688a61d4bc8', (string) $context->getParentSpanId()); + $this->assertTrue($context->getParentSampled()); + } else { + $this->assertNotSame('566e3688a61d4bc888951642d6f14a19', (string) $context->getTraceId()); + $this->assertNull($context->getParentSpanId()); + $this->assertNull($context->getParentSampled()); + $this->assertNull($context->getMetadata()->getDynamicSamplingContext()); + } + } + } + + public static function strictTraceContinuationDataProvider(): \Generator + { + // First 10 Test cases are modelled after: https://develop.sentry.dev/sdk/telemetry/traces/#stricttracecontinuation + yield [ + new Options([ + 'strict_trace_continuation' => false, + 'org_id' => 1, + ]), + 'sentry-org_id=1', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => false, + 'org_id' => 1, + ]), + '', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => false, + ]), + 'sentry-org_id=1', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => false, + ]), + '', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => false, + 'org_id' => 2, + ]), + 'sentry-org_id=1', + false, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'org_id' => 1, + ]), + 'sentry-org_id=1', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'org_id' => 1, + ]), + '', + false, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + ]), + 'sentry-org_id=1', + false, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + ]), + '', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'org_id' => 2, + ]), + 'sentry-org_id=1', + false, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'dsn' => 'http://public@o1.example.com/1', + ]), + 'sentry-org_id=1', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'dsn' => 'http://public@o1.example.com/1', + 'org_id' => 2, + ]), + 'sentry-org_id=1', + false, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => true, + 'dsn' => 'http://public@o1.example.com/1', + 'org_id' => 2, + ]), + 'sentry-org_id=2', + true, + ]; + + yield [ + new Options([ + 'strict_trace_continuation' => false, + 'org_id' => 1, + ]), + 'sentry-org_id=01', + false, + ]; + } +}