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: pytest deadlock when using gevent #9141

Merged
merged 17 commits into from
May 6, 2024

Conversation

brettlangdon
Copy link
Member

@brettlangdon brettlangdon commented May 1, 2024

Fixes #8281

This fix resolves an issue when using pytest + gevent where the telemetry writer was eager initialized by pytest entrypoints loading of our plugin causing a potential dead lock.

Reproduction

The specific case that we targeted solving with this PR:

# test.py
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()


def test_thing():
    pass
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py

Previously would dead-lock, but now will execute without problem.

Trade offs

We no longer get telemetry enabled and sending data just from import ddtrace.

Moving telemetry writer starting to ddtrace.bootstrap.preload will mean that any unhandled exceptions that occur before the telemetry writer can be started will not get reported.

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
Copy link

datadog-dd-trace-py-rkomorn bot commented May 1, 2024

Datadog Report

Branch report: brettlangdon/move.telemetry.init
Commit report: 1f8f670
Test service: dd-trace-py

✅ 0 Failed, 173833 Passed, 1133 Skipped, 11h 29m 47.21s Total duration (20m 39.68s time saved)

@brettlangdon brettlangdon marked this pull request as ready for review May 1, 2024 16:27
@brettlangdon brettlangdon requested review from a team as code owners May 1, 2024 16:27
@codecov-commenter
Copy link

codecov-commenter commented May 1, 2024

Codecov Report

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

Project coverage is 6.65%. Comparing base (3ea8bd8) to head (cdf3420).

Files Patch % Lines
tests/ci_visibility/test_cli.py 0.00% 10 Missing ⚠️
ddtrace/internal/runtime/runtime_metrics.py 0.00% 8 Missing ⚠️
tests/telemetry/test_telemetry.py 0.00% 6 Missing ⚠️
ddtrace/bootstrap/preload.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #9141       +/-   ##
===========================================
- Coverage   78.55%    6.65%   -71.90%     
===========================================
  Files        1273     1244       -29     
  Lines      120300   118535     -1765     
===========================================
- Hits        94496     7886    -86610     
- Misses      25804   110649    +84845     

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

@brettlangdon brettlangdon enabled auto-merge (squash) May 2, 2024 17:14
Copy link
Member

@tonyredondo tonyredondo left a comment

Choose a reason for hiding this comment

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

LGTM

@brettlangdon brettlangdon merged commit 9e862f9 into main May 6, 2024
172 of 174 checks passed
@brettlangdon brettlangdon deleted the brettlangdon/move.telemetry.init branch May 6, 2024 16:44
Copy link

github-actions bot commented May 6, 2024

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

github-actions bot pushed a commit that referenced this pull request May 6, 2024
Fixes #8281

This fix resolves an issue when using ``pytest`` + ``gevent`` where the
telemetry writer was eager initialized by ``pytest`` entrypoints loading
of our plugin causing a potential dead lock.

## Reproduction

The specific case that we targeted solving with this PR:

```python
# test.py
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()

def test_thing():
    pass
```

```shell
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py
```

Previously would dead-lock, but now will execute without problem.

## Trade offs

We no longer get telemetry enabled and sending data just from `import
ddtrace`.

Moving telemetry writer starting to `ddtrace.bootstrap.preload` will
mean that any unhandled exceptions that occur before the telemetry
writer can be started will not get reported.

## 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)

(cherry picked from commit 9e862f9)
brettlangdon added a commit that referenced this pull request May 6, 2024
Fixes #8281

This fix resolves an issue when using ``pytest`` + ``gevent`` where the
telemetry writer was eager initialized by ``pytest`` entrypoints loading
of our plugin causing a potential dead lock.

The specific case that we targeted solving with this PR:

```python
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()

def test_thing():
    pass
```

```shell
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py
```

Previously would dead-lock, but now will execute without problem.

We no longer get telemetry enabled and sending data just from `import
ddtrace`.

Moving telemetry writer starting to `ddtrace.bootstrap.preload` will
mean that any unhandled exceptions that occur before the telemetry
writer can be started will not get reported.

- [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`.

- [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)

(cherry picked from commit 9e862f9)
brettlangdon added a commit that referenced this pull request May 6, 2024
Fixes #8281

This fix resolves an issue when using ``pytest`` + ``gevent`` where the
telemetry writer was eager initialized by ``pytest`` entrypoints loading
of our plugin causing a potential dead lock.

## Reproduction

The specific case that we targeted solving with this PR:

```python
# test.py
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()

def test_thing():
    pass
```

```shell
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py
```

Previously would dead-lock, but now will execute without problem.

## Trade offs

We no longer get telemetry enabled and sending data just from `import
ddtrace`.

Moving telemetry writer starting to `ddtrace.bootstrap.preload` will
mean that any unhandled exceptions that occur before the telemetry
writer can be started will not get reported.

## 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)

(cherry picked from commit 9e862f9)
brettlangdon added a commit that referenced this pull request May 7, 2024
Backport
9e862f9
from #9141 to 2.8.

Fixes #8281

This fix resolves an issue when using ``pytest`` + ``gevent`` where the
telemetry writer was eager initialized by ``pytest`` entrypoints loading
of our plugin causing a potential dead lock.

The specific case that we targeted solving with this PR:

```python
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()

def test_thing():
    pass
```

```shell
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py
```

Previously would dead-lock, but now will execute without problem.

We no longer get telemetry enabled and sending data just from `import
ddtrace`.

Moving telemetry writer starting to `ddtrace.bootstrap.preload` will
mean that any unhandled exceptions that occur before the telemetry
writer can be started will not get reported.

- [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`.

- [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)
brettlangdon added a commit that referenced this pull request May 7, 2024
Backport 9e862f9 from #9141 to 2.9.

Fixes #8281

This fix resolves an issue when using ``pytest`` + ``gevent`` where the
telemetry writer was eager initialized by ``pytest`` entrypoints loading
of our plugin causing a potential dead lock.

## Reproduction

The specific case that we targeted solving with this PR:

```python
# test.py
import ddtrace

import gevent.monkey

gevent.monkey.patch_all()


def test_thing():
    pass
```

```shell
$ pytest test.py
$ pytest -p ddtrace -p ddtrace.pytest_bdd -p ddtrace.pytest_benchmark test.py
$ pytest -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark test.py
```

Previously would dead-lock, but now will execute without problem.


## Trade offs

We no longer get telemetry enabled and sending data just from `import
ddtrace`.

Moving telemetry writer starting to `ddtrace.bootstrap.preload` will
mean that any unhandled exceptions that occur before the telemetry
writer can be started will not get reported.

## 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: Brett Langdon <brett.langdon@datadoghq.com>
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.

gevent could not work with ddtrace in pytest
4 participants