contrib/internal/httptrace: add support for inferred proxy spans by jordan-wong · Pull Request #3052 · DataDog/dd-trace-go

jordan-wong · 2024-12-19T17:34:30Z

What does this PR do?

This PR introduces functionality to infer proxy spans based on headers that we instruct users to inject upstream, allowing these headers to be propagated downstream. The headers are then used to infer a proxy span to provide increased visibility to a user's trace. The headers are as follows:

Header Name	AWS Mappings	Header Contents
`x-dd-proxy`	`aws-apigateway`	`aws-apigateway`
`x-dd-proxy-request-time-ms`	`context.requestTimeEpoch`	Start time of request in ms
`x-dd-proxy-domain-name`	`context.domainName`	AWS domain name
`x-dd-proxy-httpmethod`	`context.httpMethod`	HTTP method used for request
`x-dd-proxy-path`	`context.path`	Path for request
`x-dd-proxy-stage`	`context.stage`	AWS stage name

Motivation

There is no current support for API Gateway and other proxies that do not run Datadog code. By inferring spans via headers, the gaps in observability can be filled to provide a more accurate trace.

Reviewer's Checklist

Changed code has unit tests for its functionality at or near 100% coverage.
System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
There is a benchmark for any new code, or changes to existing code.
If this interacts with the agent in a new way, a system test has been added.
Add an appropriate team label so this PR gets put in the right place for the release notes.
Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.
For internal contributors, a matching PR should be created to the v2-dev branch and reviewed by @DataDog/apm-go.

Unsure? Have a question? Request a review!

zarirhamza

Needs a bit of cleanup and logging for prod but logically looks good. Going to request @wantsui to look at it as well to confirm the span information is relatively similar as in dd-trace-js.

pr-commenter · 2024-12-20T18:54:09Z

Benchmarks

Benchmark execution time: 2025-01-22 22:12:35

Comparing candidate commit 39087f1 in PR branch add-inferred-proxy-spans with baseline commit 241a6f1 in branch main.

Found 0 performance improvements and 3 performance regressions! Performance is the same for 55 metrics, 1 unstable metrics.

scenario:BenchmarkStartRequestSpan-24

🟥 allocated_mem [+72 bytes; +72 bytes] or [+40.909%; +40.909%]
🟥 allocations [+1; +1] or [+20.000%; +20.000%]
🟥 execution_time [+66.057ns; +67.123ns] or [+21.642%; +21.992%]

zarirhamza · 2025-01-07T15:37:08Z

Let's also make sure that we test that our inferred span matches the behavior of the parent span by asserting things like that status code are the same

…W spans via headers Function checks for existence of AWS APIGW headers and infers proxy span if necessary Updates usage in all contribs that implement function Updates test usage that implement function

datadog-datadog-prod-us1 · 2025-01-15T05:48:33Z

Datadog Report

Branch report: add-inferred-proxy-spans
Commit report: 64501fb
Test service: dd-trace-go

✅ 0 Failed, 5207 Passed, 72 Skipped, 2m 19.94s Total Time

darccio · 2025-01-22T15:33:05Z

@zarirhamza @jordan-wong Do you know why the benchmarks are reporting this 50%+ increase in execution time in BenchmarkStartRequestSpan?

🟥 execution_time [+159.899ns; +160.941ns] or [+52.326%; +52.667%]

darccio · 2025-01-22T15:39:28Z

+	spanParentCtx, spanParentErr := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header))
+	spanLinksCtx, spanLinksOk := spanParentCtx.(ddtrace.SpanContextWithLinks)


@zarirhamza @jordan-wong I think this unconditional extraction of spanParentCtx is the source of the performance impact.

Would it be possible to isolate everything related to the inferred proxy spans so it runs if the feature is activated?

Previously, the span extraction would happen on every call anyway just within the span option func. I tried isolating all behaviors as much as possible so that should improve the performance.

Also unsure if the fact we now have cleanup functions via StartRequestSpans may result in increased usage that is expected solely due to behavior

darccio

Looks like the increased CPU time and memory are to be expected due to the new implementation.

add working draft inferred proxy spans

6fb5400

jordan-wong requested a review from zarirhamza December 19, 2024 17:34

jordan-wong self-assigned this Dec 19, 2024

jordan-wong added 2 commits December 19, 2024 13:11

remove unnecessary setting of env vars in tests

9762bfd

fix small logic error with env var check

63c8418

zarirhamza requested changes Dec 19, 2024

View reviewed changes

zarirhamza requested a review from wantsui December 19, 2024 20:42

jordan-wong added 3 commits December 20, 2024 11:16

clean up print statements

5f16ca4

change print statements to log lines, make code more readable

10ed7f7

More cleanup

923d28b

wantsui requested changes Dec 23, 2024

View reviewed changes

Comment thread contrib/internal/httptrace/inferred_proxy.go Outdated

Comment thread contrib/internal/httptrace/httptrace_api_gateway_test.go Outdated

Comment thread contrib/internal/httptrace/inferred_proxy.go Outdated

Comment thread contrib/internal/httptrace/inferred_proxy.go Outdated

add fallback value for service name using tracer ServiceName

b356a70

wantsui reviewed Jan 7, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/inferred_proxy.go Outdated

zarirhamza added 2 commits January 9, 2025 15:20

rewrite env var check with config

7c93173

contrib/internal/httptrace: Update StartRequestSpan to infer AWS APIG…

8da09a0

…W spans via headers Function checks for existence of AWS APIGW headers and infers proxy span if necessary Updates usage in all contribs that implement function Updates test usage that implement function

rarguelloF requested changes Jan 13, 2025

View reviewed changes

zarirhamza added 5 commits January 14, 2025 16:05

refactor code for readability

ff31ebf

adds span links for inferred spans

ce40b43

fix conflicts with spanlinks

a9aeec2

Merge branch 'main' into add-inferred-proxy-spans

43ff23f

fix copyright

1ec1be9

zarirhamza added 2 commits January 15, 2025 01:13

compare http and apigw spans in tests

92fe98b

fix propagation tests

68be243

darccio requested changes Jan 15, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/httptrace.go Outdated

Comment thread contrib/internal/httptrace/httptrace.go Outdated

Comment thread contrib/internal/httptrace/httptrace.go Outdated

rarguelloF and others added 3 commits January 15, 2025 17:28

refactor

8be56b3

add new test

78c83fa

Merge branch 'main' into add-inferred-proxy-spans

aceef66

zarirhamza added 2 commits January 15, 2025 16:14

fix errors and refactor

5510462

refactor

6122e7d

rarguelloF requested changes Jan 17, 2025

View reviewed changes

zarirhamza and others added 3 commits January 19, 2025 01:09

address changes

a4d8959

Merge branch 'main' into add-inferred-proxy-spans

2bb7737

Merge branch 'main' into add-inferred-proxy-spans

a7641c7

zarirhamza changed the title ~~Add inferred proxy spans~~ contrib/internal/httptrace: add support for inferred proxy spans Jan 21, 2025

github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Jan 21, 2025

zarirhamza marked this pull request as ready for review January 21, 2025 14:55

zarirhamza requested review from a team as code owners January 21, 2025 14:55

rarguelloF requested changes Jan 21, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/inferred_proxy.go Outdated

zarirhamza added 2 commits January 21, 2025 10:50

removes unnnecessary errors

16d7388

fix errors

4394d26

rarguelloF requested changes Jan 21, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/httptrace.go Outdated

fix errors again

d64e3f8

zarirhamza approved these changes Jan 21, 2025

View reviewed changes

rarguelloF self-requested a review January 21, 2025 16:14

rarguelloF approved these changes Jan 21, 2025

View reviewed changes

wantsui reviewed Jan 21, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/inferred_proxy.go

wantsui requested changes Jan 21, 2025

View reviewed changes

Comment thread contrib/internal/httptrace/httptrace_api_gateway_test.go

Comment thread contrib/internal/httptrace/inferred_proxy.go

adds assertions to test service and errors

4a22660

wantsui approved these changes Jan 21, 2025

View reviewed changes

darccio requested changes Jan 22, 2025

View reviewed changes

zarirhamza added 2 commits January 22, 2025 15:04

improve performance by isolating inferred span operations

78b3d4b

Merge branch 'main' into add-inferred-proxy-spans

39087f1

darccio approved these changes Jan 23, 2025

View reviewed changes

darccio merged commit 700f43e into main Jan 23, 2025

darccio deleted the add-inferred-proxy-spans branch January 23, 2025 09:05

		spanParentCtx, spanParentErr := tracer.Extract(tracer.HTTPHeadersCarrier(r.Header))
		spanLinksCtx, spanLinksOk := spanParentCtx.(ddtrace.SpanContextWithLinks)

Uh oh!

Conversation

jordan-wong commented Dec 19, 2024 • edited by zarirhamza Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Reviewer's Checklist

Uh oh!

zarirhamza left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pr-commenter Bot commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:BenchmarkStartRequestSpan-24

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zarirhamza commented Jan 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

datadog-datadog-prod-us1 Bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

darccio commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darccio Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

zarirhamza Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

zarirhamza Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

darccio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jordan-wong commented Dec 19, 2024 •

edited by zarirhamza

Loading

pr-commenter Bot commented Dec 20, 2024 •

edited

Loading

datadog-datadog-prod-us1 Bot commented Jan 15, 2025 •

edited

Loading

darccio commented Jan 22, 2025 •

edited

Loading