dd-trace-php icon indicating copy to clipboard operation
dd-trace-php copied to clipboard

w3c phase 2: add last parent_id to tracestate

Open mabdinur opened this issue 11 months ago • 4 comments

Description

With this change span_id will be encoded in both the tracestate and traceparent headers. This will allow us to re-connect datadog traces that contain non-datadog spans.

Ex: Host A: Datadog Spans ->tracecontext-> Host B: Otel Dynatrace Spans -->tracecontext-> Host C: Otel Datadog Spans

  • By adding span_id to the tracestate Host C will be able to detect upstream Datadog spans from Host A. This span_id will be sent to Datadog intake services via _dd.parent_id tag. The Datadog internal services will use this tag to reconnect visualize spans.

Tests will be added in this PR: https://github.com/DataDog/system-tests/pull/2181

Reviewer checklist

  • [ ] Test coverage seems ok.
  • [ ] Appropriate labels assigned.

mabdinur avatar Feb 29 '24 22:02 mabdinur

Codecov Report

Merging #2549 (5799170) into master (91227e5) will decrease coverage by 23.59%. The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #2549       +/-   ##
=============================================
- Coverage     75.77%   52.18%   -23.59%     
  Complexity     2563     2563               
=============================================
  Files           241      241               
  Lines         27024    27043       +19     
  Branches        976      976               
=============================================
- Hits          20478    14113     -6365     
- Misses         6026    12410     +6384     
  Partials        520      520               
Flag Coverage Δ
appsec-extension 69.13% <ø> (ø)
tracer-extension 78.73% <100.00%> (+0.03%) :arrow_up:
tracer-php 12.71% <ø> (-62.01%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
ext/ddtrace.c 73.13% <100.00%> (ø)
ext/distributed_tracing_headers.c 84.46% <100.00%> (+0.51%) :arrow_up:
ext/handlers_http.h 94.40% <100.00%> (+0.37%) :arrow_up:

... and 61 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 91227e5...5799170. Read the comment docs.

codecov-commenter avatar Feb 29 '24 22:02 codecov-commenter

Benchmarks

Benchmark execution time: 2024-03-19 12:14:47

Comparing candidate commit 8fcf55131e533bd0feaebdd810af1edaa9964df3 in PR branch munir/add-p-to-php with baseline commit 91227e576ddc17ad4e7434c288c9d8d85d063688 in branch master.

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

scenario:ContextPropagationBench/benchExtractTraceContext128Bit

  • 🟥 execution_time [+75.995ns; +124.005ns] or [+3.447%; +5.624%]

scenario:ContextPropagationBench/benchExtractTraceContext128Bit-opcache

  • 🟥 execution_time [+45.059ns; +130.941ns] or [+2.050%; +5.957%]

scenario:LaravelBench/benchLaravelBaseline-opcache

  • 🟥 execution_time [+76.841µs; +228.919µs] or [+3.067%; +9.137%]

scenario:PDOBench/benchPDOBaseline-opcache

  • 🟩 execution_time [-14.636µs; -13.410µs] or [-7.786%; -7.134%]

scenario:PDOBench/benchPDOOverhead-opcache

  • 🟩 execution_time [-16.453µs; -14.379µs] or [-5.593%; -4.888%]

scenario:PDOBench/benchPDOOverheadWithDBM-opcache

  • 🟩 execution_time [-16.476µs; -14.336µs] or [-5.063%; -4.405%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2

  • 🟥 execution_time [+164.649ns; +431.151ns] or [+2.706%; +7.087%]

scenario:SamplingRuleMatchingBench/benchRegexMatching2-opcache

  • 🟥 execution_time [+122.734ns; +338.866ns] or [+2.031%; +5.608%]

scenario:SamplingRuleMatchingBench/benchRegexMatching3

  • 🟥 execution_time [+150.797ns; +420.003ns] or [+2.483%; +6.916%]

scenario:SamplingRuleMatchingBench/benchRegexMatching4

  • 🟥 execution_time [+189.401ns; +402.599ns] or [+3.094%; +6.577%]

scenario:SpanBench/benchOpenTelemetryAPI

  • 🟥 mem_peak [+795.710KB; +795.716KB] or [+2.062%; +2.062%]

scenario:SpanBench/benchOpenTelemetryAPI-opcache

  • 🟥 mem_peak [+793.138KB; +793.147KB] or [+2.288%; +2.288%]

pr-commenter[bot] avatar Mar 01 '24 17:03 pr-commenter[bot]

We will need to ignore/filter out the p value in snapshot tests

mabdinur avatar Mar 06 '24 14:03 mabdinur

@mabdinur Yep, needs a change in the test-agent to filter them there.

bwoebi avatar Mar 06 '24 15:03 bwoebi

@bwoebi thanks for fixing the failing tests and updating the OpenTelemetry SpanContext. A few parametric tests are failing due to the changes we made to tracestate. Once those failures are addressed we can merge this PR 🥳

mabdinur avatar Mar 19 '24 14:03 mabdinur