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

fix(opentelemetry): support attribute argument in TracerProvider.get_tracer() #9941

Merged
merged 9 commits into from
Jul 26, 2024

Conversation

mabdinur
Copy link
Contributor

@mabdinur mabdinur commented Jul 25, 2024

Resolves incompatibility introduced by: open-telemetry/opentelemetry-python@d4e13bd.

In opentelemetry-api==1.26.0, ddtrace.opentelemetry.TracerProvider.get_tracer(...) gets called with the attributes argument (here). This optional argument is not currently supported in ddtrace and this raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations for ddtrace.opentelemetry.TracerProvider.get_tracer, one for opentelemetry-api>=1.26 and one for opentelemetry<1.26.

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

Copy link
Contributor

github-actions bot commented Jul 25, 2024

CODEOWNERS have been resolved as:

.riot/requirements/8dea090.txt                                          @DataDog/apm-python
.riot/requirements/9b3b6c2.txt                                          @DataDog/apm-python
.riot/requirements/a0f2001.txt                                          @DataDog/apm-python
.riot/requirements/b1fd8ec.txt                                          @DataDog/apm-python
.riot/requirements/c3e8b1a.txt                                          @DataDog/apm-python
releasenotes/notes/fix-otel-get-tracer-args-0195cb8c1f7614ac.yaml       @DataDog/apm-python
.riot/requirements/16af7e0.txt                                          @DataDog/apm-python
.riot/requirements/1aaf6f6.txt                                          @DataDog/apm-python
.riot/requirements/1fa807e.txt                                          @DataDog/apm-python
.riot/requirements/dd525d9.txt                                          @DataDog/apm-python
ddtrace/opentelemetry/_trace.py                                         @DataDog/apm-sdk-api-python
riotfile.py                                                             @DataDog/apm-python
.riot/requirements/1153ad9.txt                                          @DataDog/apm-python
.riot/requirements/118cb50.txt                                          @DataDog/apm-python
.riot/requirements/135aac0.txt                                          @DataDog/apm-python
.riot/requirements/1438a95.txt                                          @DataDog/apm-python
.riot/requirements/14f0b34.txt                                          @DataDog/apm-python
.riot/requirements/15235b0.txt                                          @DataDog/apm-python
.riot/requirements/17a929f.txt                                          @DataDog/apm-python
.riot/requirements/196d465.txt                                          @DataDog/apm-python
.riot/requirements/1aa3044.txt                                          @DataDog/apm-python
.riot/requirements/1c47005.txt                                          @DataDog/apm-python
.riot/requirements/1e1ea62.txt                                          @DataDog/apm-python
.riot/requirements/1f3d2c7.txt                                          @DataDog/apm-python
.riot/requirements/1fbf1f2.txt                                          @DataDog/apm-python
.riot/requirements/427c22a.txt                                          @DataDog/apm-python
.riot/requirements/5be696d.txt                                          @DataDog/apm-python
.riot/requirements/5e63315.txt                                          @DataDog/apm-python
.riot/requirements/69997b1.txt                                          @DataDog/apm-python
.riot/requirements/7c104f7.txt                                          @DataDog/apm-python
.riot/requirements/85c90b4.txt                                          @DataDog/apm-python
.riot/requirements/a8351f1.txt                                          @DataDog/apm-python
.riot/requirements/d825572.txt                                          @DataDog/apm-python
.riot/requirements/e20bbeb.txt                                          @DataDog/apm-python
.riot/requirements/f19daa4.txt                                          @DataDog/apm-python

@mabdinur mabdinur force-pushed the munir/fix-otel-get-tracer-args branch from 0a09212 to eb9921b Compare July 25, 2024 17:00
@cbeauchesne
Copy link
Contributor

Jira to close once fixed -> https://datadoghq.atlassian.net/browse/APMAPI-180

And also PR to revert DataDog/system-tests#2818

@pr-commenter
Copy link

pr-commenter bot commented Jul 25, 2024

Benchmarks

Benchmark execution time: 2024-07-26 03:13:14

Comparing candidate commit c18b655 in PR branch munir/fix-otel-get-tracer-args with baseline commit 3237351 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
Copy link

datadog-dd-trace-py-rkomorn bot commented Jul 25, 2024

Datadog Report

Branch report: munir/fix-otel-get-tracer-args
Commit report: c18b655
Test service: dd-trace-py

✅ 0 Failed, 175922 Passed, 1737 Skipped, 10h 0m 27.29s Total duration (2m 22.34s time saved)

@mabdinur mabdinur marked this pull request as ready for review July 25, 2024 18:17
@mabdinur mabdinur requested review from a team as code owners July 25, 2024 18:17
@mabdinur mabdinur requested review from juanjux and duncanista and removed request for a team July 25, 2024 18:17
@mabdinur
Copy link
Contributor Author

Blocked by: #9929

Base automatically changed from munir/otel-fix-circular-imports to main July 25, 2024 21:38
@circleci-checks circleci-checks bot requested a review from a team as a code owner July 25, 2024 21:38
@mabdinur mabdinur enabled auto-merge (squash) July 26, 2024 02:26
@mabdinur mabdinur force-pushed the munir/fix-otel-get-tracer-args branch from c18b655 to a2fb1f3 Compare July 26, 2024 02:53
@mabdinur mabdinur merged commit ffe2201 into main Jul 26, 2024
173 of 174 checks passed
@mabdinur mabdinur deleted the munir/fix-otel-get-tracer-args branch July 26, 2024 03:22
@doudoujay
Copy link

@mabdinur hello there I'm wondering why pinning to 1.25.0 wouldnt fix this?

# 1.26.0 is broken with dtrace.opentelemetry.TracerProvider.get_tracer 
# https://github.com/DataDog/dd-trace-py/pull/9941
opentelemetry-api==1.25.0

my dockerfile:

# Base image
FROM python:3.10

# Datadog https://docs.datadoghq.com/serverless/google_cloud_run?code-lang=python
# https://registry.hub.docker.com/r/datadog/serverless-init
COPY --from=datadog/serverless-init:1.2.5 /datadog-init /app/datadog-init
RUN pip install --target /dd_tracer/python/ ddtrace
.....

After the docker build i still got

sudo docker run --rm mycontainer:latest pip list | grep opentelemetry-api
opentelemetry-api                             1.26.0

@doudoujay
Copy link

To other DD_TRACE_OTEL_ENABLED trace user, I found the only temp solution is to disable DD_TRACE_OTEL_ENABLED before the release is pushed to https://pypi.org/project/ddtrace/

gnufede pushed a commit that referenced this pull request Jul 26, 2024
…tracer() (#9941)

Resolves incompatibility introduced by:
open-telemetry/opentelemetry-python@d4e13bd.

In `opentelemetry-api==1.26.0`,
`ddtrace.opentelemetry.TracerProvider.get_tracer(...)` gets called with
attributes argument
([here](https://github.com/open-telemetry/opentelemetry-python/blob/v1.26.0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L519)).
This optional argument is not currently supported in ddtrace and this
raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations
for `ddtrace.opentelemetry.TracerProvider.get_tracer`, one for
opentelemetry-api>=1.26 and one for opentelemetry<1.26.

## Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
@mabdinur
Copy link
Contributor Author

To other DD_TRACE_OTEL_ENABLED trace user, I found the only temp solution is to disable DD_TRACE_OTEL_ENABLED before the release is pushed to https://pypi.org/project/ddtrace/

This change was merged yesterday and it resolves an incompatibility introduced in opentelemetry-api v1.25.0: #9929

Try using v1.24.0. Opentelemetry-api<=1.24.0 should compatible with all recent versions of ddtrace.

Copy link
Contributor

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-9941-to-2.10
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ffe2201787a252edbab704f9556800d7adfdd33e
# Push it to GitHub
git push --set-upstream origin backport-9941-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-9941-to-2.10.

Copy link
Contributor

The backport to 2.8 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.8 2.8
# Navigate to the new working tree
cd .worktrees/backport-2.8
# Create a new branch
git switch --create backport-9941-to-2.8
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ffe2201787a252edbab704f9556800d7adfdd33e
# Push it to GitHub
git push --set-upstream origin backport-9941-to-2.8
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.8

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

mabdinur added a commit that referenced this pull request Jul 26, 2024
…tracer() (#9941)

Resolves incompatibility introduced by:
open-telemetry/opentelemetry-python@d4e13bd.

In `opentelemetry-api==1.26.0`,
`ddtrace.opentelemetry.TracerProvider.get_tracer(...)` gets called with
attributes argument
([here](https://github.com/open-telemetry/opentelemetry-python/blob/v1.26.0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L519)).
This optional argument is not currently supported in ddtrace and this
raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations
for `ddtrace.opentelemetry.TracerProvider.get_tracer`, one for
opentelemetry-api>=1.26 and one for opentelemetry<1.26.

- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
mabdinur added a commit that referenced this pull request Jul 26, 2024
…tracer() (#9941)

Resolves incompatibility introduced by:
open-telemetry/opentelemetry-python@d4e13bd.

In `opentelemetry-api==1.26.0`,
`ddtrace.opentelemetry.TracerProvider.get_tracer(...)` gets called with
attributes argument
([here](https://github.com/open-telemetry/opentelemetry-python/blob/v1.26.0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L519)).
This optional argument is not currently supported in ddtrace and this
raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations
for `ddtrace.opentelemetry.TracerProvider.get_tracer`, one for
opentelemetry-api>=1.26 and one for opentelemetry<1.26.

- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
mabdinur added a commit that referenced this pull request Jul 26, 2024
…tracer() (backport #9941 to 2.10) (#9963)

Resolves incompatibility introduced by:

open-telemetry/opentelemetry-python@d4e13bd.

In `opentelemetry-api==1.26.0`,
`ddtrace.opentelemetry.TracerProvider.get_tracer(...)` gets called with
attributes argument

([here](https://github.com/open-telemetry/opentelemetry-python/blob/v1.26.0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L519)).
This optional argument is not currently supported in ddtrace and this
raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations
for `ddtrace.opentelemetry.TracerProvider.get_tracer`, one for
opentelemetry-api>=1.26 and one for opentelemetry<1.26.

## Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
mabdinur added a commit that referenced this pull request Jul 27, 2024
mabdinur added a commit that referenced this pull request Aug 5, 2024
…tracer() (#9941)

Resolves incompatibility introduced by:
open-telemetry/opentelemetry-python@d4e13bd.

In `opentelemetry-api==1.26.0`,
`ddtrace.opentelemetry.TracerProvider.get_tracer(...)` gets called with
attributes argument
([here](https://github.com/open-telemetry/opentelemetry-python/blob/v1.26.0/opentelemetry-api/src/opentelemetry/trace/__init__.py#L519)).
This optional argument is not currently supported in ddtrace and this
raises a TypeError when ddtrace-opentelemetry support is enabled.

This PR resolves this incompatibility by providing two implementations
for `ddtrace.opentelemetry.TracerProvider.get_tracer`, one for
opentelemetry-api>=1.26 and one for opentelemetry<1.26.

- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
mabdinur added a commit that referenced this pull request Aug 6, 2024
…tracer() [backport #9941 to 2.8] (#10083)

Backports #9941

## Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
- The change includes or references documentation updates if necessary
- Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))

## Reviewer Checklist
- [x] 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](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
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](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants