fix(opentelemetry): resolve circular imports

[mabdinur]

Resolves: #9917

Background

ddtrace replaces the default opentelemetry RuntimeContext with a datadog-aware runtime context. This allows the ddtrace library to correlate datadog and opentelemetry spans in the same trace.

The OpenTelemetry API exposes an entrypoint for overriding the default RuntimeContext (ContextVarsRuntimeContext). The ddtrace library implements this entrypoint and overrides ContextVarsRuntimeContext with DDRuntimeContext.

When DD_TRACE_OTEL_ENABLED=True and opentelemetry-api==1.25.0 is used the following occurs:

-> ddtrace imports set_tracer_provider from openelemetry.trace
-> On import openelemetry.trace the opentelemetry.context package is imported.
-> On import opentelemetry.context the _RUNTIME_CONTEXT object is initialized this calls the _load_runtime_context method.
-> On _load_runtime_context the DDRuntimeContext class is imported from ddtrace.opentelemetry._context.
-> On import ddtrace.opentelemetry._context the opentelemetry.trace package is loaded. This packaged is used in DDRuntimeContext to access types and functions used to generate OpenTelemetry compatible spans.
-> On import opentelemetry.trace the opentelemetry.context.__init__ module is indirectly imported.
-> BOOM circular import
-> _load_runtime_context handles the raised exception,DDRuntimeContext is not set, and the default ContextVarsRuntimeContext is used. This breaks span parenting and ddtrace opentelemetry support.

What changed in opentelemetry-api==1.25.0?

Previously, _load_runtime_context was lazily loaded and _RuntimeContext was initialized when the first span was created. This changed in this commit: open-telemetry/opentelemetry-python@52abb61. With this change the _RuntimeContext is ALWAYS initialized as a side effect of importing the opentelemetry.trace package.

Description

Since _load_runtime_context is no longer lazy loaded we must ensure opentelemetry.context is NOT imported as a side effect of importing DDRuntimeContext. We achieve this behavior by moving the ddtrace.opentelemetry._context.py module and inlining problematic imports.

Regression Test

Run the ddtrace opentelemetry testsuite with opentelemetry-api==1.25.0 (currently we use 1.22.0). This should be sufficient to reproduce this bug.

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]

CODEOWNERS have been resolved as:

releasenotes/notes/fix-otel-circular-imports-66f6c926d8626751.yaml      @DataDog/apm-python
.github/CODEOWNERS                                                      @DataDog/python-guild @DataDog/apm-core-python
.riot/requirements/1029814.txt                                          @DataDog/apm-python
.riot/requirements/10f03b7.txt                                          @DataDog/apm-python
.riot/requirements/14a0494.txt                                          @DataDog/apm-python
.riot/requirements/1634a62.txt                                          @DataDog/apm-python
.riot/requirements/1d3d6ad.txt                                          @DataDog/apm-python
.riot/requirements/4de07e7.txt                                          @DataDog/apm-python
.riot/requirements/59f93d3.txt                                          @DataDog/apm-python
.riot/requirements/82d4fd1.txt                                          @DataDog/apm-python
.riot/requirements/a16e5c1.txt                                          @DataDog/apm-python
.riot/requirements/a367077.txt                                          @DataDog/apm-python
.riot/requirements/a3e6122.txt                                          @DataDog/apm-python
.riot/requirements/a4a20ae.txt                                          @DataDog/apm-python
.riot/requirements/ca367e6.txt                                          @DataDog/apm-python
.riot/requirements/ea8be54.txt                                          @DataDog/apm-python
.riot/requirements/efe1fdc.txt                                          @DataDog/apm-python
pyproject.toml                                                          @DataDog/python-guild
riotfile.py                                                             @DataDog/apm-python
tests/.suitespec.json                                                   @DataDog/python-guild @DataDog/apm-core-python
tests/opentelemetry/conftest.py                                         @DataDog/apm-sdk-api-python
ddtrace/internal/opentelemetry/context.py                               @DataDog/apm-sdk-api-python

[pr-commenter]

Benchmarks

Benchmark execution time: 2024-07-25 19:11:01

Comparing candidate commit 59a3247 in PR branch munir/otel-fix-circular-imports with baseline commit 27cd94c in branch main.

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

scenario:otelspan-start

• 🟥 execution_time [+6.543ms; +7.867ms] or [+23.317%; +28.033%]
• 🟩 max_rss_usage [-15.487MB; -15.422MB] or [-34.002%; -33.859%]

scenario:otelspan-start-finish

• 🟥 execution_time [+8.468ms; +9.563ms] or [+11.570%; +13.066%]

scenario:otelspan-start-finish-telemetry

• 🟥 execution_time [+8.263ms; +9.306ms] or [+11.028%; +12.420%]

[brettlangdon]

Do we need to get rid of ddtrace/opentelemetry/_context.py ?

[mabdinur]

Do we need to get rid of ddtrace/opentelemetry/_context.py ?

I renamed ddtrace/opentelemetry/_context.py to ddtrace/internal/opentelemetry/context.py.

ddtrace/opentelemetry/init.py exposes the ddtrace TracerProvider to users. Users are expected to use this class to enable opentelemetry support. The ddtrace TracerProvider imports from the opentelemetry.trace package and this triggers this circular import. The easiest way to avoid breaking our public api and resolving the circular import is to move the DDRuntimeContext object from the ddtrace.opentelemetry package.

This is definitely a hacky workaround. Ideally we should reach out to opentelemetry-python maintainers and ask them to revisit this change: open-telemetry/opentelemetry-python@52abb61

[datadog-dd-trace-py-rkomorn]

Datadog Report

Branch report: munir/otel-fix-circular-imports
Commit report: 6ab64fc
Test service: dd-trace-py

✅ 0 Failed, 118157 Passed, 60122 Skipped, 3h 54m 26.84s Total duration (21m 55.52s time saved)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.55%. Comparing base (5b5b702) to head (05d8b27).
Report is 2 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #9929       +/-   ##
===========================================
- Coverage   27.08%   10.55%   -16.54%     
===========================================
  Files        1385     1368       -17     
  Lines      129571   127970     -1601     
===========================================
- Hits        35093    13503    -21590     
- Misses      94478   114467    +19989     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[brettlangdon]

I renamed ddtrace/opentelemetry/_context.py to ddtrace/internal/opentelemetry/context.py.

I still see the original file there.

https://github.com/DataDog/dd-trace-py/blob/05d8b274ec7b687bc0ee6221412b8997941c3aa4/ddtrace/opentelemetry/_context.py

[mabdinur]

I renamed ddtrace/opentelemetry/_context.py to ddtrace/internal/opentelemetry/context.py.

I still see the original file there.

https://github.com/DataDog/dd-trace-py/blob/05d8b274ec7b687bc0ee6221412b8997941c3aa4/ddtrace/opentelemetry/_context.py

Ahh I must've reverted a change during a rebase. The file is removed: https://github.com/DataDog/dd-trace-py/pull/9929/files#diff-62a0b204575d0586747747152ea77d0466cf05d05ca2bd5f4327478ea9583682.

For consistency we can also move ddtrace.opentelemetry._span and ddtrace.opentelemetry._trace to ddtrace/internal/opentelemetry. I can include this refactor in a future PR.

[ZStriker19]

Thanks for updating codeowners and suitespec too

[erikayasuda]

Just some NB nits. Thanks for the great description!

[github-actions]

The backport to 2.9 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.9 2.9
# Navigate to the new working tree
cd .worktrees/backport-2.9
# Create a new branch
git switch --create backport-9929-to-2.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3237351b4a35dfd52e3d3d05c81bfe5b51279372
# Push it to GitHub
git push --set-upstream origin backport-9929-to-2.9
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.9

Then, create a pull request where the base branch is 2.9 and the compare/head branch is backport-9929-to-2.9.

[github-actions]

The backport to 2.10 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.10 2.10
# Navigate to the new working tree
cd .worktrees/backport-2.10
# Create a new branch
git switch --create backport-9929-to-2.10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 3237351b4a35dfd52e3d3d05c81bfe5b51279372
# Push it to GitHub
git push --set-upstream origin backport-9929-to-2.10
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.10

Then, create a pull request where the base branch is 2.10 and the compare/head branch is backport-9929-to-2.10.