From bcf201b45e234d24a895531fd807587b89077c48 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Tue, 5 Dec 2023 14:38:51 +0100 Subject: [PATCH 1/6] Merge Master --- ext/ddtrace.stub.php | 7 +- ext/ddtrace_arginfo.h | 16 ++++- ext/span.c | 22 ++++++ ext/span.h | 1 + src/DDTrace/OpenTelemetry/Context.php | 3 +- src/DDTrace/OpenTelemetry/Span.php | 3 + src/DDTrace/OpenTelemetry/SpanBuilder.php | 11 ++- src/DDTrace/OpenTelemetry/SpanContext.php | 30 ++++----- .../Integration/InteroperabilityTest.php | 67 +++++++++++++++++++ 9 files changed, 137 insertions(+), 23 deletions(-) diff --git a/ext/ddtrace.stub.php b/ext/ddtrace.stub.php index 01b33fd366..e6610a091f 100644 --- a/ext/ddtrace.stub.php +++ b/ext/ddtrace.stub.php @@ -120,6 +120,11 @@ class SpanData { */ public array $peerServiceSources = []; + /** + * @var \Closure|null $endCallback A callback to be called when the span is finished, if any. + */ + public \Closure|null $endCallback = null; + /** * @var SpanData|null The parent span, or 'null' if there is none */ @@ -424,7 +429,7 @@ function update_span_duration(SpanData $span, float $finishTime = 0): null {} * @param float $startTime Start time of the span in seconds. * @return SpanData The newly created root span */ - function start_trace_span(float $startTime = 0): SpanData {} + function start_trace_span(float $startTime = 0): RootSpanData {} /** * Get the active stack diff --git a/ext/ddtrace_arginfo.h b/ext/ddtrace_arginfo.h index 0d0ed49079..4d234f1a2e 100644 --- a/ext/ddtrace_arginfo.h +++ b/ext/ddtrace_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 1d07ca443c39ea7a0a831b062bb0efe4baf69b0f */ + * Stub hash: d95d154ba605844f46f82c7ed9df0298bf5a0872 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_trace_method, 0, 3, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, className, IS_STRING, 0) @@ -80,6 +80,11 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_set_priority_sampling, 0 ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, global, _IS_BOOL, 0, "false") ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_set_end_callback, 0, 2, IS_VOID, 0) + ZEND_ARG_OBJ_INFO(0, span, DDTrace\\SpanData, 0) + ZEND_ARG_OBJ_INFO(0, closure, Closure, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_get_priority_sampling, 0, 0, IS_LONG, 1) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, global, _IS_BOOL, 0, "false") ZEND_END_ARG_INFO() @@ -285,6 +290,7 @@ ZEND_FUNCTION(DDTrace_active_stack); ZEND_FUNCTION(DDTrace_create_stack); ZEND_FUNCTION(DDTrace_switch_stack); ZEND_FUNCTION(DDTrace_set_priority_sampling); +ZEND_FUNCTION(DDTrace_set_end_callback); ZEND_FUNCTION(DDTrace_get_priority_sampling); ZEND_FUNCTION(DDTrace_get_sanitized_exception_trace); ZEND_FUNCTION(DDTrace_consume_distributed_tracing_headers); @@ -364,6 +370,7 @@ static const zend_function_entry ext_functions[] = { ZEND_NS_FALIAS("DDTrace", create_stack, DDTrace_create_stack, arginfo_DDTrace_create_stack) ZEND_NS_FALIAS("DDTrace", switch_stack, DDTrace_switch_stack, arginfo_DDTrace_switch_stack) ZEND_NS_FALIAS("DDTrace", set_priority_sampling, DDTrace_set_priority_sampling, arginfo_DDTrace_set_priority_sampling) + ZEND_NS_FALIAS("DDTrace", set_end_callback, DDTrace_set_end_callback, arginfo_DDTrace_set_end_callback) ZEND_NS_FALIAS("DDTrace", get_priority_sampling, DDTrace_get_priority_sampling, arginfo_DDTrace_get_priority_sampling) ZEND_NS_FALIAS("DDTrace", get_sanitized_exception_trace, DDTrace_get_sanitized_exception_trace, arginfo_DDTrace_get_sanitized_exception_trace) ZEND_NS_FALIAS("DDTrace", consume_distributed_tracing_headers, DDTrace_consume_distributed_tracing_headers, arginfo_DDTrace_consume_distributed_tracing_headers) @@ -569,6 +576,13 @@ static zend_class_entry *register_class_DDTrace_SpanData(void) zend_declare_typed_property(class_entry, property_peerServiceSources_name, &property_peerServiceSources_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_MASK(MAY_BE_ARRAY)); zend_string_release(property_peerServiceSources_name); + zval property_endCallback_default_value; + ZVAL_NULL(&property_endCallback_default_value); + zend_string *property_endCallback_name = zend_string_init("endCallback", sizeof("endCallback") - 1, 1); + zend_string *property_endCallback_class_Closure = zend_string_init("Closure", sizeof("Closure")-1, 1); + zend_declare_typed_property(class_entry, property_endCallback_name, &property_endCallback_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_CLASS(property_endCallback_class_Closure, 0, MAY_BE_NULL)); + zend_string_release(property_endCallback_name); + zval property_parent_default_value; ZVAL_UNDEF(&property_parent_default_value); zend_string *property_parent_name = zend_string_init("parent", sizeof("parent") - 1, 1); diff --git a/ext/span.c b/ext/span.c index c419033f53..0da92e60dc 100644 --- a/ext/span.c +++ b/ext/span.c @@ -411,6 +411,28 @@ bool ddtrace_span_alter_root_span_config(zval *old_value, zval *new_value) { } void dd_trace_stop_span_time(ddtrace_span_data *span) { + zval *end_closure_zv = &span->property_end_closure; + + if (end_closure_zv + && Z_TYPE_P(end_closure_zv) == IS_OBJECT + && Z_OBJCE_P(end_closure_zv) == zend_ce_closure) { + zval rv; + zval span_zv; + ZVAL_OBJ(&span_zv, &span->std); + bool success = zai_symbol_call( + ZAI_SYMBOL_SCOPE_GLOBAL, + NULL, + ZAI_SYMBOL_FUNCTION_CLOSURE, + end_closure_zv, + &rv, + 1, + &span_zv + ); + if (!success) { + LOG(Warn, "Unable to call end closure"); + } + } + span->duration = _get_nanoseconds(USE_MONOTONIC_CLOCK) - span->duration_start; } diff --git a/ext/span.h b/ext/span.h index 3f53d56463..0e75c334c8 100644 --- a/ext/span.h +++ b/ext/span.h @@ -50,6 +50,7 @@ typedef union ddtrace_span_properties { }; zval property_links; zval property_peer_service_sources; + zval property_end_closure; union { union ddtrace_span_properties *parent; zval property_parent; diff --git a/src/DDTrace/OpenTelemetry/Context.php b/src/DDTrace/OpenTelemetry/Context.php index 0e28b8486d..0f07c638ce 100644 --- a/src/DDTrace/OpenTelemetry/Context.php +++ b/src/DDTrace/OpenTelemetry/Context.php @@ -4,6 +4,7 @@ namespace OpenTelemetry\Context; +use DDTrace\OpenTelemetry\API\Trace as DDTraceAPI; use DDTrace\SpanData; use DDTrace\Tag; use DDTrace\Util\ObjectKVStore; @@ -191,7 +192,7 @@ private static function activateParent(?SpanData $currentSpan): ContextInterface $OTelCurrentSpan = SDK\Span::startSpan( $currentSpan, - API\SpanContext::create($currentTraceId, $currentSpanId, $traceFlags, $traceState), // $context + DDTraceAPI\SpanContext::createFromLocalSpan($currentSpan, $traceFlags === API\TraceFlags::SAMPLED, $currentSpanId), self::getDDInstrumentationScope(), // $instrumentationScope isset($currentSpan->meta[Tag::SPAN_KIND]) ? self::convertDDSpanKindToOtel($currentSpan->meta[Tag::SPAN_KIND]) : API\SpanKind::KIND_INTERNAL, // $kind API\Span::fromContext($parentContext), // $parentSpan (TODO: Handle null parent span) ? diff --git a/src/DDTrace/OpenTelemetry/Span.php b/src/DDTrace/OpenTelemetry/Span.php index 9f1b5d8461..8a5b7c5945 100644 --- a/src/DDTrace/OpenTelemetry/Span.php +++ b/src/DDTrace/OpenTelemetry/Span.php @@ -111,6 +111,8 @@ private function __construct( $span->links[] = $spanLink; } } + + $span->endCallback = fn () => $this->endOTelSpan(); } /** @@ -401,6 +403,7 @@ public function end(int $endEpochNanos = null): void return; } + $this->span->endCallback = null; // No need for the callback $this->endOTelSpan($endEpochNanos); switch_stack($this->span); diff --git a/src/DDTrace/OpenTelemetry/SpanBuilder.php b/src/DDTrace/OpenTelemetry/SpanBuilder.php index 2d4f85c41b..992d95f8a2 100644 --- a/src/DDTrace/OpenTelemetry/SpanBuilder.php +++ b/src/DDTrace/OpenTelemetry/SpanBuilder.php @@ -138,8 +138,13 @@ public function startSpan(): SpanInterface $parentSpan = Span::fromContext($parentContext); $parentSpanContext = $parentSpan->getContext(); - $span = $parentSpanContext->isValid() ? null : \DDTrace\start_trace_span($this->startEpochNanos); - $traceId = $parentSpanContext->isValid() ? $parentSpanContext->getTraceId() : \DDTrace\root_span()->traceId; + if ($parentSpanContext->isValid()) { + $span = null; + $traceId = $parentSpanContext->getTraceId(); + } else { + $span = \DDTrace\start_trace_span($this->startEpochNanos); + $traceId = $span->traceId; + } $samplingResult = $this ->tracerSharedState @@ -174,7 +179,7 @@ public function startSpan(): SpanInterface } $hexSpanId = $span->hexId(); - $spanContext = DDTraceAPI\SpanContext::createFromLocalSpan($span, $sampled, $traceId, $hexSpanId); + $spanContext = DDTraceAPI\SpanContext::createFromLocalSpan($span, $sampled, $hexSpanId); if (!in_array($samplingDecision, [SamplingResult::RECORD_AND_SAMPLE, SamplingResult::RECORD_ONLY], true)) { return Span::wrap($spanContext); diff --git a/src/DDTrace/OpenTelemetry/SpanContext.php b/src/DDTrace/OpenTelemetry/SpanContext.php index 2baef1e215..175c2cb35b 100644 --- a/src/DDTrace/OpenTelemetry/SpanContext.php +++ b/src/DDTrace/OpenTelemetry/SpanContext.php @@ -5,6 +5,7 @@ namespace DDTrace\OpenTelemetry\API\Trace; use DDTrace\OpenTelemetry\SDK\Trace\Span; +use DDTrace\RootSpanData; use DDTrace\SpanData; use OpenTelemetry\API\Trace as API; use OpenTelemetry\API\Trace\SpanContextInterface; @@ -23,27 +24,22 @@ final class SpanContext implements SpanContextInterface private bool $remote; - private ?string $traceId; - private ?string $spanId; - private bool $isValid = true; + private static function getRootSpan(SpanData $span): RootSpanData + { + while (!($span instanceof RootSpanData) && $span->parent) { + $span = $span->parent; + } + return $span; + } - private function __construct(SpanData $span, bool $sampled, bool $remote, ?string $traceId = null, ?string $spanId = null) + private function __construct(SpanData $span, bool $sampled, bool $remote, ?string $spanId = null) { $this->span = $span; $this->sampled = $sampled; $this->remote = $remote; - $this->traceId = $traceId ?: \DDTrace\root_span()->traceId; $this->spanId = $spanId ?: $this->span->hexId(); - - // TraceId must be exactly 16 bytes (32 chars) and at least one non-zero byte - // SpanId must be exactly 8 bytes (16 chars) and at least one non-zero byte - if (!SpanContextValidator::isValidTraceId($this->traceId) || !SpanContextValidator::isValidSpanId($this->spanId)) { - $this->traceId = SpanContextValidator::INVALID_TRACE; - $this->spanId = SpanContextValidator::INVALID_SPAN; - $this->isValid = false; - } } /** @@ -51,7 +47,8 @@ private function __construct(SpanData $span, bool $sampled, bool $remote, ?strin */ public function getTraceId(): string { - return $this->traceId; + $rootSpan = self::getRootSpan($this->span); + return $rootSpan->traceId; } public function getTraceIdBinary(): string @@ -85,7 +82,7 @@ public function isSampled(): bool public function isValid(): bool { - return $this->isValid; + return SpanContextValidator::isValidTraceId(self::getRootSpan($this->span)->traceId) && SpanContextValidator::isValidSpanId($this->spanId); } public function isRemote(): bool @@ -116,13 +113,12 @@ public static function getInvalid(): SpanContextInterface return API\SpanContext::getInvalid(); } - public static function createFromLocalSpan(SpanData $span, bool $sampled, ?string $traceId = null, ?string $spanId = null) + public static function createFromLocalSpan(SpanData $span, bool $sampled, ?string $spanId = null) { return new self( $span, $sampled, false, - $traceId, $spanId ); } diff --git a/tests/OpenTelemetry/Integration/InteroperabilityTest.php b/tests/OpenTelemetry/Integration/InteroperabilityTest.php index e1b34b76bc..0a97c07f36 100644 --- a/tests/OpenTelemetry/Integration/InteroperabilityTest.php +++ b/tests/OpenTelemetry/Integration/InteroperabilityTest.php @@ -23,8 +23,11 @@ use OpenTelemetry\SDK\Trace\Sampler\AlwaysOnSampler; use OpenTelemetry\SDK\Trace\TracerProvider; use function DDTrace\active_span; +use function DDTrace\add_distributed_tag; use function DDTrace\close_span; use function DDTrace\close_spans_until; +use function DDTrace\generate_distributed_tracing_headers; +use function DDTrace\set_distributed_tracing_context; use function DDTrace\start_span; use function DDTrace\start_trace_span; @@ -910,6 +913,21 @@ public function testAttributesInteroperability() $this->assertEquals(2, $traces[0][0]['metrics']['m2']); } + public function testModifyTracestateAfterEnd() + { + $rootSpan = \DDTrace\start_span(); + + $ddSpan = start_span(); + + /** @var \OpenTelemetry\SDK\Trace\Span $remappedSpan */ + $remappedSpan = Span::getCurrent(); + + $preTraceState = generate_distributed_tracing_headers(['tracecontext'])['tracestate']; + + add_distributed_tag('key', 'value'); + $readOnlyCopy = $remappedSpan->toSpanData(); + $midTraceState = generate_distributed_tracing_headers(['tracecontext'])['tracestate']; + $remappedTraceState = (string) $readOnlyCopy->getContext()->getTraceState(); public function testSpanLinksInteroperabilityFromDatadogSpan() { $traces = $this->isolateTracer(function () { @@ -1001,6 +1019,55 @@ public function testSpanLinksInteroperabilityBothTypes() $datadogSpanLinks = active_span()->links; $this->assertCount(2, $datadogSpanLinks); + $this->assertSame($midTraceState, $remappedTraceState); + + close_span(); // Closes $ddSpan + + add_distributed_tag('my', 'tag'); // Shouldn't affect the closed span + $readOnlyCopy = $remappedSpan->toSpanData(); + $postTraceState = generate_distributed_tracing_headers(['tracecontext'])['tracestate']; + $remappedTraceState = (string) $readOnlyCopy->getContext()->getTraceState(); + + $this->assertSame($midTraceState, $remappedTraceState); + $this->assertNotSame($preTraceState, $postTraceState); + $this->assertNotSame($midTraceState, $postTraceState); + + close_span(); // Closes $rootSpan + } + + public function testModifyTraceIdAfterEnd() + { + $rootSpan = \DDTrace\start_trace_span(); + $ddSpan = start_span(); + + /** @var \OpenTelemetry\SDK\Trace\Span $remappedSpan */ + $remappedSpan = Span::getCurrent(); + + set_distributed_tracing_context("23475823097097752842117799874953798269", "42"); + + $midTraceId = $remappedSpan->getContext()->getTraceId(); + + $this->assertSame($midTraceId, $rootSpan->traceId); + $this->assertSame('11a94770f26bd5a0aafe31aee31ce27d', $midTraceId); + + close_span(); // Closes $ddSpan + + set_distributed_tracing_context("33475823097097752843117799874953798249", "42"); // Note the different trace id + + $this->assertSame('192f3581c8461c79b9d31f028a80e269', $rootSpan->traceId); + + $readOnlyCopy = $remappedSpan->toSpanData(); + + $this->assertNotSame($midTraceId, $rootSpan->traceId); + $this->assertSame('11a94770f26bd5a0aafe31aee31ce27d', $midTraceId); + + $remappedTraceId = $readOnlyCopy->getContext()->getTraceId(); + + $this->assertSame($midTraceId, $remappedTraceId); + $this->assertSame('11a94770f26bd5a0aafe31aee31ce27d', $remappedTraceId); + + close_span(); // Closes $rootSpan + } $this->assertSame('12345678876543211234567887654321', $datadogSpanLinks[0]->traceId); $this->assertSame('8765432112345678', $datadogSpanLinks[0]->spanId); $this->assertSame('dd=t.dm:-0', $datadogSpanLinks[0]->traceState); From 8afa69cf9029b90ef9fb238042b490aad614df58 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Tue, 5 Dec 2023 14:39:18 +0100 Subject: [PATCH 2/6] Add test file --- tests/ext/span_callback_basic.phpt | 62 ++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 tests/ext/span_callback_basic.phpt diff --git a/tests/ext/span_callback_basic.phpt b/tests/ext/span_callback_basic.phpt new file mode 100644 index 0000000000..2d5c6138ca --- /dev/null +++ b/tests/ext/span_callback_basic.phpt @@ -0,0 +1,62 @@ +--TEST-- +Basic end callback behavior +--ENV-- +DD_TRACE_DEBUG=1 +--FILE-- +name\n"; + echo "Passed span: $span->name\n"; + $span->meta['counter.value'] = $myGlobalClosedSpansCounter; +}; + +$span = \DDTrace\start_span(); +$span->name = "mySpan"; +//\DDTrace\set_end_callback($span, $callback); +$span->endCallback = $callback; + +var_dump(\DDTrace\current_context()); +var_dump(\DDTrace\logs_correlation_trace_id()); + +\DDTrace\close_span(); + +var_dump($myGlobalClosedSpansCounter); +var_dump(dd_trace_serialize_closed_spans()); + +class mySpanWrapper { + private $mySpan; + + private function __construct(\DDTrace\SpanData $span) { + $this->mySpan = $span; + /* + // Doesn't crash + $span->endCallback = function (\DDTrace\SpanData $span) { + $this->end(); + };*/ + } + + public static function create(\DDTrace\SpanData $span) { + $spanWrapper = new mySpanWrapper($span); + // Crash + $span->endCallback = function (\DDTrace\SpanData $span) use ($spanWrapper) { + $spanWrapper->end(); + }; + } + + public function end() { + echo "This is the end of {$this->mySpan->name}\n"; + } +} + +$span = \DDTrace\start_span(); +$span->name = "mySpan"; +mySpanWrapper::create($span); +\DDTrace\close_span(); + +?> +--EXPECT-- From 0d14b13182a973ee5a5ab47fe7131f98a98fa9d1 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Tue, 5 Dec 2023 14:48:37 +0100 Subject: [PATCH 3/6] Update stub file --- ext/ddtrace_arginfo.h | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/ext/ddtrace_arginfo.h b/ext/ddtrace_arginfo.h index 4d234f1a2e..2e2120cc1a 100644 --- a/ext/ddtrace_arginfo.h +++ b/ext/ddtrace_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: d95d154ba605844f46f82c7ed9df0298bf5a0872 */ + * Stub hash: 55972d231789add1cf24ac96d74b532cd7f112f5 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_trace_method, 0, 3, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, className, IS_STRING, 0) @@ -61,7 +61,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_update_span_duration, 0, ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, finishTime, IS_DOUBLE, 0, "0") ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_DDTrace_start_trace_span, 0, 0, DDTrace\\SpanData, 0) +ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_DDTrace_start_trace_span, 0, 0, DDTrace\\RootSpanData, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, startTime, IS_DOUBLE, 0, "0") ZEND_END_ARG_INFO() @@ -80,11 +80,6 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_set_priority_sampling, 0 ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, global, _IS_BOOL, 0, "false") ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_set_end_callback, 0, 2, IS_VOID, 0) - ZEND_ARG_OBJ_INFO(0, span, DDTrace\\SpanData, 0) - ZEND_ARG_OBJ_INFO(0, closure, Closure, 0) -ZEND_END_ARG_INFO() - ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_get_priority_sampling, 0, 0, IS_LONG, 1) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, global, _IS_BOOL, 0, "false") ZEND_END_ARG_INFO() @@ -290,7 +285,6 @@ ZEND_FUNCTION(DDTrace_active_stack); ZEND_FUNCTION(DDTrace_create_stack); ZEND_FUNCTION(DDTrace_switch_stack); ZEND_FUNCTION(DDTrace_set_priority_sampling); -ZEND_FUNCTION(DDTrace_set_end_callback); ZEND_FUNCTION(DDTrace_get_priority_sampling); ZEND_FUNCTION(DDTrace_get_sanitized_exception_trace); ZEND_FUNCTION(DDTrace_consume_distributed_tracing_headers); @@ -370,7 +364,6 @@ static const zend_function_entry ext_functions[] = { ZEND_NS_FALIAS("DDTrace", create_stack, DDTrace_create_stack, arginfo_DDTrace_create_stack) ZEND_NS_FALIAS("DDTrace", switch_stack, DDTrace_switch_stack, arginfo_DDTrace_switch_stack) ZEND_NS_FALIAS("DDTrace", set_priority_sampling, DDTrace_set_priority_sampling, arginfo_DDTrace_set_priority_sampling) - ZEND_NS_FALIAS("DDTrace", set_end_callback, DDTrace_set_end_callback, arginfo_DDTrace_set_end_callback) ZEND_NS_FALIAS("DDTrace", get_priority_sampling, DDTrace_get_priority_sampling, arginfo_DDTrace_get_priority_sampling) ZEND_NS_FALIAS("DDTrace", get_sanitized_exception_trace, DDTrace_get_sanitized_exception_trace, arginfo_DDTrace_get_sanitized_exception_trace) ZEND_NS_FALIAS("DDTrace", consume_distributed_tracing_headers, DDTrace_consume_distributed_tracing_headers, arginfo_DDTrace_consume_distributed_tracing_headers) From 426c4096d2da86e077a5fa94c5c267ae84ecf999 Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Tue, 5 Dec 2023 14:52:50 +0100 Subject: [PATCH 4/6] Add cat:opentelemetry to labeller.yml --- .github/labeller.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/labeller.yml b/.github/labeller.yml index 10f89df8c9..d49c4e6a8e 100644 --- a/.github/labeller.yml +++ b/.github/labeller.yml @@ -22,3 +22,6 @@ labels: - name: "tracing" allFilesIn: "(.*\/ext\/.*|.*\/src\/.*|.*\/tests\/.*|.*\/zend_abstract_interface\/.*|docker-compose.yml|.*\/.circleci\/.*)" + + - name: "cat:opentelemetry" + allFilesIn: "(.*\/OpenTelemetry\/.*)" From e91ed7329db25dad1f910c4fdb0593e83b1830da Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Sat, 9 Dec 2023 00:18:49 +0100 Subject: [PATCH 5/6] test: Add sample failing test --- ext/ddtrace.stub.php | 4 +-- ext/ddtrace_arginfo.h | 4 +-- ext/span.c | 52 +++++++++++++++++++---------- tests/ext/span_callback_static.phpt | 28 ++++++++++++++++ 4 files changed, 67 insertions(+), 21 deletions(-) create mode 100644 tests/ext/span_callback_static.phpt diff --git a/ext/ddtrace.stub.php b/ext/ddtrace.stub.php index e6610a091f..92192ec5ad 100644 --- a/ext/ddtrace.stub.php +++ b/ext/ddtrace.stub.php @@ -121,9 +121,9 @@ class SpanData { public array $peerServiceSources = []; /** - * @var \Closure|null $endCallback A callback to be called when the span is finished, if any. + * @var \Closure|string|null $endCallback A callback to be called when the span is finished, if any. */ - public \Closure|null $endCallback = null; + public \Closure|string|null $endCallback = null; /** * @var SpanData|null The parent span, or 'null' if there is none diff --git a/ext/ddtrace_arginfo.h b/ext/ddtrace_arginfo.h index 2e2120cc1a..38c3aaa096 100644 --- a/ext/ddtrace_arginfo.h +++ b/ext/ddtrace_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 55972d231789add1cf24ac96d74b532cd7f112f5 */ + * Stub hash: e001e68def98ce39cf3f6a11337dc109cca4868f */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_trace_method, 0, 3, _IS_BOOL, 0) ZEND_ARG_TYPE_INFO(0, className, IS_STRING, 0) @@ -573,7 +573,7 @@ static zend_class_entry *register_class_DDTrace_SpanData(void) ZVAL_NULL(&property_endCallback_default_value); zend_string *property_endCallback_name = zend_string_init("endCallback", sizeof("endCallback") - 1, 1); zend_string *property_endCallback_class_Closure = zend_string_init("Closure", sizeof("Closure")-1, 1); - zend_declare_typed_property(class_entry, property_endCallback_name, &property_endCallback_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_CLASS(property_endCallback_class_Closure, 0, MAY_BE_NULL)); + zend_declare_typed_property(class_entry, property_endCallback_name, &property_endCallback_default_value, ZEND_ACC_PUBLIC, NULL, (zend_type) ZEND_TYPE_INIT_CLASS(property_endCallback_class_Closure, 0, MAY_BE_STRING|MAY_BE_NULL)); zend_string_release(property_endCallback_name); zval property_parent_default_value; diff --git a/ext/span.c b/ext/span.c index 0da92e60dc..64c255cbf7 100644 --- a/ext/span.c +++ b/ext/span.c @@ -410,29 +410,47 @@ bool ddtrace_span_alter_root_span_config(zval *old_value, zval *new_value) { } } -void dd_trace_stop_span_time(ddtrace_span_data *span) { +bool execute_span_end_callback(ddtrace_span_data *span) { zval *end_closure_zv = &span->property_end_closure; - if (end_closure_zv - && Z_TYPE_P(end_closure_zv) == IS_OBJECT - && Z_OBJCE_P(end_closure_zv) == zend_ce_closure) { - zval rv; - zval span_zv; - ZVAL_OBJ(&span_zv, &span->std); - bool success = zai_symbol_call( - ZAI_SYMBOL_SCOPE_GLOBAL, - NULL, - ZAI_SYMBOL_FUNCTION_CLOSURE, - end_closure_zv, - &rv, - 1, - &span_zv - ); + if (end_closure_zv && !ZVAL_IS_NULL(end_closure_zv)) { + uint8_t type = Z_TYPE_P(end_closure_zv); + bool success = false; + + if (type == IS_OBJECT && Z_OBJCE_P(end_closure_zv) == zend_ce_closure) { + zval rv; + zval span_zv; + ZVAL_OBJ(&span_zv, &span->std); + + success = zai_symbol_call( + ZAI_SYMBOL_SCOPE_GLOBAL, + NULL, + ZAI_SYMBOL_FUNCTION_CLOSURE, + end_closure_zv, + &rv, + 1, + &span_zv + ); + + zval_ptr_dtor(&rv); + } // TODO: string, etc? TBD, else refactor + if (!success) { - LOG(Warn, "Unable to call end closure"); + zend_string *hex_trace_id = ddtrace_trace_id_as_hex_string(span->root->trace_id); + zend_string *hex_span_id = ddtrace_span_id_as_hex_string(span->span_id); + LOG(Warn, "Error while calling end closure on span %s [dd.trace_id=%s dd.span_id=%s]", Z_STRVAL_P(&span->property_name), ZSTR_VAL(hex_trace_id), ZSTR_VAL(hex_span_id)); + zend_string_release(hex_trace_id); + zend_string_release(hex_span_id); } + + return success; + } else { + return true; } +} +void dd_trace_stop_span_time(ddtrace_span_data *span) { + execute_span_end_callback(span); span->duration = _get_nanoseconds(USE_MONOTONIC_CLOCK) - span->duration_start; } diff --git a/tests/ext/span_callback_static.phpt b/tests/ext/span_callback_static.phpt new file mode 100644 index 0000000000..2a61872dd0 --- /dev/null +++ b/tests/ext/span_callback_static.phpt @@ -0,0 +1,28 @@ +--TEST-- +End callback using a static method +--ENV-- +DD_TRACE_DEBUG=1 +--FILE-- +endCallback = $closure; +\DDTrace\close_span(); + +var_dump($counter); + +?> +--EXPECT-- From 78fe75b06edae4ae8c69d691a9e0eb3a393394bc Mon Sep 17 00:00:00 2001 From: Alexandre Choura Date: Sat, 9 Dec 2023 01:03:10 +0100 Subject: [PATCH 6/6] (PoC) Fixes failing static & object calls --- ext/span.c | 38 ++++++++++++++++++++++-------- tests/ext/span_callback_basic.phpt | 7 ++---- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/ext/span.c b/ext/span.c index 64c255cbf7..2926149f45 100644 --- a/ext/span.c +++ b/ext/span.c @@ -421,16 +421,34 @@ bool execute_span_end_callback(ddtrace_span_data *span) { zval rv; zval span_zv; ZVAL_OBJ(&span_zv, &span->std); - - success = zai_symbol_call( - ZAI_SYMBOL_SCOPE_GLOBAL, - NULL, - ZAI_SYMBOL_FUNCTION_CLOSURE, - end_closure_zv, - &rv, - 1, - &span_zv - ); + // Get the closure's scope + zend_class_entry *closure_called_scope; + zend_function *closure_func; + zend_object *closure_this; +#if PHP_VERSION_ID < 80000 + Z_OBJ_HANDLER_P(end_closure_zv, get_closure)(end_closure_zv, &closure_called_scope, &closure_func, &closure_this); +#else + Z_OBJ_HANDLER_P(end_closure_zv, get_closure)(Z_OBJ_P(end_closure_zv), &closure_called_scope, &closure_func, &closure_this, true); +#endif + if (closure_called_scope && closure_func->common.fn_flags & ZEND_ACC_STATIC) { + success = zai_symbol_call_static( + closure_called_scope, + zai_str_from_zstr(closure_func->common.function_name), + &rv, + 1, + &span_zv + ); + } else { + success = zai_symbol_call( + closure_called_scope ? ZAI_SYMBOL_SCOPE_CLASS : ZAI_SYMBOL_SCOPE_GLOBAL, + closure_called_scope ? closure_called_scope : NULL, + ZAI_SYMBOL_FUNCTION_CLOSURE, + end_closure_zv, + &rv, + 1, + &span_zv + ); + } zval_ptr_dtor(&rv); } // TODO: string, etc? TBD, else refactor diff --git a/tests/ext/span_callback_basic.phpt b/tests/ext/span_callback_basic.phpt index 2d5c6138ca..497a0a6b50 100644 --- a/tests/ext/span_callback_basic.phpt +++ b/tests/ext/span_callback_basic.phpt @@ -12,7 +12,7 @@ $callback = function (\DDTrace\SpanData $span) use (&$myGlobalClosedSpansCounter $activeSpan = \DDTrace\active_span(); echo "$activeSpan->name\n"; echo "Passed span: $span->name\n"; - $span->meta['counter.value'] = $myGlobalClosedSpansCounter; + //$span->meta['counter.value'] = $myGlobalClosedSpansCounter; }; $span = \DDTrace\start_span(); @@ -20,13 +20,10 @@ $span->name = "mySpan"; //\DDTrace\set_end_callback($span, $callback); $span->endCallback = $callback; -var_dump(\DDTrace\current_context()); -var_dump(\DDTrace\logs_correlation_trace_id()); - \DDTrace\close_span(); var_dump($myGlobalClosedSpansCounter); -var_dump(dd_trace_serialize_closed_spans()); +//var_dump(dd_trace_serialize_closed_spans()); class mySpanWrapper { private $mySpan;