From 2e15427af8da1783f0c201d9550a5a4ad30df94b Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Thu, 4 Jan 2024 11:15:32 +0100 Subject: [PATCH 1/7] feat: Basic OTel Span Links Functionalities --- src/DDTrace/OpenTelemetry/Context.php | 17 +++++++- src/DDTrace/OpenTelemetry/Span.php | 40 ++++++++++++++++++- src/DDTrace/OpenTelemetry/SpanBuilder.php | 21 +++++++++- .../Integration/SDK/SpanBuilderTest.php | 6 --- 4 files changed, 72 insertions(+), 12 deletions(-) diff --git a/src/DDTrace/OpenTelemetry/Context.php b/src/DDTrace/OpenTelemetry/Context.php index dcea348ffe..0e28b8486d 100644 --- a/src/DDTrace/OpenTelemetry/Context.php +++ b/src/DDTrace/OpenTelemetry/Context.php @@ -8,6 +8,7 @@ use DDTrace\Tag; use DDTrace\Util\ObjectKVStore; use OpenTelemetry\API\Trace as API; +use OpenTelemetry\SDK\Common\Attribute\Attributes; use OpenTelemetry\SDK\Common\Attribute\AttributesFactory; use OpenTelemetry\SDK\Common\Instrumentation\InstrumentationScopeFactory; use OpenTelemetry\SDK\Common\Instrumentation\InstrumentationScopeInterface; @@ -176,6 +177,18 @@ private static function activateParent(?SpanData $currentSpan): ContextInterface : null; $traceState = new API\TraceState($traceContext['tracestate'] ?? null); + // Check for span links + $links = []; + foreach ($currentSpan->links as $spanLink) { + $linkSpanContext = API\SpanContext::create( + $spanLink->traceId, + $spanLink->spanId, + API\TraceFlags::DEFAULT, + new API\TraceState($spanLink->traceState ?? null), + ); + $links[] = new SDK\Link($linkSpanContext, Attributes::create($spanLink->attributes)); + } + $OTelCurrentSpan = SDK\Span::startSpan( $currentSpan, API\SpanContext::create($currentTraceId, $currentSpanId, $traceFlags, $traceState), // $context @@ -186,8 +199,8 @@ private static function activateParent(?SpanData $currentSpan): ContextInterface NoopSpanProcessor::getInstance(), // $spanProcessor ResourceInfoFactory::defaultResource(), // $resource (new AttributesFactory())->builder(), // $attributesBuilder - [], // TODO: Handle Span Links - 0, // TODO: Handle Span Links + $links, // $links + count($links), // $totalRecordedLinks false // The span was created using the DD Api ); ObjectKVStore::put($currentSpan, 'otel_span', $OTelCurrentSpan); diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index ae823a71c3..d1fc3a1f39 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -5,6 +5,7 @@ namespace OpenTelemetry\SDK\Trace; use DDTrace\SpanData; +use DDTrace\SpanLink; use DDTrace\Tag; use DDTrace\Util\Convention; use DDTrace\Util\ObjectKVStore; @@ -35,6 +36,16 @@ final class Span extends API\Span implements ReadWriteSpanInterface /** @readonly */ private SpanProcessorInterface $spanProcessor; + /** + * @readonly + * + * @var list + */ + private array $links; + + /** @readonly */ + private int $totalRecordedLinks; + /** @readonly */ private int $kind; @@ -56,6 +67,8 @@ private function __construct( API\SpanContextInterface $parentSpanContext, SpanProcessorInterface $spanProcessor, ResourceInfo $resource, + array $links = [], + int $totalRecordedLinks = 0, bool $isRemapped = true ) { $this->span = $span; @@ -65,6 +78,8 @@ private function __construct( $this->parentSpanContext = $parentSpanContext; $this->spanProcessor = $spanProcessor; $this->resource = $resource; + $this->links = $links; + $this->totalRecordedLinks = $totalRecordedLinks; $this->status = StatusData::unset(); @@ -75,6 +90,25 @@ private function __construct( // done in serializer.c:ddtrace_set_root_span_properties (as of v0.92.0) $span->name = $this->operationNameConvention = Convention::defaultOperationName($span); } + + // Set the span links + if ($isRemapped) { + // At initialization time (now), only set the links if the span was created using the OTel API + // Otherwise, the links were already set in DD's OpenTelemetry\Context\Context + foreach ($links as $link) { + /** @var LinkInterface $link */ + $linkContext = $link->getSpanContext(); + + $spanLink = new SpanLink(); + $spanLink->traceId = $linkContext->getTraceId(); + $spanLink->spanId = $linkContext->getSpanId(); + $spanLink->traceState = (string)$linkContext->getTraceState(); // __toString() + $spanLink->attributes = $link->getAttributes()->toArray(); + $spanLink->droppedAttributesCount = 0; // Attributes limit aren't supported/meaningful in DD + + $span->links[] = $spanLink; + } + } } /** @@ -116,6 +150,8 @@ public static function startSpan( $parentSpan->getContext(), $spanProcessor, $resource, + $links, + $totalRecordedLinks, $isRemapped ); @@ -165,7 +201,7 @@ public function toSpanData(): SpanDataInterface return new ImmutableSpan( $this, $this->getName(), - [], // TODO: Handle Span Links + $this->links, [], // TODO: Handle Span Events Attributes::create(array_merge($this->span->meta, $this->span->metrics)), 0, @@ -206,7 +242,7 @@ public function getStartEpochNanos(): int public function getTotalRecordedLinks(): int { - return 0; + return $this->totalRecordedLinks; } public function getTotalRecordedEvents(): int diff --git a/src/DDTrace/OpenTelemetry/SpanBuilder.php b/src/DDTrace/OpenTelemetry/SpanBuilder.php index af96184adf..66f7db119a 100644 --- a/src/DDTrace/OpenTelemetry/SpanBuilder.php +++ b/src/DDTrace/OpenTelemetry/SpanBuilder.php @@ -69,7 +69,24 @@ public function setParent($context): API\SpanBuilderInterface public function addLink(SpanContextInterface $context, iterable $attributes = []): SpanBuilderInterface { - // TODO: Span Links are future works + if (!$context->isValid()) { + return $this; + } + + $this->totalNumberOfLinksAdded++; + + if (count($this->links) === $this->tracerSharedState->getSpanLimits()->getLinkCountLimit()) { + return $this; + } + + $this->links[] = new Link( + $context, + $this->tracerSharedState + ->getSpanLimits() + ->getLinkAttributesFactory() + ->builder($attributes) + ->build(), + ); return $this; } @@ -186,7 +203,7 @@ public function startSpan(): SpanInterface $this->tracerSharedState->getResource(), $attributesBuilder, $this->links, - 0, + $this->totalNumberOfLinksAdded, ); } diff --git a/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php b/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php index d024e1a1fa..662022b56e 100644 --- a/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php +++ b/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php @@ -98,7 +98,6 @@ public function test_set_parent_invalid_context(): void */ public function test_add_link(): void { - $this->markTestSkipped("Span Links aren't yet supported"); /** @var Span $span */ $span = $this ->tracer @@ -112,7 +111,6 @@ public function test_add_link(): void public function test_add_link_invalid(): void { - $this->markTestIncomplete("Span Links aren't yet supported"); /** @var Span $span */ $span = $this ->tracer @@ -127,7 +125,6 @@ public function test_add_link_invalid(): void public function test_add_link_dropping_links(): void { - $this->markTestSkipped("Span Links aren't yet supported"); $maxNumberOfLinks = 8; $tracerProvider = new TracerProvider([], null, null, (new SpanLimitsBuilder())->setLinkCountLimit($maxNumberOfLinks)->build()); $spanBuilder = $tracerProvider @@ -156,7 +153,6 @@ public function test_add_link_dropping_links(): void public function test_add_link_truncate_link_attributes(): void { - $this->markTestSkipped("Span Links aren't yet supported"); $tracerProvider = new TracerProvider([], null, null, (new SpanLimitsBuilder())->setAttributePerLinkCountLimit(1)->build()); /** @var Span $span */ $span = $tracerProvider @@ -178,7 +174,6 @@ public function test_add_link_truncate_link_attributes(): void public function test_add_link_truncate_link_attribute_value(): void { - $this->markTestSkipped("Spans Links aren't yet supported"); $maxLength = 25; $strVal = str_repeat('a', $maxLength); @@ -218,7 +213,6 @@ public function test_add_link_truncate_link_attribute_value(): void */ public function test_add_link_no_effect_after_start_span(): void { - $this->markTestSkipped("Span Links aren't yet supported"); $spanBuilder = $this->tracer->spanBuilder(self::SPAN_NAME); /** @var Span $span */ From d6870ac0507065e093f1bc31d786697377b9a16e Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Thu, 4 Jan 2024 11:33:57 +0100 Subject: [PATCH 2/7] test: Add Basic Interoperability Tests --- .../Integration/InteroperabilityTest.php | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php index 46348090b7..96c6055c93 100644 --- a/tests/OpenTelemetry/Integration/InteroperabilityTest.php +++ b/tests/OpenTelemetry/Integration/InteroperabilityTest.php @@ -2,6 +2,7 @@ namespace DDTrace\Tests\OpenTelemetry\Integration; +use DDTrace\SpanLink; use DDTrace\Tag; use DDTrace\Tests\Common\BaseTestCase; use DDTrace\Tests\Common\SpanAssertion; @@ -14,6 +15,8 @@ use OpenTelemetry\API\Trace\SpanContext; use OpenTelemetry\API\Trace\SpanContextValidator; use OpenTelemetry\API\Trace\SpanKind; +use OpenTelemetry\API\Trace\TraceFlags; +use OpenTelemetry\API\Trace\TraceState; use OpenTelemetry\Context\Context; use OpenTelemetry\Context\ContextStorage; use OpenTelemetry\Extension\Propagator\B3\B3Propagator; @@ -907,5 +910,69 @@ public function testAttributesInteroperability() $this->assertEquals(2, $traces[0][0]['metrics']['m2']); } + public function testSpanLinksInteroperabilityFromDatadogSpan() + { + $traces = $this->isolateTracer(function () { + $span = start_span(); + $span->name = "dd.span"; + + $spanLink = new SpanLink(); + $spanLink->traceId = "ff0000000000051791e0000000000041"; + $spanLink->spanId = "ff00000000000517"; + $spanLink->traceState = "dd=t.dm:-0"; + $spanLink->attributes = [ + 'arg1' => 'value1', + 'arg2' => 'value2', + ]; + $span->links[] = $spanLink; + + /** @var \OpenTelemetry\SDK\Trace\Span $OTelSpan */ + $OTelSpan = Span::getCurrent(); + $OTelSpanLink = $OTelSpan->toSpanData()->getLinks()[0]; + $OTelSpanLinkContext = $OTelSpanLink->getSpanContext(); + + $this->assertSame('ff0000000000051791e0000000000041', $OTelSpanLinkContext->getTraceId()); + $this->assertSame('ff00000000000517', $OTelSpanLinkContext->getSpanId()); + $this->assertSame('dd=t.dm:-0', (string) $OTelSpanLinkContext->getTraceState()); + + $this->assertSame([ + 'arg1' => 'value1', + 'arg2' => 'value2', + ], $OTelSpanLink->getAttributes()->toArray()); + + close_span(); + }); + $this->assertCount(1, $traces[0]); + $this->assertSame("[{\"trace_id\":\"ff0000000000051791e0000000000041\",\"span_id\":\"ff00000000000517\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"arg1\":\"value1\",\"arg2\":\"value2\"}}]", $traces[0][0]['meta']['_dd.span_links']); + } + + public function testSpanLinksInteroperabilityFromOpenTelemetrySpan() + { + $sampledSpanContext = SpanContext::create( + '12345678876543211234567887654321', + '8765432112345678', + TraceFlags::SAMPLED, + new TraceState('dd=t.dm:-0') + ); + + $traces = $this->isolateTracer(function () use ($sampledSpanContext) { + $otelSpan = self::getTracer()->spanBuilder("otel.span") + ->addLink($sampledSpanContext, ['arg1' => 'value1']) + ->startSpan(); + + $activeSpan = active_span(); + $spanLink = $activeSpan->links[0]; + $this->assertSame('12345678876543211234567887654321', $spanLink->traceId); + $this->assertSame('8765432112345678', $spanLink->spanId); + $this->assertSame('dd=t.dm:-0', $spanLink->traceState); + $this->assertSame(['arg1' => 'value1'], $spanLink->attributes); + $this->assertEquals(0, $spanLink->droppedAttributesCount); + + $otelSpan->end(); + }); + + $this->assertCount(1, $traces[0]); + $this->assertSame("[{\"trace_id\":\"12345678876543211234567887654321\",\"span_id\":\"8765432112345678\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"arg1\":\"value1\"},\"dropped_attributes_count\":0}]", $traces[0][0]['meta']['_dd.span_links']); + } } From ab750d89db1b598d70cb46f35f465805411d2a84 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Thu, 4 Jan 2024 11:56:38 +0100 Subject: [PATCH 3/7] feat: Partially handle on-the-fly addition of span links --- src/DDTrace/OpenTelemetry/Span.php | 28 ++++++++++ .../Integration/InteroperabilityTest.php | 55 +++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index d1fc3a1f39..766e6aaf58 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -198,6 +198,8 @@ public function toSpanData(): SpanDataInterface { $hasEnded = $this->hasEnded(); + $this->updateSpanLinks(); + return new ImmutableSpan( $this, $this->getName(), @@ -436,4 +438,30 @@ public function getDDSpan(): SpanData { return $this->span; } + + private function updateSpanLinks() + { + // Important: Links are assumed not to be removable to update the span links from Datadog to OpenTelemetry + // For instance, if there are 4 links in Datadog and only 2 in OpenTelemetry, the 2 missing links will be added + // to OpenTelemetry + $datadogSpanLinks = $this->span->links; + $otelSpanLinks = $this->links; + + $datadogSpanLinksCount = count($datadogSpanLinks); + $otelSpanLinksCount = count($otelSpanLinks); + + for ($i = $otelSpanLinksCount; $i < $datadogSpanLinksCount; $i++) { + $spanLink = $datadogSpanLinks[$i]; + + $linkSpanContext = API\SpanContext::create( + $spanLink->traceId, + $spanLink->spanId, + API\TraceFlags::DEFAULT, + new API\TraceState($spanLink->traceState ?? null), + ); + + $this->links[] = new Link($linkSpanContext, Attributes::create($spanLink->attributes ?? [])); + $this->totalRecordedLinks++; + } + } } diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php index 96c6055c93..8ea065c725 100644 --- a/tests/OpenTelemetry/Integration/InteroperabilityTest.php +++ b/tests/OpenTelemetry/Integration/InteroperabilityTest.php @@ -975,4 +975,59 @@ public function testSpanLinksInteroperabilityFromOpenTelemetrySpan() $this->assertCount(1, $traces[0]); $this->assertSame("[{\"trace_id\":\"12345678876543211234567887654321\",\"span_id\":\"8765432112345678\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"arg1\":\"value1\"},\"dropped_attributes_count\":0}]", $traces[0][0]['meta']['_dd.span_links']); } + + public function testSpanLinksInteroperabilityBothTypes() + { + $sampledSpanContext = SpanContext::create( + '12345678876543211234567887654321', + '8765432112345678', + TraceFlags::SAMPLED, + new TraceState('dd=t.dm:-0') + ); + + $traces = $this->isolateTracer(function () use ($sampledSpanContext) { + // Add 1 span link using the OTel API + $otelSpan = self::getTracer()->spanBuilder("otel.span") + ->addLink($sampledSpanContext, ['arg1' => 'value1']) + ->startSpan(); + + // Add 1 span link using the DD API + $newSpanLink = new SpanLink(); + $newSpanLink->traceId = "ff0000000000051791e0000000000041"; + $newSpanLink->spanId = "ff00000000000517"; + active_span()->links[] = $newSpanLink; + + // Verify the span links from DD's POV + $datadogSpanLinks = active_span()->links; + $this->assertCount(2, $datadogSpanLinks); + + $this->assertSame('12345678876543211234567887654321', $datadogSpanLinks[0]->traceId); + $this->assertSame('8765432112345678', $datadogSpanLinks[0]->spanId); + $this->assertSame('dd=t.dm:-0', $datadogSpanLinks[0]->traceState); + $this->assertSame(['arg1' => 'value1'], $datadogSpanLinks[0]->attributes); + $this->assertEquals(0, $datadogSpanLinks[0]->droppedAttributesCount); + + $this->assertSame('ff0000000000051791e0000000000041', $datadogSpanLinks[1]->traceId); + $this->assertSame('ff00000000000517', $datadogSpanLinks[1]->spanId); + + // Verify the span links from OTel's POV + $otelSpanLinks = $otelSpan->toSpanData()->getLinks(); + + $firstSpanLinkContext = $otelSpanLinks[0]->getSpanContext(); + $this->assertSame('12345678876543211234567887654321', $firstSpanLinkContext->getTraceId()); + $this->assertSame('8765432112345678', $firstSpanLinkContext->getSpanId()); + $this->assertSame('dd=t.dm:-0', (string) $firstSpanLinkContext->getTraceState()); + $this->assertSame(['arg1' => 'value1'], $otelSpanLinks[0]->getAttributes()->toArray()); + + $secondSpanLinkContext = $otelSpanLinks[1]->getSpanContext(); + $this->assertSame('ff0000000000051791e0000000000041', $secondSpanLinkContext->getTraceId()); + $this->assertSame('ff00000000000517', $secondSpanLinkContext->getSpanId()); + + + $otelSpan->end(); + }); + + $this->assertCount(1, $traces[0]); + $this->assertSame("[{\"trace_id\":\"12345678876543211234567887654321\",\"span_id\":\"8765432112345678\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"arg1\":\"value1\"},\"dropped_attributes_count\":0},{\"trace_id\":\"ff0000000000051791e0000000000041\",\"span_id\":\"ff00000000000517\"}]", $traces[0][0]['meta']['_dd.span_links']); + } } From f183ba67ddbba1b749952d061ece7b60eb8e16a8 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Mon, 8 Jan 2024 09:02:18 +0100 Subject: [PATCH 4/7] feat: Handle Span Links Removal (Interoperability) --- src/DDTrace/OpenTelemetry/Span.php | 53 +++++++---- src/DDTrace/OpenTelemetry/SpanBuilder.php | 4 - .../Integration/InteroperabilityTest.php | 92 +++++++++++++++++++ .../Integration/SDK/SpanBuilderTest.php | 1 + 4 files changed, 127 insertions(+), 23 deletions(-) diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index 766e6aaf58..0f44f254ce 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -59,6 +59,8 @@ final class Span extends API\Span implements ReadWriteSpanInterface private string $operationNameConvention = ""; + private string $uniqueIdentifier = ""; + private function __construct( SpanData $span, API\SpanContextInterface $context, @@ -91,6 +93,8 @@ private function __construct( $span->name = $this->operationNameConvention = Convention::defaultOperationName($span); } + $this->uniqueIdentifier = \spl_object_hash($this); // traceId + spanId is NOT a valid unique identifier, as the traceId can be modified + // Set the span links if ($isRemapped) { // At initialization time (now), only set the links if the span was created using the OTel API @@ -106,6 +110,8 @@ private function __construct( $spanLink->attributes = $link->getAttributes()->toArray(); $spanLink->droppedAttributesCount = 0; // Attributes limit aren't supported/meaningful in DD + // Save the link + ObjectKVStore::put($spanLink, $this->uniqueIdentifier, $link); $span->links[] = $spanLink; } } @@ -441,27 +447,36 @@ public function getDDSpan(): SpanData private function updateSpanLinks() { - // Important: Links are assumed not to be removable to update the span links from Datadog to OpenTelemetry - // For instance, if there are 4 links in Datadog and only 2 in OpenTelemetry, the 2 missing links will be added - // to OpenTelemetry + // Important: Span Links are supposed immutable $datadogSpanLinks = $this->span->links; - $otelSpanLinks = $this->links; - - $datadogSpanLinksCount = count($datadogSpanLinks); - $otelSpanLinksCount = count($otelSpanLinks); - - for ($i = $otelSpanLinksCount; $i < $datadogSpanLinksCount; $i++) { - $spanLink = $datadogSpanLinks[$i]; - $linkSpanContext = API\SpanContext::create( - $spanLink->traceId, - $spanLink->spanId, - API\TraceFlags::DEFAULT, - new API\TraceState($spanLink->traceState ?? null), - ); - - $this->links[] = new Link($linkSpanContext, Attributes::create($spanLink->attributes ?? [])); - $this->totalRecordedLinks++; + $otel = []; + foreach ($datadogSpanLinks as $datadogSpanLink) { + // Check if the link relationship exists + $link = ObjectKVStore::get($datadogSpanLink, $this->uniqueIdentifier); + if ($link === null) { + // Create the link + $link = new Link( + API\SpanContext::create( + $datadogSpanLink->traceId, + $datadogSpanLink->spanId, + $this->context->getTraceFlags(), + new API\TraceState($datadogSpanLink->traceState ?? null) + ), + Attributes::create($datadogSpanLink->attributes ?? []) + ); + + // Save the link + ObjectKVStore::put($datadogSpanLink, $this->uniqueIdentifier, $link); + $otel[] = $link; + } else { + // Save the link + $otel[] = $link; + } } + + // Update the links + $this->links = $otel; + $this->totalRecordedLinks = count($otel); } } diff --git a/src/DDTrace/OpenTelemetry/SpanBuilder.php b/src/DDTrace/OpenTelemetry/SpanBuilder.php index 66f7db119a..2d4f85c41b 100644 --- a/src/DDTrace/OpenTelemetry/SpanBuilder.php +++ b/src/DDTrace/OpenTelemetry/SpanBuilder.php @@ -75,10 +75,6 @@ public function addLink(SpanContextInterface $context, iterable $attributes = [] $this->totalNumberOfLinksAdded++; - if (count($this->links) === $this->tracerSharedState->getSpanLimits()->getLinkCountLimit()) { - return $this; - } - $this->links[] = new Link( $context, $this->tracerSharedState diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php index 8ea065c725..e6a0e30c5d 100644 --- a/tests/OpenTelemetry/Integration/InteroperabilityTest.php +++ b/tests/OpenTelemetry/Integration/InteroperabilityTest.php @@ -1030,4 +1030,96 @@ public function testSpanLinksInteroperabilityBothTypes() $this->assertCount(1, $traces[0]); $this->assertSame("[{\"trace_id\":\"12345678876543211234567887654321\",\"span_id\":\"8765432112345678\",\"trace_state\":\"dd=t.dm:-0\",\"attributes\":{\"arg1\":\"value1\"},\"dropped_attributes_count\":0},{\"trace_id\":\"ff0000000000051791e0000000000041\",\"span_id\":\"ff00000000000517\"}]", $traces[0][0]['meta']['_dd.span_links']); } + + public function testSpanLinksInteroperabilityRemoval() + { + $sampledSpanContext = SpanContext::create( + '12345678876543211234567887654321', + '8765432112345678', + TraceFlags::SAMPLED, + new TraceState('dd=t.dm:-0') + ); + + $traces = $this->isolateTracer(function () use ($sampledSpanContext) { + // Add 1 span link using the OTel API + $otelSpan = self::getTracer()->spanBuilder("otel.span") + ->addLink($sampledSpanContext, ['arg1' => 'value1']) + ->startSpan(); + + // Remove the span link using the DD API + unset(active_span()->links[0]); + + // Add a span link using the DD API + $newSpanLink = new SpanLink(); + $newSpanLink->traceId = "ff0000000000051791e0000000000041"; + $newSpanLink->spanId = "ff00000000000517"; + $newSpanLink->traceState = "dd=t.dm:-1"; + $newSpanLink->attributes = [ + 'arg3' => 'value3', + 'arg4' => 'value4', + ]; + active_span()->links[] = $newSpanLink; + + // Verify that there is only 1 span link from OTel's POV + $otelSpanLinks = $otelSpan->toSpanData()->getLinks(); + $this->assertCount(1, $otelSpanLinks); + $spanLinkContext = $otelSpanLinks[0]->getSpanContext(); + $this->assertSame('ff0000000000051791e0000000000041', $spanLinkContext->getTraceId()); + $this->assertSame('ff00000000000517', $spanLinkContext->getSpanId()); + $this->assertSame('dd=t.dm:-1', (string) $spanLinkContext->getTraceState()); + $this->assertSame(['arg3' => 'value3', 'arg4' => 'value4'], $otelSpanLinks[0]->getAttributes()->toArray()); + + $otelSpan->end(); + }); + + $this->assertCount(1, $traces[0]); + } + + public function testSpanLinksInteroperabilityRemovalDuplicates() + { + $traces = $this->isolateTracer(function () { + $context1 = SpanContext::create('00000000000000000000000000000001', '0000000000000001'); + $context2 = SpanContext::create('00000000000000000000000000000002', '0000000000000002'); + $context3 = SpanContext::create('00000000000000000000000000000003', '0000000000000003'); + + // Otel -> [Link(T1, S1), Link(T2, S2), Link(T2, S2), Link(T3, S3)] + $otelSpan = self::getTracer()->spanBuilder("otel.span") + ->addLink($context1) + ->addLink($context2) + ->addLink($context2) // Duplicate + ->addLink($context3) + ->startSpan(); + $initialOtelSpanLinks = $otelSpan->toSpanData()->getLinks(); + + // Modify to [Link(T1, S1), Link(T2, S2), Link(T4, S4)] + unset(active_span()->links[3]); // Remove Link(T3, S3) + unset(active_span()->links[1]); // Remove Link(T2, S2) + $newSpanLink = new SpanLink(); + $newSpanLink->traceId = "00000000000000000000000000000004"; + $newSpanLink->spanId = "0000000000000004"; + active_span()->links[] = $newSpanLink; // Add Link(T4, S4) + + // Verify the spans links from OTel's POV + $otelSpanLinks = $otelSpan->toSpanData()->getLinks(); + $this->assertCount(3, $otelSpanLinks); + + // Verify Link(T1, S1) + Instance + $spanLinkContext = $otelSpanLinks[0]->getSpanContext(); + $this->assertSame('00000000000000000000000000000001', $spanLinkContext->getTraceId()); + $this->assertSame('0000000000000001', $spanLinkContext->getSpanId()); + $this->assertSame($initialOtelSpanLinks[0], $otelSpanLinks[0]); + + // Verify Link(T2, S2) + Instance + $spanLinkContext = $otelSpanLinks[1]->getSpanContext(); + $this->assertSame('00000000000000000000000000000002', $spanLinkContext->getTraceId()); + $this->assertSame('0000000000000002', $spanLinkContext->getSpanId()); + $this->assertSame($initialOtelSpanLinks[2], $otelSpanLinks[1]); + + // Verify Link(T4, S4) + $spanLinkContext = $otelSpanLinks[2]->getSpanContext(); + $this->assertSame('00000000000000000000000000000004', $spanLinkContext->getTraceId()); + $this->assertSame('0000000000000004', $spanLinkContext->getSpanId()); + }); + + } } diff --git a/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php b/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php index 662022b56e..b849ce7e45 100644 --- a/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php +++ b/tests/OpenTelemetry/Integration/SDK/SpanBuilderTest.php @@ -125,6 +125,7 @@ public function test_add_link_invalid(): void public function test_add_link_dropping_links(): void { + $this->markTestSkipped("Irrelevant: Span Links Limits aren't supported"); $maxNumberOfLinks = 8; $tracerProvider = new TracerProvider([], null, null, (new SpanLimitsBuilder())->setLinkCountLimit($maxNumberOfLinks)->build()); $spanBuilder = $tracerProvider From 29088811da9ae992a8c5146ead501773e223b613 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Mon, 8 Jan 2024 11:08:10 +0100 Subject: [PATCH 5/7] test: Duplicate Instances Addition --- .../Integration/InteroperabilityTest.php | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php index e6a0e30c5d..e1b34b76bc 100644 --- a/tests/OpenTelemetry/Integration/InteroperabilityTest.php +++ b/tests/OpenTelemetry/Integration/InteroperabilityTest.php @@ -1120,6 +1120,38 @@ public function testSpanLinksInteroperabilityRemovalDuplicates() $this->assertSame('00000000000000000000000000000004', $spanLinkContext->getTraceId()); $this->assertSame('0000000000000004', $spanLinkContext->getSpanId()); }); + } + + public function testSpanLinksInteroperabilityAddDuplicates() + { + $traces = $this->isolateTracer(function () { + $context1 = SpanContext::create('00000000000000000000000000000001', '0000000000000001'); + $context2 = SpanContext::create('00000000000000000000000000000002', '0000000000000002'); + + $otelSpan = self::getTracer()->spanBuilder("otel.span") + ->addLink($context1) + ->addLink($context2) + ->startSpan(); + + $spanLink2 = active_span()->links[1]; + active_span()->links[] = $spanLink2; // Duplicate, same instance + + $otelSpanLinks = $otelSpan->toSpanData()->getLinks(); + + $this->assertCount(3, $otelSpanLinks); + // Verify that the duplicate is the same instance + $this->assertSame($otelSpanLinks[1], $otelSpanLinks[2]); + + // Create a new span link with the same trace id and span id + $newSpanLink = new SpanLink(); + $newSpanLink->traceId = $spanLink2->traceId; + $newSpanLink->spanId = $spanLink2->spanId; + active_span()->links[] = $newSpanLink; // Duplicate, but different instance + $otelSpanLinks = $otelSpan->toSpanData()->getLinks(); + $this->assertCount(4, $otelSpanLinks); + // Verify that the duplicate is not the same instance + $this->assertNotSame($otelSpanLinks[1], $otelSpanLinks[3]); + }); } } From 1f0e8179fbbc03497c8e1c24f7525336f7966b48 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Tue, 9 Jan 2024 12:24:13 +0100 Subject: [PATCH 6/7] fix: Prevent creation of useless instances --- src/DDTrace/OpenTelemetry/Span.php | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index 0f44f254ce..e43bb713eb 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -93,8 +93,6 @@ private function __construct( $span->name = $this->operationNameConvention = Convention::defaultOperationName($span); } - $this->uniqueIdentifier = \spl_object_hash($this); // traceId + spanId is NOT a valid unique identifier, as the traceId can be modified - // Set the span links if ($isRemapped) { // At initialization time (now), only set the links if the span was created using the OTel API @@ -111,7 +109,7 @@ private function __construct( $spanLink->droppedAttributesCount = 0; // Attributes limit aren't supported/meaningful in DD // Save the link - ObjectKVStore::put($spanLink, $this->uniqueIdentifier, $link); + ObjectKVStore::put($spanLink, "link", $link); $span->links[] = $spanLink; } } @@ -453,7 +451,7 @@ private function updateSpanLinks() $otel = []; foreach ($datadogSpanLinks as $datadogSpanLink) { // Check if the link relationship exists - $link = ObjectKVStore::get($datadogSpanLink, $this->uniqueIdentifier); + $link = ObjectKVStore::get($datadogSpanLink, "link"); if ($link === null) { // Create the link $link = new Link( @@ -467,12 +465,9 @@ private function updateSpanLinks() ); // Save the link - ObjectKVStore::put($datadogSpanLink, $this->uniqueIdentifier, $link); - $otel[] = $link; - } else { - // Save the link - $otel[] = $link; + ObjectKVStore::put($datadogSpanLink, "link", $link); } + $otel[] = $link; } // Update the links From a2a481577147103ac5c691a2c73f7e7d63c5bb45 Mon Sep 17 00:00:00 2001 From: Alexandre Choura <42672104+PROFeNoM@users.noreply.github.com> Date: Tue, 9 Jan 2024 13:04:53 +0100 Subject: [PATCH 7/7] Remove unused instance variable Co-authored-by: Bob Weinand --- src/DDTrace/OpenTelemetry/Span.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index e43bb713eb..9f1b5d8461 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -59,8 +59,6 @@ final class Span extends API\Span implements ReadWriteSpanInterface private string $operationNameConvention = ""; - private string $uniqueIdentifier = ""; - private function __construct( SpanData $span, API\SpanContextInterface $context,