feat: API Gateway Tracing #3095

PROFeNoM · 2025-02-19T12:13:14Z

Description

This PR adds the possibility to create a new span on top of the existing request span based on received headers. This feature is opt-in through DD_TRACE_INFERRED_PROXY_SERVICES_ENABLED.

The newly created span (the "inferred span") will be a root span. The original request span will also be a root span. The latter will be child of the former.

Runtime information (e.g., memory peak) will be set on the inferred span. It won't be set on both spans.

While the inferred span being the root span of the stack, it won't be returned by using DDTrace\root_span. Instead, the child will be returned to preserve the current behavior. The inferred span can still be accessed by using the parent property.

Errors set on the original request span will be set on the inferred span as well.

Implementation Doc: link

Telemetry PR: https://github.com/DataDog/dd-go/pull/171137

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

zarirhamza · 2025-02-20T15:38:01Z

tests/Integrations/Laravel/APIGWTest.php

+                    'A simple GET request returning a string',
+                    '/simple?key=value&pwd=should_redact',
+                    [
+                        'x-dd-proxy: aws.apigateway',


The way we implemented it in other languages was to have a map w/the header value of aws-apigateway to map to a span name value of aws.apigateway. We want the header value to remain unchanged and we want the span name to be slightly different which can be represented by a key value map.

That way we can add more values in the future if we have more proxy headers that correspond to different operation names

Refer to this as an example

Note, they also include the component value in the map which would change for future proxies as well that may not necessarily be equal to the proxy header

bwoebi · 2025-02-21T10:28:49Z

ext/ddtrace.c

    UNUSED(old_value, new_str);

    ddtrace_span_properties *pspan = ddtrace_active_span_props();
    while (pspan) {
+        if (skip_inferred) {
+            ddtrace_span_data *span = SPANDATA(pspan);
+            if (span && span->type == DDTRACE_INFERRED_SPAN) {


Suggested change

if (span && span->type == DDTRACE_INFERRED_SPAN) {

if (span->type == DDTRACE_INFERRED_SPAN) {

Nit, but SPANDATA() is just an offset calculation from pspan and will never be NULL.

bwoebi · 2025-02-21T10:34:52Z

ext/inferred_proxy_headers.c

+    ddtrace_proxy_info *info;
+    ZEND_HASH_FOREACH_PTR(&proxy_info_map, info) {
+        efree(info);
+    } ZEND_HASH_FOREACH_END();


Nit: the idiomatic way would be using:

static dd_proxy_info_dtor(zval *zv) { efree(Z_PTR_P(zv)); }

and passing it to zend_hash_init as destructor.

bwoebi · 2025-02-21T10:35:40Z

ext/inferred_proxy_headers.c

+}
+
+void ddtrace_init_proxy_info_map(void) {
+    zend_hash_init(&proxy_info_map, 8, NULL, NULL, 1);


This should be 0 as last parameter right? persistent=1 is for memory supposed to survive the request cycle.

Oops, indeed. I initially wanted to do the initialization/free in MINIT/MSHUTDOWN, but faced issues. Nice catch, ty.

this should be in minit/mshutdown though. No point in reinitializing that every request.

bwoebi · 2025-02-21T12:32:02Z

ext/serializer.c

+    } else if (span->std.ce == ddtrace_ce_root_span_data) {
+        ddtrace_root_span_data *root = ROOTSPANDATA(&span->std);
+        if (root->child_root) { // Can't check for DDTRACE_INFERRED_SPAN because the span is now closed
+            zval *child_exception_zv = &root->child_root->span.property_exception;
+            has_exception = Z_TYPE_P(child_exception_zv) == IS_OBJECT && instanceof_function(Z_OBJCE_P(child_exception_zv), zend_ce_throwable);


Can you move this before the if (has_exception) above?

// inherit exception on inferred span from child root if (!has_exception && span->std.ce == ddtrace_ce_root_span_data) { ddtrace_root_span_data *root = ROOTSPANDATA(&span->std); if (root->child_root) { // Can't check for DDTRACE_INFERRED_SPAN because the span is now closed exception_zv = &root->child_root->span.property_exception; has_exception = Z_TYPE_P(child_exception_zv) == IS_OBJECT && instanceof_function(Z_OBJCE_P(child_exception_zv), zend_ce_throwable); } }

The duplication was confusing me

bwoebi · 2025-02-21T12:38:20Z

ext/span.h

@@ -86,6 +87,7 @@ struct ddtrace_span_data {
    bool notify_user_req_end;
    struct ddtrace_span_data *next;
    struct ddtrace_root_span_data *root;
+    bool is_child_of_inferred_span;


Why do we have this property at all? Isn't it redundant with child_root != NULL on the root_span_data?

I see only one usage of it, and it's trivially replaced.

bwoebi · 2025-02-21T12:45:12Z

ext/span.h

@@ -107,6 +109,7 @@ struct ddtrace_root_span_data {
    ddtrace_rule_result sampling_rule;
    bool explicit_sampling_priority;
    enum ddtrace_trace_limited trace_is_limited;
+    struct ddtrace_root_span_data *child_root; // Only used when inferring proxy services (type: DDTRACE_INFERRED_SPAN)


let's rename it to inferred_root

bwoebi · 2025-02-27T12:45:44Z

ext/span.c

+    if (!proxy_info) {
+        zend_string_release(result.system);
+        zend_string_release(result.start_time_ms);
+        return NULL;


You're leaking the other strings in result here, there are more than two.

bwoebi · 2025-02-27T12:47:12Z

ext/inferred_proxy_headers.c

+    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);
+    read_header((zai_str)ZAI_STRL("X_DD_PROXY_STAGE"), "x-dd-proxy-stage", &result.stage, data);


you should probably put this in an if (result.system || result.path) to short-circuit this away.

bwoebi · 2025-02-27T12:51:14Z

ext/span.c

+        ZVAL_STR(&span->property_resource, strpprintf(0, "%s %s", ZSTR_VAL(result.http_method), ZSTR_VAL(result.path)));
+    }
+
+    span->start = (zend_long)zend_strtod(ZSTR_VAL(result.start_time_ms), NULL) * 1000000;


This looks fishy. Either you should directly parse it as a long, or you need parenthesis (if the number is actually a decimal number). The (zend_long) cast is evaluated before the multiplication.
If the goal is string to zend_long, use ZEND_ATOL().

bwoebi · 2025-02-27T13:11:00Z

How is this supposed to interact with sampling decisions and distributed tracing?
Currently it's going to use the api gateway root span for sampling decisions (e.g. DD_TRACE_SAMPLING_RULES is going to match against the inferred root rather the "normal" root). Is that intentional? Also, it's two root spans, whose sampling wins? Especially when manually setting sampling decisions...
Same for distributed tracing, where are the propagated root tags to be taken from?

Also: ddtrace_get_root_span() is the function appsec uses to attach sampling info and root tags to. Is that supposed to be the inferred span or the proper root?

bwoebi · 2025-02-27T13:21:01Z

ext/span.c

+
+    if (result.domain) {
+        ZVAL_STR_COPY(&zv, result.domain);
+        ddtrace_assign_variable(&span->property_service, &zv);


Is this if (result.domain) rather a safeguard or something which will regularly happen? Because if it can happen that the domain is not set, the inferred span will have the generic service name (http.request), while frameworks will have the service name assigned by the integrations on the root span. Which will certainly be unexpected.

ext/span.c

PROFeNoM · 2025-02-27T13:25:01Z

If I refer to the implementation doc, it would appear that it is expected for the sampling rules to be matched against the new, inferred root span. I assume this is intentional.

About the other questions, I don't know. At a glance, I am more concerned about the appsec question, as this is an internal behavior that the customer won't have power upon at all. I'd have to ask to know whether a decision was already made.

bwoebi · 2025-02-27T14:21:59Z

@PROFeNoM Could you please ask whether that's supposed to affect all metadata (like, "tags" or "resource"), or only the service name? Because if it's the former it will be impossible to define sampling rules according to runtime behaviour (i.e. "if this tag is set, then apply only this reduced sampling rate), essentially.

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

Superseded by #3116

PROFeNoM added 30 commits February 7, 2025 11:23

Base Implementation

f0c4bf2

fix: segfault

4d706fb

fix: duration

39f9f2a

fix: duration

fccd91b

error log start/duration/duration_start

2bbf531

fix: duration

291f75b

x-dd-proxy-domain-name add debug logs

6fc1632

service debug logs

162b382

tests: Laravel x InferredProxy

641811a

fix: Specifically do not inherit service property

144de18

more debug logs

ba71c60

fix: Don't overwrite inferred span's service.

9fb4ab9

style: refactor service alter skip

7153541

feat: Octane Swoole support

03c0569

style: Refactor using DDTRACE_INFERRED_SPAN type

cf65501

tests: Add some (unfinished) tests

a1c525a

feat: Very quick Roadrunner support (to be enhanced)

bd91a71

Set _dd.inferred_span to 1

bcc25af

tests: Clean EXPECTF section

d4b2745

fix: Memory leak

e91cc69

chore: update snapshots

dbd2e66

tests: Distributed Tracing

3e1945c

tests: Exceptions in Roadrunner/Swoole

969ff8b

fix: Laravel Latest CLI Test Script Location

18a1527

fix: persisting DD_SERVICE on Laravel Tests

8c22cd5

fix: Roadrunner tests order

42e3581

fix: Laravel Connection issue

cf59fa3

fix: Laravel Connection issue

509bc51

error output

4c29c16

error output

340b32e

PROFeNoM added 5 commits February 20, 2025 10:07

style: Change x-dd-proxy value from aws-apigateway to aws.apigateway

d39e0b7

style: Remove duration from serialization debug logs

6aecc1f

fix: Prevent segfaults/mem leak on missing headers

926dd2b

style: one-liner

bdb31b5

style: change test description

e5707d7

zarirhamza requested changes Feb 20, 2025

View reviewed changes

Add a hashmap of supported proxies

aaac753

bwoebi reviewed Feb 21, 2025

View reviewed changes

CR refactor

2a1da16

PROFeNoM marked this pull request as ready for review February 27, 2025 09:25

PROFeNoM requested review from a team as code owners February 27, 2025 09:25

bwoebi reviewed Feb 27, 2025

View reviewed changes

cr: early-return & memory leak

2e54a22

bwoebi reviewed Feb 27, 2025

View reviewed changes

ext/span.c Outdated Show resolved Hide resolved

cr: Set free_inferred_proxy_result to static

c0f1c28

PROFeNoM marked this pull request as draft February 28, 2025 08:50

PROFeNoM closed this Mar 4, 2025

	if (span && span->type == DDTRACE_INFERRED_SPAN) {
	if (span->type == DDTRACE_INFERRED_SPAN) {

feat: API Gateway Tracing #3095

feat: API Gateway Tracing #3095

Uh oh!

Conversation

PROFeNoM commented Feb 19, 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!

zarirhamza Feb 20, 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!

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 Feb 21, 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!

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 commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwoebi Feb 27, 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!

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

Uh oh!

Uh oh!

bwoebi commented Feb 27, 2025

Uh oh!

PROFeNoM commented Mar 4, 2025

Uh oh!

Uh oh!

PROFeNoM commented Feb 19, 2025 •

edited

Loading

zarirhamza Feb 20, 2025 •

edited

Loading

bwoebi Feb 21, 2025 •

edited

Loading

bwoebi commented Feb 27, 2025 •

edited

Loading

bwoebi Feb 27, 2025 •

edited

Loading

PROFeNoM commented Feb 27, 2025 •

edited

Loading