WIP: otel baggage support initial PR #10389

rachelyangdog · 2024-08-26T20:28:48Z

First PR introducing otel baggage support

Checklist

PR author has checked that all the criteria below are met
The PR description includes an overview of the change
The PR description articulates the motivation for the change
The change includes tests OR the PR description describes a testing strategy
The PR description notes risks associated with the change, if any
Newly-added code is easy to change
The change follows the library release note guidelines
The change includes or references documentation updates if necessary
Backport labels are set (if applicable)

Reviewer Checklist

Reviewer has checked that all the criteria below are met
Title is accurate
All changes are related to the pull request's stated goal
Avoids breaking API changes
Testing strategy adequately addresses listed risks
Newly-added code is easy to change
Release note makes sense to a user of the library
If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
Backport labels are set in a manner that is consistent with the release branch maintenance policy

github-actions · 2024-08-26T20:29:17Z

CODEOWNERS have been resolved as:

ddtrace/_trace/context.py                                               @DataDog/apm-sdk-api-python
ddtrace/_trace/span.py                                                  @DataDog/apm-sdk-api-python
ddtrace/internal/constants.py                                           @DataDog/apm-core-python
ddtrace/internal/opentelemetry/context.py                               @DataDog/apm-sdk-api-python
ddtrace/propagation/http.py                                             @DataDog/apm-sdk-api-python
ddtrace/settings/config.py                                              @DataDog/python-guild @DataDog/apm-sdk-api-python
tests/opentelemetry/test_context.py                                     @DataDog/apm-sdk-api-python
tests/tracer/test_propagation.py                                        @DataDog/apm-sdk-api-python
tests/tracer/test_span.py                                               @DataDog/apm-sdk-api-python

pr-commenter · 2024-08-26T21:43:03Z

Benchmarks

Benchmark execution time: 2024-08-27 15:31:04

Comparing candidate commit 36d68d3 in PR branch rachel.yang/baggage with baseline commit 7112beb in branch main.

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

datadog-dd-trace-py-rkomorn · 2024-08-27T14:50:34Z

Datadog Report

Branch report: rachel.yang/baggage
Commit report: 4c28ab9
Test service: dd-trace-py

❌ 31 Failed (0 Known Flaky), 175899 Passed, 1677 Skipped, 9h 26m 4.02s Total duration (9m 24.94s time saved)

❌ Failed Tests (31)

This report shows up to 5 failed tests.

test_otel_multithreading - test_context.py - Details

Expand for error

 At request <Request GET /test/session/snapshot >:
    At snapshot (token='tests.opentelemetry.test_context.test_otel_multithreading'):
     - Directory: /snapshots
     - CI mode: 0
     - Trace File: /snapshots/tests.opentelemetry.test_context.test_otel_multithreading.json
     - Stats File: /snapshots/tests.opentelemetry.test_context.test_otel_multithreading_tracestats.json
     At compare of 1 expected trace(s) to 1 received trace(s):
      At trace 'internal' (13 spans):
 Received fewer spans (5) than expected (13). Expected unmatched spans: 'internal', 'internal', 'internal', 'internal', 'internal', 'internal', 'internal', 'internal'

test_otel_span_parenting - test_context.py - Details

Expand for error

 At request <Request GET /test/session/snapshot >:
    At snapshot (token='tests.opentelemetry.test_context.test_otel_span_parenting'):
     - Directory: /snapshots
     - CI mode: 0
     - Trace File: /snapshots/tests.opentelemetry.test_context.test_otel_span_parenting.json
     - Stats File: /snapshots/tests.opentelemetry.test_context.test_otel_span_parenting_tracestats.json
     At compare of 1 expected trace(s) to 1 received trace(s):
      At trace 'internal' (6 spans):
 Received fewer spans (1) than expected (6). Expected unmatched spans: 'internal', 'internal', 'internal', 'internal', 'internal'

test_otel_trace_across_fork - test_context.py - Details

Expand for error

 At request <Request GET /test/session/snapshot >:
    At snapshot (token='tests.opentelemetry.test_context.test_otel_trace_across_fork'):
     - Directory: /snapshots
     - CI mode: 0
     - Trace File: /snapshots/tests.opentelemetry.test_context.test_otel_trace_across_fork.json
     - Stats File: /snapshots/tests.opentelemetry.test_context.test_otel_trace_across_fork_tracestats.json
     At compare of 1 expected trace(s) to 1 received trace(s):
      At trace 'internal' (2 spans):
 Received fewer spans (1) than expected (2). Expected unmatched spans: 'internal'

test_otel_trace_multiple_coroutines - test_context.py - Details

Expand for error

 At request <Request GET /test/session/snapshot >:
    At snapshot (token='tests.opentelemetry.test_context.test_otel_trace_multiple_coroutines'):
     - Directory: /snapshots
     - CI mode: 0
     - Trace File: /snapshots/tests.opentelemetry.test_context.test_otel_trace_multiple_coroutines.json
     - Stats File: /snapshots/tests.opentelemetry.test_context.test_otel_trace_multiple_coroutines_tracestats.json
     At compare of 1 expected trace(s) to 1 received trace(s):
      At trace 'internal' (5 spans):
 Received fewer spans (1) than expected (5). Expected unmatched spans: 'internal', 'internal', 'internal', 'internal'

test_sampling_decisions_across_processes[manual.drop] - test_context.py - Details

Expand for error

 At request <Request GET /test/session/snapshot >:
    At snapshot (token='tests.opentelemetry.test_context.test_sampling_decisions_across_processes[manual.drop]'):
     - Directory: /snapshots
     - CI mode: 0
     - Trace File: /snapshots/tests.opentelemetry.test_context.test_sampling_decisions_across_processes[manual.drop].json
     - Stats File: /snapshots/tests.opentelemetry.test_context.test_sampling_decisions_across_processes[manual.drop]_tracestats.json
     At compare of 1 expected trace(s) to 1 received trace(s):
      At trace 'internal' (2 spans):
 Received fewer spans (1) than expected (2). Expected unmatched spans: 'internal'

ddtrace/internal/constants.py

mabdinur · 2024-08-27T15:40:45Z

ddtrace/internal/opentelemetry/context.py

                log.error(
                    "Programming ERROR: ddtrace does not support activiting spans with the type: %s. Please open a "
                    "github issue at: https://github.com/Datadog/dd-trace-py and set DD_TRACE_OTEL_ENABLED=True.",
                    type(otel_span),
                )

+            # get current open telemetry baggage and store it on the datadog context object


Can we add test coverage for baggage to tests/opentelemetry/test_context.py

bump on this, we need a test case. We should use this: https://github.com/open-telemetry/opentelemetry-python/blob/bd51fcb7a3afb08aec975e7302c9cc36060bcacc/opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py#L29

This test will look similar to: https://github.com/DataDog/dd-trace-py/blob/af9d095c8955e92a318ec78eeb8d9f3143e625c3/tests/opentelemetry/test_trace.py#L162-#L184

Use this to get the baggage set by the propagator: https://github.com/open-telemetry/opentelemetry-python/blob/bd51fcb7a3afb08aec975e7302c9cc36060bcacc/opentelemetry-api/src/opentelemetry/baggage/__init__.py#L53C5-L53C16

We should also make sure the opentelemetry baggage api getters and setters work as expected and create/set/remove values on datadog context objects: https://github.com/open-telemetry/opentelemetry-python/blob/bd51fcb7a3afb08aec975e7302c9cc36060bcacc/opentelemetry-api/src/opentelemetry/baggage/__init__.py#L53-#L117

mabdinur · 2024-08-27T15:41:08Z

ddtrace/internal/opentelemetry/context.py

+            dd_baggage = {}
+
+        # getting current active baggage
+        for key, value in dd_baggage.items():


Same here we should add test coverage for this

ddtrace/propagation/http.py

mabdinur · 2024-08-27T16:29:02Z

ddtrace/propagation/http.py

+            return
+
+        header_value = ",".join(
+            f"{_BaggageHeader._encode_key(str(key).strip())}={_BaggageHeader._encode_value(str(value).strip())}"


Should we raise an unhandled exception if value can not be converted to a string? How should we handle dicts,arrays,booleans,etc. The format needs to be consistent across languages (ex: encoding True vs true)

When "extracting baggage from propagation headers, they may encounter malformed header contents." When this occurs, we "should ignore the entire header." (RFC) So instead of an error, we could potentially just ignore it? As of now, I think it would just do something like a type error

Should we raise an unhandled exception if value can not be converted to a string?

I don't know who errors are handles in Python, but an invalid header should not break the service we are instrumenting. We could definitely log a warning or whatever you usually do in cases like this.

"should ignore the entire header." (RFC)

The main point here was that we should not try to extract individual values while ignore that bad ones. If something is wrong, don't try to extract anything. I clear this up in the RFC, thanks.

ddtrace/propagation/http.py

mabdinur · 2024-08-27T16:35:12Z

ddtrace/propagation/http.py

+        for key_value in baggages:
+            key, value = key_value.split("=", 1)
+            key = urllib.parse.unquote(key.strip())
+            value = urllib.parse.unquote(value.strip())


How should this deconding/unquoting handle errors?

ddtrace/_trace/span.py

ddtrace/propagation/http.py

mabdinur · 2024-09-10T19:31:12Z

ddtrace/propagation/http.py

+            if style_w_ctx[0] == _PROPAGATION_STYLE_BAGGAGE:
+                baggage_context = contexts[0]
+                contexts.append(baggage_context)
+                del contexts[0]
+                styles_w_ctx.append(_PROPAGATION_STYLE_BAGGAGE)
+                del style_w_ctx[0]


Can we handle this outside of the for-loop? baggage should never be the primary_context

In the RFC it says that users can also disable other propagation styles (tracecontext or datadog) and enable only the baggage propagator which means baggage could be the primary_context in certain cases. Considering this, I think we can leave the code inside the for-loop. Let me know your thoughts because I think there are may be better ways to go about this.

datadog-datadog-prod-us1 · 2024-09-11T20:26:01Z

ddtrace/propagation/http.py

@@ -885,12 +887,55 @@ def _inject(span_context, headers):
        return headers


+class _BaggageHeader:


⚪ Code Quality Violation

Class _BaggageHeader should have an init method (...read more)

Ensure that a class has an __init__ method. This check is bypassed when the class is a data class (annotated with @DataClass).

datadog-datadog-prod-us1 · 2024-09-11T20:26:02Z

ddtrace/propagation/http.py

+            if style_w_ctx == _PROPAGATION_STYLE_BAGGAGE:
+                primary_context._baggage.update(context._baggage)


🔴 Code Quality Violation

too many nesting levels (...read more)

Avoid to nest too many loops together. Having too many loops make your code harder to understand.
Prefer to organize your code in functions and unit of code you can clearly understand.

Learn More

Computer Programming wikibooks - Minimize nesting

ddtrace/propagation/http.py

WIP: otel baggage support initial

d7e6d78

rachelyangdog requested review from a team as code owners August 26, 2024 20:28

rachelyangdog requested review from zacharycmontoya, juanjux and emmettbutler August 26, 2024 20:28

rachelyangdog marked this pull request as draft August 26, 2024 20:28

adding otel context

8384630

rachelyangdog added 2 commits August 27, 2024 10:03

lint

ce0e420

spelling errors

36d68d3

mabdinur reviewed Aug 27, 2024

View reviewed changes

ddtrace/internal/constants.py Show resolved Hide resolved

mabdinur reviewed Aug 27, 2024

View reviewed changes

ddtrace/propagation/http.py Outdated Show resolved Hide resolved

mabdinur reviewed Aug 27, 2024

View reviewed changes

ddtrace/propagation/http.py Show resolved Hide resolved

mabdinur reviewed Aug 27, 2024

View reviewed changes

safe_characters

8adbfd7