Skip to content

Commit

Permalink
fix: pytest deadlock when using gevent [backport 2.8] (#9166)
Browse files Browse the repository at this point in the history
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)
  • Loading branch information
brettlangdon committed May 7, 2024
1 parent d8a5718 commit a5ee561
Show file tree
Hide file tree
Showing 18 changed files with 235 additions and 138 deletions.
22 changes: 0 additions & 22 deletions .riot/requirements/17fe3fa.txt

This file was deleted.

29 changes: 29 additions & 0 deletions .riot/requirements/1b90fc9.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#
# This file is autogenerated by pip-compile with Python 3.10
# by the following command:
#
# pip-compile --no-annotate --resolver=backtracking .riot/requirements/1b90fc9.in
#
attrs==23.2.0
coverage[toml]==7.5.0
exceptiongroup==1.2.1
gevent==24.2.1
greenlet==3.0.3
hypothesis==6.45.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.8
opentracing==2.4.0
packaging==24.0
pluggy==1.5.0
pytest==8.2.0
pytest-cov==5.0.0
pytest-mock==3.14.0
pytest-randomly==3.15.0
sortedcontainers==2.4.0
tomli==2.0.1
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
24 changes: 0 additions & 24 deletions .riot/requirements/1d12360.txt

This file was deleted.

24 changes: 0 additions & 24 deletions .riot/requirements/1d45085.txt

This file was deleted.

20 changes: 0 additions & 20 deletions .riot/requirements/1ea9ac5.txt

This file was deleted.

31 changes: 31 additions & 0 deletions .riot/requirements/27d0ff3.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#
# This file is autogenerated by pip-compile with Python 3.8
# by the following command:
#
# pip-compile --no-annotate .riot/requirements/27d0ff3.in
#
attrs==23.2.0
coverage[toml]==7.5.0
exceptiongroup==1.2.1
gevent==24.2.1
greenlet==3.0.3
hypothesis==6.45.0
importlib-metadata==7.1.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.8
opentracing==2.4.0
packaging==24.0
pluggy==1.5.0
pytest==8.2.0
pytest-cov==5.0.0
pytest-mock==3.14.0
pytest-randomly==3.15.0
sortedcontainers==2.4.0
tomli==2.0.1
zipp==3.18.1
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
17 changes: 12 additions & 5 deletions .riot/requirements/11fe1f6.txt → .riot/requirements/2d19e52.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,31 @@
# This file is autogenerated by pip-compile with Python 3.7
# by the following command:
#
# pip-compile --config=pyproject.toml --no-annotate --resolver=backtracking .riot/requirements/11fe1f6.in
# pip-compile --config=pyproject.toml --no-annotate --resolver=backtracking .riot/requirements/2d19e52.in
#
attrs==23.1.0
attrs==23.2.0
coverage[toml]==7.2.7
exceptiongroup==1.2.0
exceptiongroup==1.2.1
gevent==22.10.2
greenlet==3.0.3
hypothesis==6.45.0
importlib-metadata==6.7.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.5
opentracing==2.4.0
packaging==23.2
packaging==24.0
pluggy==1.2.0
pytest==7.4.3
pytest==7.4.4
pytest-cov==4.1.0
pytest-mock==3.11.1
pytest-randomly==3.12.0
sortedcontainers==2.4.0
tomli==2.0.1
typing-extensions==4.7.1
zipp==3.15.0
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
31 changes: 31 additions & 0 deletions .riot/requirements/61891b4.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#
# This file is autogenerated by pip-compile with Python 3.9
# by the following command:
#
# pip-compile --no-annotate .riot/requirements/61891b4.in
#
attrs==23.2.0
coverage[toml]==7.5.0
exceptiongroup==1.2.1
gevent==24.2.1
greenlet==3.0.3
hypothesis==6.45.0
importlib-metadata==7.1.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.8
opentracing==2.4.0
packaging==24.0
pluggy==1.5.0
pytest==8.2.0
pytest-cov==5.0.0
pytest-mock==3.14.0
pytest-randomly==3.15.0
sortedcontainers==2.4.0
tomli==2.0.1
zipp==3.18.1
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
27 changes: 27 additions & 0 deletions .riot/requirements/d171c08.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#
# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
# pip-compile --no-annotate .riot/requirements/d171c08.in
#
attrs==23.2.0
coverage[toml]==7.5.0
gevent==24.2.1
greenlet==3.0.3
hypothesis==6.45.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.8
opentracing==2.4.0
packaging==24.0
pluggy==1.5.0
pytest==8.2.0
pytest-cov==5.0.0
pytest-mock==3.14.0
pytest-randomly==3.15.0
sortedcontainers==2.4.0
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
20 changes: 0 additions & 20 deletions .riot/requirements/daaf281.txt

This file was deleted.

27 changes: 27 additions & 0 deletions .riot/requirements/de578a7.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
#
# This file is autogenerated by pip-compile with Python 3.12
# by the following command:
#
# pip-compile --no-annotate .riot/requirements/de578a7.in
#
attrs==23.2.0
coverage[toml]==7.5.0
gevent==24.2.1
greenlet==3.0.3
hypothesis==6.45.0
iniconfig==2.0.0
mock==5.1.0
msgpack==1.0.8
opentracing==2.4.0
packaging==24.0
pluggy==1.5.0
pytest==8.2.0
pytest-cov==5.0.0
pytest-mock==3.14.0
pytest-randomly==3.15.0
sortedcontainers==2.4.0
zope-event==5.0
zope-interface==6.3

# The following packages are considered to be unsafe in a requirements file:
# setuptools
10 changes: 0 additions & 10 deletions ddtrace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,6 @@

from .settings import _config as config

if config._telemetry_enabled:
from ddtrace.internal import telemetry

telemetry.install_excepthook()
# In order to support 3.12, we start the writer upon initialization.
# See https://github.com/python/cpython/pull/104826.
# Telemetry events will only be sent after the `app-started` is queued.
# This will occur when the agent writer starts.
telemetry.telemetry_writer.enable()

from ._monkey import patch # noqa: E402
from ._monkey import patch_all # noqa: E402
from .internal.utils.deprecations import DDTraceDeprecationWarning # noqa: E402
Expand Down
12 changes: 12 additions & 0 deletions ddtrace/bootstrap/preload.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,18 @@ def register_post_preload(func: t.Callable) -> None:
log = get_logger(__name__)


# Enable telemetry writer and excepthook as early as possible to ensure we capture any exceptions from initialization
if config._telemetry_enabled:
from ddtrace.internal import telemetry

telemetry.install_excepthook()
# In order to support 3.12, we start the writer upon initialization.
# See https://github.com/python/cpython/pull/104826.
# Telemetry events will only be sent after the `app-started` is queued.
# This will occur when the agent writer starts.
telemetry.telemetry_writer.enable()


if profiling_config.enabled:
log.debug("profiler enabled via environment variable")
try:
Expand Down
13 changes: 9 additions & 4 deletions ddtrace/internal/runtime/runtime_metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,13 @@
import attr

import ddtrace
from ddtrace import config
from ddtrace.internal import atexit
from ddtrace.internal import forksafe

from .. import periodic
from .. import telemetry
from ..dogstatsd import get_dogstatsd_client
from ..logger import get_logger
from ..telemetry.constants import TELEMETRY_RUNTIMEMETRICS_ENABLED
from .constants import DEFAULT_RUNTIME_METRICS
from .metric_collectors import GCRuntimeMetricCollector
from .metric_collectors import PSUtilRuntimeMetricCollector
Expand All @@ -24,6 +23,10 @@

log = get_logger(__name__)

if config._telemetry_enabled:
import ddtrace.internal.telemetry as telemetry
from ddtrace.internal.telemetry.constants import TELEMETRY_RUNTIMEMETRICS_ENABLED


class RuntimeCollectorsIterable(object):
def __init__(self, enabled=None):
Expand Down Expand Up @@ -108,7 +111,8 @@ def disable(cls):
cls.enabled = False

# Report status to telemetry
telemetry.telemetry_writer.add_configuration(TELEMETRY_RUNTIMEMETRICS_ENABLED, False, origin="unknown")
if config._telemetry_enabled:
telemetry.telemetry_writer.add_configuration(TELEMETRY_RUNTIMEMETRICS_ENABLED, False, origin="unknown")

@classmethod
def _restart(cls):
Expand All @@ -135,7 +139,8 @@ def enable(cls, flush_interval=None, tracer=None, dogstatsd_url=None):
cls.enabled = True

# Report status to telemetry
telemetry.telemetry_writer.add_configuration(TELEMETRY_RUNTIMEMETRICS_ENABLED, True, origin="unknown")
if config._telemetry_enabled:
telemetry.telemetry_writer.add_configuration(TELEMETRY_RUNTIMEMETRICS_ENABLED, True, origin="unknown")

def flush(self):
# type: () -> None
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
fixes:
- |
telemetry: 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.
1 change: 1 addition & 0 deletions riotfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -2615,6 +2615,7 @@ def select_pys(min_version=MIN_PYTHON_VERSION, max_version=MAX_PYTHON_VERSION):
"msgpack": latest,
"coverage": latest,
"pytest-randomly": latest,
"gevent": latest,
},
),
Venv(
Expand Down

0 comments on commit a5ee561

Please sign in to comment.