fix(tracing): lazy importing asyncio should not reset existing trace context #9272

joeyzhao2018 · 2024-05-15T16:13:06Z

This PR introduces two main changes:

Do not reset _DD_CONTEXTVAR when initializing a DefaultContextProvider
Explicitly clear _DD_CONTEXTVAR after every test in dd-trace-py CI
1. We also emit a warning, so pytest will show them to us

Motivation

Currently, when

asyncio auto instrumentation is patched
and there's an existing trace context
and asyncio is imported
Then the existing trace context will be reset to None.
The expected behavior is that the existing trace context should not be reset for this kind of lazy imports

Checklist

Change(s) are motivated and described in the PR description
Testing strategy is described if automated tests are not included in the PR
Risks are described (performance impact, potential for breakage, maintainability)
Change is maintainable (easy to change, telemetry, documentation)
Library release note guidelines are followed or label changelog/no-changelog is set
Documentation is included (in-code, generated user docs, public corp docs)
Backport labels are set (if applicable)
If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

Title is accurate
All changes are related to the pull request's stated goal
Description motivates each change
Avoids breaking API changes
Testing strategy adequately addresses listed risks
Change is maintainable (easy to change, telemetry, documentation)
Release note makes sense to a user of the library
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

datadog-dd-trace-py-rkomorn · 2024-05-15T16:54:17Z

Datadog Report

Branch report: joey/context_reset_issue
Commit report: 50baee9
Test service: dd-trace-py

✅ 0 Failed, 132808 Passed, 43116 Skipped, 6h 33m 22.44s Total duration (3h 11m 36.04s time saved)

pr-commenter · 2024-05-15T17:07:26Z

Benchmarks

Benchmark execution time: 2024-05-21 18:25:16

Comparing candidate commit 573acd3 in PR branch joey/context_reset_issue with baseline commit 3c618ab in branch main.

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

tests/conftest.py

codecov-commenter · 2024-05-20T19:35:49Z

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 10.42%. Comparing base (e605bd3) to head (e2d9812).
Report is 1 commits behind head on main.

Files	Patch %	Lines
tests/contrib/asyncio/test_lazyimport.py	0.00%	12 Missing ⚠️
tests/conftest.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #9272       +/-   ##
===========================================
- Coverage   76.22%   10.42%   -65.81%     
===========================================
  Files        1289     1260       -29     
  Lines      122509   120684     -1825     
===========================================
- Hits        93386    12582    -80804     
- Misses      29123   108102    +78979

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

…ontext (#9272) This PR introduces two main changes: 1. Do not reset `_DD_CONTEXTVAR` when initializing a `DefaultContextProvider` 2. Explicitly clear `_DD_CONTEXTVAR` after every test in dd-trace-py CI 1. We also emit a warning, so pytest will show them to us ## Motivation Currently, when 1. asyncio auto instrumentation is patched 5. and there's an existing trace context 6. and `asyncio` is imported Then the existing trace context will be reset to None. The expected behavior is that the existing trace context should not be reset for this kind of lazy imports ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [ ] Title is accurate - [ ] All changes are related to the pull request's stated goal - [ ] Description motivates each change - [ ] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [ ] Testing strategy adequately addresses listed risks - [ ] Change is maintainable (easy to change, telemetry, documentation) - [ ] Release note makes sense to a user of the library - [ ] 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) --------- Co-authored-by: Brett Langdon <[email protected]> (cherry picked from commit 06b8494)

…ontext (#9272) This PR introduces two main changes: 1. Do not reset `_DD_CONTEXTVAR` when initializing a `DefaultContextProvider` 2. Explicitly clear `_DD_CONTEXTVAR` after every test in dd-trace-py CI 1. We also emit a warning, so pytest will show them to us ## Motivation Currently, when 1. asyncio auto instrumentation is patched 5. and there's an existing trace context 6. and `asyncio` is imported Then the existing trace context will be reset to None. The expected behavior is that the existing trace context should not be reset for this kind of lazy imports ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [ ] Title is accurate - [ ] All changes are related to the pull request's stated goal - [ ] Description motivates each change - [ ] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [ ] Testing strategy adequately addresses listed risks - [ ] Change is maintainable (easy to change, telemetry, documentation) - [ ] Release note makes sense to a user of the library - [ ] 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) --------- Co-authored-by: Brett Langdon <[email protected]>

…context [backport 2.9] (#9336) Backport 06b8494 from #9272 to 2.9. This PR introduces two main changes: 1. Do not reset `_DD_CONTEXTVAR` when initializing a `DefaultContextProvider` 2. Explicitly clear `_DD_CONTEXTVAR` after every test in dd-trace-py CI 1. We also emit a warning, so pytest will show them to us ## Motivation Currently, when 1. asyncio auto instrumentation is patched 5. and there's an existing trace context 6. and `asyncio` is imported Then the existing trace context will be reset to None. The expected behavior is that the existing trace context should not be reset for this kind of lazy imports ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) Co-authored-by: Joey Zhao <[email protected]>

…context [backport 2.8] (#9335) Backport 06b8494 from #9272 to 2.8. This PR introduces two main changes: 1. Do not reset `_DD_CONTEXTVAR` when initializing a `DefaultContextProvider` 2. Explicitly clear `_DD_CONTEXTVAR` after every test in dd-trace-py CI 1. We also emit a warning, so pytest will show them to us ## Motivation Currently, when 1. asyncio auto instrumentation is patched 5. and there's an existing trace context 6. and `asyncio` is imported Then the existing trace context will be reset to None. The expected behavior is that the existing trace context should not be reset for this kind of lazy imports ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) Co-authored-by: Joey Zhao <[email protected]>

…context [backport 2.7] (#9334) Backport 06b8494 from #9272 to 2.7. This PR introduces two main changes: 1. Do not reset `_DD_CONTEXTVAR` when initializing a `DefaultContextProvider` 2. Explicitly clear `_DD_CONTEXTVAR` after every test in dd-trace-py CI 1. We also emit a warning, so pytest will show them to us ## Motivation Currently, when 1. asyncio auto instrumentation is patched 5. and there's an existing trace context 6. and `asyncio` is imported Then the existing trace context will be reset to None. The expected behavior is that the existing trace context should not be reset for this kind of lazy imports ## Checklist - [x] Change(s) are motivated and described in the PR description - [x] Testing strategy is described if automated tests are not included in the PR - [x] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)) - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - [x] 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) Co-authored-by: Joey Zhao <[email protected]>

joeyzhao2018 added serverless changelog/no-changelog A changelog entry is not required for this PR. labels May 15, 2024

joeyzhao2018 changed the title ~~Lazy importing asyncio should not reset existing trace context~~ fix(internal) lazy importing asyncio should not reset existing trace context May 15, 2024

joeyzhao2018 added 2 commits May 15, 2024 12:25

Lazy importing asyncio should not reset existing trace context

b5a4620

link

cc0861f

joeyzhao2018 force-pushed the joey/context_reset_issue branch from 8c81cb3 to cc0861f Compare May 15, 2024 16:26

joeyzhao2018 removed the serverless label May 15, 2024

joeyzhao2018 changed the title ~~fix(internal) lazy importing asyncio should not reset existing trace context~~ fix(tracing) lazy importing asyncio should not reset existing trace context May 15, 2024

joeyzhao2018 mentioned this pull request May 15, 2024

[SVLS-4797] safety check before create cold start spans DataDog/datadog-lambda-python#485

Merged

11 tasks

linting

5576876

brettlangdon force-pushed the joey/context_reset_issue branch from 5db7e8d to bc74883 Compare May 20, 2024 14:01

add assertion that every test leaves the context empty

cf7d0da

brettlangdon force-pushed the joey/context_reset_issue branch from bc74883 to cf7d0da Compare May 20, 2024 14:16

brettlangdon added 4 commits May 20, 2024 13:03

Merge remote-tracking branch 'origin/main' into joey/context_reset_issue

43294d2

just assume each test meant to clear it's context

597284c

fix lazy import regression test

d238a4a

warn when context is not empty

83d01f7

brettlangdon removed the changelog/no-changelog A changelog entry is not required for this PR. label May 20, 2024

add release note

30df2b5

brettlangdon marked this pull request as ready for review May 20, 2024 17:55

brettlangdon requested review from a team as code owners May 20, 2024 17:55

brettlangdon requested review from emmettbutler and romainkomorn-exdatadog May 20, 2024 17:55

Merge branch 'main' into joey/context_reset_issue

b1999bd

brettlangdon enabled auto-merge (squash) May 20, 2024 17:55

joeyzhao2018 commented May 20, 2024

View reviewed changes

tests/conftest.py Show resolved Hide resolved

brettlangdon changed the title ~~fix(tracing) lazy importing asyncio should not reset existing trace context~~ fix(tracing): lazy importing asyncio should not reset existing trace context May 20, 2024

brettlangdon added 2 commits May 20, 2024 15:13

fix lazyimport test

44f0416

Merge remote-tracking branch 'origin/main' into joey/context_reset_issue

9a3d583

romainkomorn-exdatadog approved these changes May 21, 2024

View reviewed changes

emmettbutler added the backport 2.9 label May 21, 2024

emmettbutler approved these changes May 21, 2024

View reviewed changes

emmettbutler added backport 2.7 labels May 21, 2024

brettlangdon added 4 commits May 21, 2024 12:57

Merge branch 'main' into joey/context_reset_issue

6d5c542

Merge branch 'main' into joey/context_reset_issue

e2d9812

Merge branch 'main' into joey/context_reset_issue

573acd3

Merge branch 'main' into joey/context_reset_issue

50baee9

brettlangdon merged commit 06b8494 into main May 21, 2024
159 checks passed

brettlangdon deleted the joey/context_reset_issue branch May 21, 2024 18:29

github-actions bot mentioned this pull request May 21, 2024

fix(tracing): lazy importing asyncio should not reset existing trace context [backport 2.7] #9334

Merged

17 tasks

github-actions bot mentioned this pull request May 21, 2024

fix(tracing): lazy importing asyncio should not reset existing trace context [backport 2.8] #9335

Merged

17 tasks

github-actions bot mentioned this pull request May 21, 2024

fix(tracing): lazy importing asyncio should not reset existing trace context [backport 2.9] #9336

Merged

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tracing): lazy importing asyncio should not reset existing trace context #9272

fix(tracing): lazy importing asyncio should not reset existing trace context #9272

joeyzhao2018 commented May 15, 2024 •

edited by emmettbutler

Loading

datadog-dd-trace-py-rkomorn bot commented May 15, 2024 •

edited

Loading

pr-commenter bot commented May 15, 2024 •

edited

Loading

codecov-commenter commented May 20, 2024 •

edited

Loading

fix(tracing): lazy importing asyncio should not reset existing trace context #9272

fix(tracing): lazy importing asyncio should not reset existing trace context #9272

Conversation

joeyzhao2018 commented May 15, 2024 • edited by emmettbutler Loading

Motivation

Checklist

Reviewer Checklist

datadog-dd-trace-py-rkomorn bot commented May 15, 2024 • edited Loading

Datadog Report

pr-commenter bot commented May 15, 2024 • edited Loading

Benchmarks

codecov-commenter commented May 20, 2024 • edited Loading

Codecov Report

joeyzhao2018 commented May 15, 2024 •

edited by emmettbutler

Loading

datadog-dd-trace-py-rkomorn bot commented May 15, 2024 •

edited

Loading

pr-commenter bot commented May 15, 2024 •

edited

Loading

codecov-commenter commented May 20, 2024 •

edited

Loading