feat: API Gateway Tracing by PROFeNoM · Pull Request #3116 · DataDog/dd-trace-php

PROFeNoM · 2025-03-04T13:32:43Z

Description

Based on the following implementation doc: GDoc

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

bwoebi · 2025-03-04T13:37:35Z

    dd_start_span(INTERNAL_FUNCTION_PARAM_PASSTHRU);
 }

+/* {{{ proto InferredSpanData\null DDTrace\start_inferred_span(array $headers, SpanData $rootSpan) */


I had the suggestion of getting rid of this function and instead putting the logic into consume_distributed_tracing_headers(). Did this not work out?

Oops, forgot about it. Will try it out 👍

bwoebi · 2025-03-04T13:43:23Z

+    if (span->std.ce == ddtrace_ce_root_span_data) {
+        ddtrace_root_span_data *root = ROOTSPANDATA(&span->std);
+        zval *inferred_span_zv = &root->property_inferred_span;
+        if (Z_TYPE_P(inferred_span_zv) == IS_OBJECT) {


could you please add a condition that the inferred_span property is actually an instance of ddtrace_ce_inferred_span_data?

codecov-commenter · 2025-03-05T09:13:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 56.98%. Comparing base (760d4a6) to head (ddc3c19).
⚠️ Report is 755 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3116      +/-   ##
============================================
+ Coverage     52.26%   56.98%   +4.71%     
  Complexity     2883     2883              
============================================
  Files           114      141      +27     
  Lines         11438    15844    +4406     
  Branches          0     1094    +1094     
============================================
+ Hits           5978     9028    +3050     
- Misses         5460     6243     +783     
- Partials          0      573     +573

Flag	Coverage Δ
appsec-extension	`69.22% <ø> (?)`
tracer-php	`52.26% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.
see 27 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 760d4a6...ddc3c19. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pr-commenter · 2025-03-05T09:35:58Z

Benchmarks [ tracer ]

Benchmark execution time: 2025-03-28 19:58:05

Comparing candidate commit ddc3c19 in PR branch alex/AIDM-548_api-gateway-ter with baseline commit 760d4a6 in branch master.

Found 0 performance improvements and 7 performance regressions! Performance is the same for 171 metrics, 0 unstable metrics.

scenario:ContextPropagationBench/benchExtractHeaders128Bit

🟥 execution_time [+61.739ns; +92.927ns] or [+7.756%; +11.674%]

scenario:ContextPropagationBench/benchExtractHeaders128Bit-opcache

🟥 execution_time [+66.455ns; +80.212ns] or [+8.419%; +10.162%]

scenario:ContextPropagationBench/benchExtractTraceContext128Bit

🟥 execution_time [+67.444ns; +112.556ns] or [+3.850%; +6.424%]

scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache

🟥 execution_time [+88.300ns; +157.700ns] or [+5.031%; +8.986%]

scenario:ContextPropagationBench/benchExtractTraceContext64Bit

🟥 execution_time [+60.145ns; +89.855ns] or [+7.594%; +11.345%]

scenario:ContextPropagationBench/benchExtractTraceContext64Bit-opcache

🟥 execution_time [+50.273ns; +93.727ns] or [+6.261%; +11.672%]

scenario:TraceFlushBench/benchFlushTrace

🟥 execution_time [+1000.000ns; +1000.000ns] or [+100.000%; +100.000%]

…e_inferred_span_data

bwoebi · 2025-03-05T16:10:52Z

    })

+    if (inferred_span) {
+        ddtrace_serialize_span_to_array(inferred_span, array);


I would prefer if we could do all the transfer_data calls here (from serialized to inferred serialized), after the serializing step. But I won't insist.

bwoebi · 2025-03-07T14:35:43Z

    }

+    if (inferred_span) {
+        ddtrace_serialize_span_to_array(inferred_span, array);


You could make ddtrace_serialize_span_to_array return the inserted array instead of refetching from Z_ARR_P(array).

bwoebi · 2025-03-07T14:42:28Z


    add_next_index_zval(array, el);
+
+    return el;


noooo :-D
That returns a stack variable.

Use

return zend_hash_next_index_insert(Z_ARR_P(array), el);

(instead of add_next_index_zval)

bwoebi · 2025-03-10T11:29:10Z

+    ZVAL_OBJ(&root->property_inferred_span, &span->std);
+
+    span->span_id = ddtrace_generate_span_id();
+    ddtrace_set_global_span_properties(span);


Move this to the end of this function or e.g. DD_TAGS=stage:1 will lead to a memory leak

bwoebi · 2025-03-10T11:34:27Z

    bool is_root_span = span->std.ce == ddtrace_ce_root_span_data;
-    zval meta_zv, *meta = &span->property_meta;
+    bool is_inferred_span = span->std.ce == ddtrace_ce_inferred_span_data;
+    zval meta_zv, *meta = &span->property_meta, *inferred_span_zv = NULL;


Suggested change

zval meta_zv, *meta = &span->property_meta, *inferred_span_zv = NULL;

zval meta_zv, *meta = &span->property_meta;

ddtrace_span_data *inferred_span = NULL;

if (is_root_span) {

inferred_span = ddtrace_get_inferred_span(ROOTSPANDATA(&span->std));

}

and then if(inferred_span) ...
Let's check it consistently the same way.

…c_string

wantsui · 2025-03-17T19:13:16Z

+    "resource": "GET /test",
+    "trace_id": 0,
+    "span_id": 1,
+    "parent_id": 3978455880403641907,


Can this assert that "error": 1, for the aws api gateway span too?

That's a crazy catch. I see why.

wantsui · 2025-03-17T19:24:53Z

+
+    read_header((zai_str)ZAI_STRL("X_DD_PROXY_PATH"), "x-dd-proxy-path", &result.path, data);
+    read_header((zai_str)ZAI_STRL("X_DD_PROXY_HTTPMETHOD"), "x-dd-proxy-httpmethod", &result.http_method, data);
+    read_header((zai_str)ZAI_STRL("X_DD_PROXY_DOMAIN_NAME"), "x-dd-proxy-domain-name", &result.domain, data);


What is the value of the service if x-dd-proxy-domain-name is not provided?

(Asking because it's unclear to me from the test what happens)

If I read the code correctly, service then is never assigned and falls back to the empty string.

If there's no fallback we should try to use DD_SERVICE where possible so we don't send the span with an empty service name.

Modified to fallback to the root span's service name. I can modify it to strictly be DD_SERVICE, but then we'd have to have some special handling when altering DD_SERVICE (+The service is sometimes inferred from the app config - e.g. Laravel).

I think the new behaviour is perfectly fine.

Looping in @zarirhamza in case he has other feedback about this!

bwoebi · 2025-03-18T11:49:33Z

-        transfer_data(serialized_meta, serialized_inferred_span_meta, ZEND_STRL("error"), false);
-        transfer_data(serialized_meta, serialized_inferred_span_meta, ZEND_STRL("track_error"), false);
+        transfer_data(Z_ARR_P(el), Z_ARR_P(serialized_inferred_span), ZEND_STRL("error"), false);
+        transfer_data(Z_ARR_P(el), Z_ARR_P(serialized_inferred_span), ZEND_STRL("track_error"), false);


track_error is in meta though, right? Only array is top-level.

Is it? I looked at the following line, and considering that it is added the same way as error is, I would have assumed that it wasn't

You are right, but that's because the code adding track_error is wrong :-( Should be on meta...
Could you please fix it over there in this PR?

Ok 👍 Will do in the next commit

# Conflicts: # ext/configuration.h # ext/ddtrace_arginfo.h # tests/Frameworks/Laravel/Latest/.env # tests/Integrations/CLI/Laravel/Latest/CommonScenariosTest.php # tests/Integrations/Laravel/Latest/CommonScenariosTest.php

Signed-off-by: Bob Weinand <[email protected]>

…DM-548_api-gateway-ter

feat: InferredSpanData

3e6a4c0

PROFeNoM self-assigned this Mar 4, 2025

PROFeNoM mentioned this pull request Mar 4, 2025

feat: API Gateway Tracing #3095

Closed

2 tasks

bwoebi reviewed Mar 4, 2025

View reviewed changes

Comment thread ext/span.c

bwoebi reviewed Mar 4, 2025

View reviewed changes

fix: remove added refcount

9e45f4c

PROFeNoM added 3 commits March 5, 2025 11:16

Create inferred span from distributed headers consumption

dcc384e

cr: Check that the inferred_span property is an instance of ddtrace_c…

d2629ab

…e_inferred_span_data

style: Refactor metrics/meta data transfer during serialization

5fcf9b4

bwoebi reviewed Mar 5, 2025

View reviewed changes

PROFeNoM added 3 commits March 6, 2025 10:48

(raw) Handle propagated tags & sampling rules

14deec3

style: refactor inferred span retrieval

b867140

Change serialized data transfer location & update snapshots

86a671b

bwoebi reviewed Mar 7, 2025

View reviewed changes

Return serialized array

7b84455

bwoebi reviewed Mar 7, 2025

View reviewed changes

PROFeNoM added 6 commits March 7, 2025 15:44

fix: return value of ddtrace_serialize_span_to_array

5dc26f0

chore: update snapshots

265d359

tests: stop using request replayer

0b6a8dc

tests: update tests

dc3a925

tests: Windows compatibility

8d3a9f3

fix: SEGV when tracer is disabled

9ea9fe7

PROFeNoM marked this pull request as ready for review March 10, 2025 09:10

PROFeNoM requested a review from a team as a code owner March 10, 2025 09:10

bwoebi reviewed Mar 10, 2025

View reviewed changes

Comment thread ext/serializer.c Outdated

PROFeNoM added 2 commits March 17, 2025 10:59

tests: Update after merge

bcad5e9

fix: cast proxy_info->component to char * when passing it to add_asso…

c1d96e3

…c_string

wantsui reviewed Mar 17, 2025

View reviewed changes

zarirhamza mentioned this pull request Mar 17, 2025

[php] Add APIGW Tests DataDog/system-tests#4321

Merged

7 tasks

PROFeNoM added 2 commits March 18, 2025 08:40

style: Move _dd.inferred_span to metrics

854ce61

Fallback to global config on missing domain name

3534148

PROFeNoM marked this pull request as draft March 18, 2025 08:03

PROFeNoM added 2 commits March 18, 2025 09:09

fix: "error": 1 not properly transferred

0785b7c

tests: Assert proper span duration

e75a791

PROFeNoM marked this pull request as ready for review March 18, 2025 08:33

tests: Windows tests weird behavior

3367c01

bwoebi reviewed Mar 18, 2025

View reviewed changes

fix: correct track_error serialization in span metadata

cfff50e

PROFeNoM marked this pull request as draft March 19, 2025 08:52

PROFeNoM marked this pull request as ready for review March 19, 2025 09:20

PROFeNoM and others added 6 commits March 27, 2025 08:46

Merge master

6461f18

tests: Update active_span.phpt & span_clone.phpt

3d054c1

fix arginfo

bb9a498

Make dummy_span actually return a RootSpanData instance

9e58b81

Signed-off-by: Bob Weinand <[email protected]>

Free the proxy_map

088b3a2

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the alex/AIDM-548_api-gateway-ter branch from 7255612 to 088b3a2 Compare March 27, 2025 20:06

Avoid pcre JIT to prevent valgrind complaining

553515e

Signed-off-by: Bob Weinand <[email protected]>

bwoebi force-pushed the alex/AIDM-548_api-gateway-ter branch from f234ee7 to 553515e Compare March 28, 2025 14:10

bwoebi approved these changes Mar 28, 2025

View reviewed changes

Merge branch 'master' of github.com:DataDog/dd-trace-php into alex/AI…

ddc3c19

…DM-548_api-gateway-ter

PROFeNoM merged commit dfe83ec into master Mar 31, 2025
751 of 769 checks passed

PROFeNoM deleted the alex/AIDM-548_api-gateway-ter branch March 31, 2025 08:31

github-actions Bot added this to the 1.8.0 milestone Mar 31, 2025

wantsui mentioned this pull request Mar 31, 2025

[php][API Gateway] Add tracer version to the API Gateway test DataDog/system-tests#4438

Merged

7 tasks

-    zval meta_zv, *meta = &span->property_meta, *inferred_span_zv = NULL;
+    zval meta_zv, *meta = &span->property_meta;
+    ddtrace_span_data *inferred_span = NULL;
+    if (is_root_span) {
+        inferred_span = ddtrace_get_inferred_span(ROOTSPANDATA(&span->std));
+    }

Conversation

PROFeNoM commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Reviewer checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter Bot commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ tracer ]

scenario:ContextPropagationBench/benchExtractHeaders128Bit

scenario:ContextPropagationBench/benchExtractHeaders128Bit-opcache

scenario:ContextPropagationBench/benchExtractTraceContext128Bit

scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache

scenario:ContextPropagationBench/benchExtractTraceContext64Bit

scenario:ContextPropagationBench/benchExtractTraceContext64Bit-opcache

scenario:TraceFlushBench/benchFlushTrace

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

PROFeNoM commented Mar 4, 2025 •

edited

Loading

codecov-commenter commented Mar 5, 2025 •

edited

Loading

pr-commenter Bot commented Mar 5, 2025 •

edited

Loading

bwoebi Mar 7, 2025 •

edited

Loading

bwoebi Mar 10, 2025 •

edited

Loading

bwoebi Mar 10, 2025 •

edited

Loading

bwoebi Mar 18, 2025 •

edited

Loading