w3c phase 2: add last parent_id to tracestate (#2549)

* app p to php * Finish implementation of _dd.parent_id Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * ignore tracestate in field in snapshots * fix tests * handle errors * fix testGetSpanContextWithRemoteParent * Take the proper span context into account for OTel tracecontext generation Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> --------- Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> Co-authored-by: Bob Weinand <bob.weinand@datadoghq.com>
DataDog · Mar 19, 2024 · 4c0dcf6 · 4c0dcf6
1 parent 7c36868
commit 4c0dcf6
Show file tree

Hide file tree

Showing 12 changed files with 72 additions and 29 deletions.
diff --git a/docker-compose.yml b/docker-compose.yml
@@ -223,6 +223,7 @@ services:
       - SNAPSHOTS_DIR=/snapshots
       - SNAPSHOT_CI=0
       - SNAPSHOT_REMOVED_ATTRS=start,duration,metrics.php.compilation.total_time_ms,metrics.process_id
+      - SNAPSHOT_IGNORED_ATTRS=meta._dd.parent_id,meta.tracestate
       - ENABLED_CHECKS=trace_stall,trace_peer_service,trace_dd_service
 
 

diff --git a/ext/ddtrace.c b/ext/ddtrace.c
@@ -642,7 +642,7 @@ ZEND_METHOD(DDTrace_SpanLink, fromHeaders) {
     zend_hash_copy(Z_ARR(link->property_attributes), &result.meta_tags, NULL);
 
     zend_string *propagated_tags = ddtrace_format_propagated_tags(&result.propagated_tags, &result.meta_tags);
-    zend_string *full_tracestate = ddtrace_format_tracestate(result.tracestate, result.origin, result.priority_sampling, propagated_tags, &result.tracestate_unknown_dd_keys);
+    zend_string *full_tracestate = ddtrace_format_tracestate(result.tracestate, 0, result.origin, result.priority_sampling, propagated_tags, &result.tracestate_unknown_dd_keys);
     zend_string_release(propagated_tags);
     if (full_tracestate) {
         ZVAL_STR(&link->property_trace_state, full_tracestate);

diff --git a/ext/distributed_tracing_headers.c b/ext/distributed_tracing_headers.c
@@ -227,7 +227,9 @@ static ddtrace_distributed_tracing_result ddtrace_read_distributed_tracing_ids_t
         result.parent_id = parent_id;
         result.priority_sampling = (tracedata->trace_flags[1] & 1) == (tracedata->trace_flags[1] <= '9'); // ('a' & 1) == 1
 
-        // header format: "[*,]dd=s:1;o:rum;t.dm:-4;t.usr.id:12345[,*]"
+        zend_string *span_parent_key = zend_string_init("_dd.parent_id", strlen("_dd.parent_id"), 0);
+
+        // header format: "[*,]dd=p:0000000000000111;s:1;o:rum;t.dm:-4;t.usr.id:12345[,*]"
         if (read_header((zai_str)ZAI_STRL("TRACESTATE"), "tracestate", &tracestate, data)) {
             bool last_comma = true;
             result.tracestate = zend_string_alloc(ZSTR_LEN(tracestate), 0);
@@ -260,7 +262,13 @@ static ddtrace_distributed_tracing_result ddtrace_read_distributed_tracing_ids_t
                         }
                         size_t valuelen = valueend - valuestart;
 
-                        if (keylen == 1 && keystart[0] == 's') {
+                        if (keylen == 1 && keystart[0] == 'p') {
+                            zval zv;
+                            ZVAL_STRINGL(&zv, valuestart, valuelen);
+                            zend_hash_update(&result.meta_tags, span_parent_key, &zv);
+                            zend_string_release(span_parent_key);
+                            span_parent_key = NULL;
+                        } else if (keylen == 1 && keystart[0] == 's') {
                             int extraced_priority = strtol(valuestart, NULL, 10);
                             if ((result.priority_sampling > 0) == (extraced_priority > 0)) {
                                 result.priority_sampling = extraced_priority;
@@ -316,6 +324,13 @@ static ddtrace_distributed_tracing_result ddtrace_read_distributed_tracing_ids_t
             zend_string_release(tracestate);
         }
 
+        if (span_parent_key) {
+            zval zv;
+            ZVAL_STRING(&zv, "0000000000000000");
+            zend_hash_update(&result.meta_tags, span_parent_key, &zv);
+            zend_string_release(span_parent_key);
+        }
+
         dd_check_tid(&result);
     }
 

diff --git a/ext/handlers_http.h b/ext/handlers_http.h
@@ -7,10 +7,17 @@
 
 ZEND_EXTERN_MODULE_GLOBALS(ddtrace);
 
-static inline zend_string *ddtrace_format_tracestate(zend_string *tracestate, zend_string *origin, zend_long sampling_priority, zend_string *propagated_tags, zend_array *tracestate_unknown_dd_keys) {
+static inline zend_string *ddtrace_format_tracestate(zend_string *tracestate, uint64_t span_id, zend_string *origin, zend_long sampling_priority, zend_string *propagated_tags, zend_array *tracestate_unknown_dd_keys) {
     smart_str str = {0};
 
+    if (span_id) {
+        smart_str_append_printf(&str, "p:%016" PRIx64, span_id);
+    }
+
     if (origin) {
+        if (str.s) {
+            smart_str_appendc(&str, ';');
+        }
         smart_str_appends(&str, "o:");
         signed char *cur = (signed char *)ZSTR_VAL(str.s) + ZSTR_LEN(str.s);
         smart_str_append(&str, origin);
@@ -178,7 +185,15 @@ static inline void ddtrace_inject_distributed_headers_config(zend_array *array,
                            span_id,
                            sampling_priority > 0);
 
-                zend_string *full_tracestate = ddtrace_format_tracestate(tracestate, origin, sampling_priority, propagated_tags, tracestate_unknown_dd_keys);
+                uint64_t propagated_span_id = 0;
+                zval *old_parent_id;
+                if (root) {
+                    propagated_span_id = span_id;
+                } else if ((old_parent_id = zend_hash_str_find(&DDTRACE_G(root_span_tags_preset), ZEND_STRL("_dd.parent_id")))) {
+                    propagated_span_id = ddtrace_parse_hex_span_id(old_parent_id);
+                }
+
+                zend_string *full_tracestate = ddtrace_format_tracestate(tracestate, propagated_span_id, origin, sampling_priority, propagated_tags, tracestate_unknown_dd_keys);
                 if (full_tracestate) {
                     ADD_HEADER("tracestate", "%.*s", (int)ZSTR_LEN(full_tracestate), ZSTR_VAL(full_tracestate));
                     zend_string_release(full_tracestate);

diff --git a/src/DDTrace/OpenTelemetry/SpanContext.php b/src/DDTrace/OpenTelemetry/SpanContext.php
@@ -74,7 +74,14 @@ public function getSpanIdBinary(): string
 
     public function getTraceState(): ?TraceStateInterface
     {
-        $traceContext = generate_distributed_tracing_headers(['tracecontext']);
+        $current = \DDTrace\active_stack();
+        if ($current !== $this->span->stack) {
+            \DDTrace\switch_stack($this->span);
+            $traceContext = generate_distributed_tracing_headers(['tracecontext']);
+            \DDTrace\switch_stack($current);
+        } else {
+            $traceContext = generate_distributed_tracing_headers(['tracecontext']);
+        }
         return new TraceState($traceContext['tracestate'] ?? null);
     }
 

diff --git a/tests/OpenTelemetry/Integration/API/TracerTest.php b/tests/OpenTelemetry/Integration/API/TracerTest.php
@@ -543,9 +543,9 @@ public function testGetSpanContextWithMultipleTraceStates()
                 '_dd.p.congo' => 't61rcWkgMzE',
                 '_dd.p.some_val' => 'tehehe'
             ]);
-            $this->assertRegularExpression('/^dd=t.dm:-0;t.congo:t61rcWkgMzE;t.some_val:tehehe$/', (string)$span->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.congo:t61rcWkgMzE;t.some_val:tehehe;t.dm:-0$/', (string)$span->getContext()->getTraceState());
             $span->end();
-            $this->assertRegularExpression('/^dd=t.dm:-0;t.congo:t61rcWkgMzE;t.some_val:tehehe$/', (string)$span->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.congo:t61rcWkgMzE;t.some_val:tehehe;t.dm:-0$/', (string)$span->getContext()->getTraceState());
         });
 
         $span = $traces[0][0];
@@ -589,7 +589,7 @@ public function testGetSpanContextWithRemoteParent(int $traceFlags, ?TraceState
             $this->assertSame($remoteContext->getSpanId(), $child->getParentContext()->getSpanId());
             $this->assertFalse($childContext->isRemote()); // "When creating children from remote spans, their IsRemote flag MUST be set to false."
             $this->assertEquals(1, $childContext->getTraceFlags()); // RECORD_AND_SAMPLED ==> 01 (AlwaysOn sampler)
-            $this->assertSame("dd=t.dm:-0" . ($traceState ? ",$traceState" : ""), (string)$childContext->getTraceState());
+            $this->assertSame("dd=p:" . $child->getContext()->getSpanID() . ";t.dm:-0" . ($traceState ? ",$traceState" : ""), (string)$childContext->getTraceState());
         });
 
         $span = $traces[0][0];
@@ -664,11 +664,11 @@ public function getDescription(): string
             )))->getTracer('OpenTelemetry.TracerTest');
             $parent = $tracer->spanBuilder("parent")->startSpan(); // root sampler will be used
             $scope = $parent->activate();
-            $this->assertRegularExpression('/^dd=t.dm:-0,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
             $parent->setAttributes([
                 '_dd.p.some_val' => 'tehehe'
             ]);
-            $this->assertRegularExpression('/^dd=t.dm:-0;t.some_val:tehehe,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0;t.some_val:tehehe,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
             try {
                 $child = $tracer->spanBuilder("child")->startSpan(); // local parent sampler will be used
 
@@ -677,8 +677,8 @@ public function getDescription(): string
                 $this->assertSame($parent->getContext()->getSpanId(), $child->getParentContext()->getSpanId());
                 $this->assertFalse($childContext->isRemote()); // "When creating children from remote spans, their IsRemote flag MUST be set to false."
                 $this->assertEquals(1, $childContext->getTraceFlags()); // RECORD_AND_SAMPLED ==> 01 (AlwaysOn sampler)
-                $this->assertRegularExpression('/^dd=t.dm:-0,localparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$childContext->getTraceState());
-                $this->assertRegularExpression('/^dd=t.dm:-0,localparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
+                $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0,localparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$childContext->getTraceState());
+                $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0;t.some_val:tehehe,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
 
                 $grandChild = $tracer->spanBuilder("grandChild")
                     ->setParent(Context::getCurrent()->withContextValue($child))
@@ -690,14 +690,14 @@ public function getDescription(): string
                 $this->assertSame($child->getContext()->getSpanId(), $grandChild->getParentContext()->getSpanId());
                 $this->assertFalse($grandChildContext->isRemote()); // "When creating children from remote spans, their IsRemote flag MUST be set to false."
                 $this->assertEquals(1, $grandChildContext->getTraceFlags()); // RECORD_AND_SAMPLED ==> 01 (AlwaysOn sampler)
-                $this->assertRegularExpression('/^dd=t.dm:-0,localparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$grandChildContext->getTraceState());
+                $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0,localparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$grandChildContext->getTraceState());
 
                 $grandChildScope->detach();
                 $grandChild->end();
 
                 $child->end();
             } finally {
-                $this->assertRegularExpression('/^dd=t.dm:-0;t.some_val:tehehe,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
+                $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.dm:-0;t.some_val:tehehe,root=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE$/', (string)$parent->getContext()->getTraceState());
                 $scope->detach();
                 $parent->end();
             }
@@ -806,22 +806,25 @@ public function getDescription(): string
             $this->assertSame($remoteContext->getSpanId(), $child->getParentContext()->getSpanId());
             $this->assertFalse($childContext->isRemote()); // "When creating children from remote spans, their IsRemote flag MUST be set to false."
             $this->assertEquals(1, $childContext->getTraceFlags()); // RECORD_AND_SAMPLED ==> 01 (AlwaysOn sampler)
-            //$this->assertSame("dd=t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE", (string)$childContext->getTraceState());
+            //$this->assertSame("dd=p:[0-9a-f]{16};t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE", (string)$childContext->getTraceState());
 
             $tracer = self::getTracer();
             $grandChild = $tracer->spanBuilder("grandChild")
                 ->setParent(Context::getCurrent()->withContextValue($child))
                 ->startSpan();
-            $this->assertSame("dd=t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE", (string)$child->getContext()->getTraceState());
+            $expected_tracestate = "dd=p:" . $childContext->getSpanId() . ";t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE";
+            $this->assertSame($expected_tracestate, (string)$child->getContext()->getTraceState());
             $grandChildScope = $grandChild->activate();
 
             $grandChildContext = $grandChild->getContext();
             $this->assertSame($remoteContext->getTraceId(), $grandChildContext->getTraceId());
             $this->assertSame($child->getContext()->getSpanId(), $grandChild->getParentContext()->getSpanId());
             $this->assertFalse($grandChildContext->isRemote()); // "When creating children from remote spans, their IsRemote flag MUST be set to false."
             $this->assertEquals(1, $grandChildContext->getTraceFlags()); // RECORD_AND_SAMPLED ==> 01 (AlwaysOn sampler)
-            $this->assertSame("dd=t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE", (string)$child->getContext()->getTraceState());
-            $this->assertSame("dd=t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE", (string)$grandChildContext->getTraceState());
+            $expected_tracestate = "dd=p:" . $childContext->getSpanId() . ";t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE";
+            $this->assertSame($expected_tracestate, (string)$child->getContext()->getTraceState());
+            $expected_tracestate = "dd=p:" . $grandChildContext->getSpanId(). ";t.dm:-0,remoteparent=yes,rojo=00f067aa0ba902b7,congo=t61rcWkgMzE";
+            $this->assertSame($expected_tracestate, (string)$grandChildContext->getTraceState());
 
             $grandChildScope->detach();
             $grandChild->end();
@@ -853,7 +856,7 @@ public function testAddItemToTracestate()
                 '_dd.p.congo' => 't61rcWkgMzE',
             ]);
 
-            $this->assertRegularExpression('/^dd=t.dm:-0;t.congo:t61rcWkgMzE$/', (string)$span->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.congo:t61rcWkgMzE;t.dm:-0$/', (string)$span->getContext()->getTraceState());
 
             $traceState = $span->getContext()->getTraceState()->with('rojo', '00f067aa0ba902b7');
             $context = SpanContext::create(
@@ -867,7 +870,7 @@ public function testAddItemToTracestate()
                 ->setParent(Context::getCurrent()->withContextValue(Span::wrap($context)))
                 ->startSpan();
 
-            $this->assertRegularExpression('/^dd=t.dm:-0;t.congo:t61rcWkgMzE,rojo=00f067aa0ba902b7$/', (string)$child->getContext()->getTraceState());
+            $this->assertRegularExpression('/^dd=p:[0-9a-f]{16};t.congo:t61rcWkgMzE;t.dm:-0,rojo=00f067aa0ba902b7$/', (string)$child->getContext()->getTraceState());
 
             $child->end();
             $span->end();

diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php
@@ -646,7 +646,7 @@ public function testW3CInteroperability()
             $OTelRootSpan->end();
 
             $this->assertSame("00-ff0000000000051791e0000000000041-$DDChildSpanId-01", $carrier[TraceContextPropagator::TRACEPARENT]);
-            $this->assertSame('dd=t.dm:-0', $carrier[TraceContextPropagator::TRACESTATE]); // ff00000000000517 is the high 64-bit part of the 128-bit trace id
+            $this->assertSame("dd=p:$DDChildSpanId;t.dm:-0", $carrier[TraceContextPropagator::TRACESTATE]); // ff00000000000517 is the high 64-bit part of the 128-bit trace id
         });
 
         $this->assertSame('10511401530282737729', $traces[0][0]['trace_id']);

diff --git a/tests/OpenTelemetry/Integration/SDK/TracerTest.php b/tests/OpenTelemetry/Integration/SDK/TracerTest.php
@@ -79,7 +79,7 @@ public function test_sampler_may_override_parents_trace_state(): void
             $span = $tracer->spanBuilder('test.span')->setParent($parentContext)->startSpan();
 
             $this->assertNotEquals($parentTraceState, $span->getContext()->getTraceState());
-            $this->assertEquals('dd=t.dm:-0,new-key=new_value', (string)$span->getContext()->getTraceState());
+            $this->assertEquals("dd=p:" . $span->getContext()->getSpanID() . ";t.dm:-0,new-key=new_value", (string)$span->getContext()->getTraceState());
         });
     }
 

diff --git a/tests/ext/distributed_tracestate_consumption.phpt b/tests/ext/distributed_tracestate_consumption.phpt
@@ -7,7 +7,7 @@ DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED=1
 <?php
 
 // This is sampled, hence the mechanism must be retained
-$rawTracestate = 'rojo=00f067aa0ba902b7,dd=t.dm:-1;t.congo:t61rcWkgMzE';
+$rawTracestate = 'rojo=00f067aa0ba902b7,dd=t.dm:-1;p:0123456789abcdef;t.congo:t61rcWkgMzE';
 
 $span = \DDTrace\start_span();
 
@@ -22,12 +22,14 @@ $traceParent = "00-$traceId-$parentId-$traceFlags";
 ]);
 
 var_dump(\DDTrace\generate_distributed_tracing_headers(['tracecontext']));
+var_dump(\DDTrace\root_span()->meta["_dd.parent_id"]);
 
 ?>
 --EXPECTF--
 array(2) {
   ["traceparent"]=>
   string(55) "00-%sc151df7d6ee5e2d6-a3978fb9b92502a8-01"
   ["tracestate"]=>
-  string(52) "dd=t.dm:-1;t.congo:t61rcWkgMzE,rojo=00f067aa0ba902b7"
+  string(71) "dd=p:a3978fb9b92502a8;t.dm:-1;t.congo:t61rcWkgMzE,rojo=00f067aa0ba902b7"
 }
+string(16) "0123456789abcdef"
diff --git a/tests/ext/integrations/curl/distributed_tracing_curl.phpt b/tests/ext/integrations/curl/distributed_tracing_curl.phpt
@@ -57,7 +57,7 @@ echo 'Done.' . PHP_EOL;
 [ddtrace] [error] The to be propagated tag '_dd.p.very=looooooooooooooooong' is too long and exceeds the maximum limit of 25 characters and is thus dropped.
 b3: %s-%s-1
 traceparent: 00-%s-%s
-tracestate: dd=o:phpt-test
+tracestate: dd=p:%s;o:phpt-test
 x-b3-spanid: %s
 x-b3-traceid: %s
 x-datadog-origin: phpt-test

diff --git a/tests/ext/integrations/curl/distributed_tracing_curl_invalid_tags.phpt b/tests/ext/integrations/curl/distributed_tracing_curl_invalid_tags.phpt
@@ -45,7 +45,7 @@ echo 'Done.' . PHP_EOL;
 --EXPECTF--
 b3: %s248869c998246a2e-248869c998246a2e-1
 traceparent: 00-%s248869c998246a2e-248869c998246a2e-01
-tracestate: dd=o:_______~_: ;t.escaped:_~_: ;t.dm:-0
+tracestate: dd=p:248869c998246a2e;o:_______~_: ;t.escaped:_~_: ;t.dm:-0
 x-datadog-origin: ∂~,=;:
 x-datadog-tags: _dd.p.tid=%s,_dd.p.escaped=_=;: ,_dd.p.dm=-0
 bool(false)

diff --git a/tests/ext/integrations/curl/distributed_tracing_curl_tracestate.phpt b/tests/ext/integrations/curl/distributed_tracing_curl_tracestate.phpt
@@ -9,7 +9,7 @@ ddtrace.request_init_hook={PWD}/distributed_tracing_curl_inject.inc
 DD_TRACE_LOG_LEVEL=info,startup=off
 DD_TRACE_GENERATE_ROOT_SPAN=0
 HTTP_TRACEPARENT=00-12345678901234567890123456789012-6543210987654321-01
-HTTP_TRACESTATE=foo=bar:;=,dd=o:phpt-test;unknown1:val;t.test:qvalue;s:2;unknown2:1,baz=qux
+HTTP_TRACESTATE=foo=bar:;=,dd=o:phpt-test;p:0123456789abcdef;unknown1:val;t.test:qvalue;s:2;unknown2:1,baz=qux
 DD_PROPAGATION_STYLE_INJECT=B3 single header,tracecontext
 --FILE--
 <?php
@@ -45,7 +45,7 @@ echo 'Done.' . PHP_EOL;
 --EXPECTF--
 b3: 12345678901234567890123456789012-6543210987654321-d
 traceparent: 00-12345678901234567890123456789012-6543210987654321-01
-tracestate: dd=o:phpt-test;s:2;t.test:qvalue;t.dm:-0;unknown1:val;unknown2:1,foo=bar:;=,baz=qux
+tracestate: dd=p:0123456789abcdef;o:phpt-test;s:2;t.test:qvalue;t.dm:-0;unknown1:val;unknown2:1,foo=bar:;=,baz=qux
 x-datadog-origin: phpt-test
 x-datadog-tags: _dd.p.tid=1234567890123456,_dd.p.test=qvalue,_dd.p.dm=-0
 bool(false)