Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support w3c trace context #4170

Merged
merged 35 commits into from Sep 20, 2022
Merged

feat: support w3c trace context #4170

merged 35 commits into from Sep 20, 2022

Conversation

ZStriker19
Copy link
Contributor

@ZStriker19 ZStriker19 commented Sep 6, 2022

Description

We want to support W3C propagation headers which is what Otel uses. w3c standard.
Currently we're only adding support for traceparent to propagate the trace context. We are not yet adding support for tracestate but are planning to in future work.

Motivation

We want to support otel users and possibly other future tracers that use this format for distributed tracing

Design

Overview:

  - ``traceparent`` describes the position of the incoming request in its
    trace graph in a portable, fixed-length format. Its design focuses on
    fast parsing. Every tracing tool MUST properly set traceparent even when
    it only relies on vendor-specific information in tracestate

The format for ``traceparent`` is::

  HEXDIGLC        = DIGIT / "a" / "b" / "c" / "d" / "e" / "f"
  value           = version "-" version-format
  version         = 2HEXDIGLC
  version-format  = trace-id "-" parent-id "-" trace-flags
  trace-id        = 32HEXDIGLC
  parent-id       = 16HEXDIGLC
  trace-flags     = 2HEXDIGLC

Example value of HTTP ``traceparent`` header::

    value = 00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01
    base16(version) = 00
    base16(trace-id) = 4bf92f3577b34da6a3ce929d0e0e4736
    base16(parent-id) = 00f067aa0ba902b7
    base16(trace-flags) = 01  // sampled

Implementation details:

  - Datadog Trace and Span IDs are 64-bit unsigned integers.
  - The W3C Trace Context Trace ID is a 32-byte hexadecimal string. This is
    transformed for propagation between Datadog by taking the lower
    16-bytes.

Testing strategy

I've added tests similar to those for b3, making sure that we're converting the W3C header correctly into a context via _extract() and that we're converting the context correctly into the traceparent W3C header via `_inject().

Checklist

Reviewer Checklist

  • Title is accurate.
  • Description motivates each change.
  • No unnecessary changes were introduced in this PR.
  • PR cannot be broken up into smaller PRs.
  • Avoid breaking API changes unless absolutely necessary.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Release note has been added for fixes and features, or else changelog/no-changelog label added.
  • All relevant GitHub issues are correctly linked.
  • Backports are identified and tagged with Mergifyio.
  • Add to milestone.

@ZStriker19 ZStriker19 marked this pull request as ready for review September 8, 2022 19:44
@ZStriker19 ZStriker19 requested a review from a team as a code owner September 8, 2022 19:44
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ZStriker19 and others added 3 commits September 12, 2022 12:45
Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
…26f0e.yaml

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things, but overall good with the approach and testing 👍

ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Outdated Show resolved Hide resolved
…26f0e.yaml

Co-authored-by: Kyle Verhoog <kyle@verhoog.ca>
@codecov-commenter
Copy link

codecov-commenter commented Sep 15, 2022

Codecov Report

Merging #4170 (11dddbd) into 1.x (e315198) will increase coverage by 0.00%.
The diff coverage is 89.83%.

@@           Coverage Diff           @@
##              1.x    #4170   +/-   ##
=======================================
  Coverage   78.08%   78.09%           
=======================================
  Files         734      734           
  Lines       58874    58923   +49     
=======================================
+ Hits        45974    46016   +42     
- Misses      12900    12907    +7     
Impacted Files Coverage Δ
ddtrace/propagation/http.py 94.75% <88.46%> (-1.56%) ⬇️
ddtrace/internal/constants.py 100.00% <100.00%> (ø)
tests/tracer/test_propagation.py 100.00% <100.00%> (ø)
ddtrace/internal/telemetry/writer.py 76.82% <0.00%> (-0.67%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just one thought, otherwise lgtm!

ddtrace/propagation/http.py Show resolved Hide resolved
@majorgreys majorgreys changed the title feat: add w3c trace context http header propagation feat: support w3c trace context Sep 19, 2022
Kyle-Verhoog
Kyle-Verhoog previously approved these changes Sep 19, 2022
Copy link
Collaborator

@majorgreys majorgreys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc changes otherwise lgtm

ddtrace/propagation/http.py Outdated Show resolved Hide resolved
ddtrace/propagation/http.py Show resolved Hide resolved
ZStriker19 and others added 4 commits September 19, 2022 13:00
Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
…26f0e.yaml

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
…26f0e.yaml

Co-authored-by: Tahir H. Butt <tahir.butt@datadoghq.com>
@Kyle-Verhoog Kyle-Verhoog merged commit e652b99 into 1.x Sep 20, 2022
@Kyle-Verhoog Kyle-Verhoog deleted the majorgreys/AIT-3988 branch September 20, 2022 17:32
@github-actions github-actions bot added this to the v1.6.0 milestone Sep 20, 2022
ZStriker19 added a commit that referenced this pull request Oct 6, 2022
mergify bot pushed a commit that referenced this pull request Oct 7, 2022
Reverts #4170
The RFC for this feature is not yet approved and future changes are likely.
trace_flags = 1 if sampling_priority and sampling_priority >= AUTO_KEEP else 0

# There is currently only a single version so we always start with 00
traceparent = "00-{:032x}-{:016x}-{:02x}".format(trace_id, span_id, trace_flags)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants