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

Deadlock with pytest+ddtrace in py 3.11 that -p no:ddtrace does not solve #4845

Closed
olivierlefloch opened this issue Dec 29, 2022 · 15 comments
Closed
Assignees

Comments

@olivierlefloch
Copy link

Summary of problem

When adding Python 3.11 support to an existing codebase that uses ddtrace, we encounter deadlocks at the end of the test suite (after pytest reports its results) in CI. This does not occur on previous versions of Python.

Furthermore, disabling the ddtrace plugin using -p no:ddtrace does not resolve the issue.

Specifying DD_TRACE_ENABLED=false pytest … does work around the issue, however.

Which version of dd-trace-py are you using?

1.6.3

Which version of pip are you using?

22.3.1

Which libraries and their versions are you using?

datadog==0.44.0
[…]
ddtrace==1.6.3 
[…]
gevent==22.10.2
[…]
pytest==7.2.0

How can we reproduce your problem?

I have not (yet?) put together a minimal reproduction case as there seem to potentially be several issues at play here ; happy to help drill down as appropriate.

What is the result that you get?

Github Actions times out after our self imposted timeout.

What is the result that you expected?

  1. Test suite does not deadlock even with the ddtrace plugin enabled, on Python 3.11,
  2. Specifying -p no:ddtrace fully disables the plugin, including starting a global tracer.

Related issues

@mabdinur
Copy link
Contributor

mabdinur commented Dec 30, 2022

Hey @olivierlefloch,

Thanks for brining this issue to our attention 😄

Specifying -p no:ddtrace fully disables the plugin, including starting a global tracer.

I believe no:ddtrace does not disable the global tracer, it should only stop the ddtrace pytest plugin from being configured. When you set no:ddtrace do you still see test runs in the Datadog CI product?

Test suite does not deadlock even with the ddtrace plugin enabled, on Python 3.11,

I ran the dd-trace-py gevent tests in circleci with the ddtrace pytest plugin, gevent~=22.10.0, and pytest==7.2.0 but I was not able to reproduce the deadlocks (test run). I was wondering if you could supply a repro case to help us debug further. Also I have a few questions that might help us narrow dow the root cause(s):

  • Did you notice this issue after upgrading to v1.6.3 or were you experiencing this issue with earlier versions of ddtrace?

  • What platform are you running your tests on? (ex: windows-latest/macos-12/ubuntu-20.04/...)

  • Aside from pytest, gevent, and ddtrace are there any libraries you suspect are causing these deadlocks? Can you send us which package versions you're using?

  • When ddtrace is enabled how frequently does your application/tests deadlock? Do these deadlock occur in the same tests/modules?

  • Where does you application deadlock? Can you pinpoint this issue to an offending module/line/function? Can you share with us any logs in your application that you think are relevant (or a repo case with sensitive business logic redacted)?

  • Are you using the asm, ddtrace profiler or dynamic instrumentation in your application? If so, can you try setting DD_APPSEC_ENABLED=false, DD_PROFILING_ENABLED=false and/or DD_DYNAMIC_INSTRUMENTATION_ENABLED=false. This will help us isolate this issue to just the tracer.

  • Does anything in our gevent, gunicorn, or uwsgi docs stand out to you?

    • How are you enabling gevent in your application/tests?
    • How are you enabling ddtrace patching (ddtrace-run ... , import ddtrace; ddtrace.patch_all(), or import ddtrace; ddtrace.patch(gevent=True)?
    • Are you applying ddtrace patching before gevent?
    • If you are using ddtrace-run .... are you setting DD_GEVENT_PATCH_ALL=true?
      Can you try running your tests with ddtrace gevent support disabled? (ex: set the following environment variable DD_TRACE_GEVENT_ENABLED=false or remove import ddtrace; ddtrace.patch(gevent=True) from your application code)?
  • Do your tests generate traces? Are you able view these traces in the datadog product? If not, are you running the datadog agent?

  • We made some changes to how gevent

If you're more comfortable sharing this information privately you can contact our support team here and we can continue this conversation there.

@rmoneys
Copy link

rmoneys commented Jan 3, 2023

@mabdinur,

-p no:ddtrace used to block the Pytest plugin, and also the connection errors reported at the end of a test session when there's no agent to send trace data to. Now it appears to have no effect. With that option Pytest no longer omits the plugin from the list that appears at the start of a session:

% pytest -p no:ddtrace
================================================================================================ test session starts =================================================================================================
platform darwin -- Python 3.11.1, pytest-7.2.0, pluggy-1.0.0
rootdir: /Users/blah/blah/blah, configfile: pyproject.toml
plugins: ddtrace-1.6.4, anyio-3.6.2

This worked as expected with Python 3.10.3 and,

  1. ddtrace==0.56.1
  2. pytest==7.0.1
  3. pluggy==1.0.0

I tried downgrading Pytest, but this combination still fails:

  1. ddtrace==1.6.4
  2. pytest==7.0.1
  3. pluggy==1.0.0

It seems ddtrace==1.6.0 is the earliest release that I can install with Python 3.11.1 and that has the same issue as ddtrace==1.6.4: the plugin cannot be blocked with -p no:ddtrace.

@mabdinur
Copy link
Contributor

Hey,

Sorry for the late reply. ddtrace v1.3.0 introduced pytest-bdd plugin (feature). This plugin starts the global tracer and can be disabled using -p no:ddtrace.pytest_bdd.

Try running pytest with the following command and let us know if this disables the tracer:
pytest -p no:ddtrace -p no:ddtrace.pytest_bdd ....

@rmoneys
Copy link

rmoneys commented Jan 24, 2023

Awesome. Thanks!

The plugin no longer loads with,

pytest -p no:ddtrace -p no:ddtrace.pytest_bdd ...

I still get a "failed to send traces to Datadog Agent" error for tests that cover functions wrapped with tracer.wrap, but I can silence those with the environment variable, "DD_TRACE_ENABLED=False".

@fredrikaverpil
Copy link

fredrikaverpil commented Jan 24, 2023

I too have seen these "failed to send traces to Datadog Agent" for some time. The only thing that fixes it is to have the environment variable (DD_TRACE_ENABLED=False) set. This is however not something which is clear from Datadog docs and not really expected behavior when running pytest.

Related: #4179

@github-actions github-actions bot added the stale label Feb 24, 2023
@matroscoe
Copy link

matroscoe commented Aug 22, 2023

I can confirm this is still an issue with Python 3.11.4, ddtrace: 1.18.0 pytest 7.4.0 and with DD_TRACE_ENABLED=False set as an environment variable and -p no:ddtrace -p no:ddtrace.pytest_bdd being passed into the adopts under tool.pytest.ini_options in our pyproject.toml

Also an issue in ddtrace 1.18.1 which was just released.

@github-actions github-actions bot removed the stale label Aug 23, 2023
@github-actions github-actions bot added the stale label Sep 23, 2023
@matroscoe
Copy link

matroscoe commented Oct 11, 2023

This is still an issue with Python 3.11.6, ddtrace: 1.20.3 with pytest 7.4.2. All environment variables are configured in my previous comment from August 22nd 2023.

The specific error we are seeing:

Unable to export profile: ddtrace.profiling.exporter.ExportError: HTTP upload request failed: [Errno 111] Connection refused. Ignoring.

@github-actions github-actions bot removed the stale label Oct 12, 2023
@romainkomorn-exdatadog
Copy link
Contributor

I work on the pytest plugin, so I figured I'd take a look at this (and #4179, at least temporarily), although it looks like the reports here are explicitly not using the plugin, so we can get it resolved. I may need some help getting in reproducing and/or confirming current symptoms.

@matroscoe , when you say "this is still an issue", you're referring to the deadlock, or to the "failed to send traces" log entries (or both)?

@matroscoe
Copy link

@romainkomorndatadog I am specifically referring to the issue with the datadog system still being enabled and attempting to send traces with both the -p no:ddtrace -p no:ddtrace.pytest_bdd being adopted.

@romainkomorn-exdatadog
Copy link
Contributor

romainkomorn-exdatadog commented Oct 20, 2023

@matroscoe , since you're not using (or you're disabling) the ddtrace plugin for pytest, I assume you're installing ddtrace as a dependency for the purpose of using it in the application you're testing?

In the pytest case, the ddtrace module is being imported (regardless of whether or not the plugin's activated), and -p no:ddtrace won't prevent the import from happening. We can modify the plugin's behavior to only import ddtrace when the plugin's enabled and the session starts.

I don't have a repro case yet but I suspect that likely won't resolve the issue if ddtrace is being imported somewhere else in the application, so I think there's more investigating work to be done.

@matroscoe
Copy link

@romainkomorndatadog to be clear I think you are right. We have included ddtrace as part of our application (should we not be doing this?). We do not need it or want it to run during pytest (to stop the upload attempt from happening). We would be fine with ddtrace being imported and running just not attempting the upload which tends to confuse users and make it look like there is something wrong when there isn't.

Something like a -p no:ddtrace-upload would even work just to disable the upload part. The main reason I am worried about not having ddtrace attempting to run is we do benchmarking in our CI. We want ddtrace to be deployed and so we want out benchmarks to account for the impact of ddtrace being run with our code.

I think at least my companies use case would be satisfied if we could be provided with an option to specifically disable the upload attempt.

@ZStriker19
Copy link
Contributor

Hi @matroscoe could you please clarify whether or not setting DD_TRACE_ENABLED to false fixes the issue for you? Some users have reported it does, although it seems like you reported it did not? That seems very strange to me, as when that's set, we avoid calling the spanaggregator on span finish. So no traces get sent. In a way it's equivalent to filtering out all traces from ever getting uploaded.

If that's the case, that's what we'd currently recommend. We can add your request for a plugin flag (e.g. -p no:ddtrace-upload) to mimic this behavior as a feature request.

@github-actions github-actions bot added the stale label Jan 1, 2024
@jlucas91
Copy link

jlucas91 commented Jan 5, 2024

@romainkomorndatadog To chime in here - outside of the logging concerns raised by others - I cannot seem to disable the ddtrace pytest plugin at all using the solutions recommended here.

Passing in -p no:ddtrace -p no:ddtrace.pytest_bdd does not remove ddtrace from the enabled plugins list. Furthermore, in stack traces within my tests, I see ddtrace. Ex:

/usr/local/lib/python3.12/site-packages/ddtrace/internal/module.py:212: in _exec_module
    self.loader.exec_module(module)

This is on Python 3.12.1, ddtrace 2.3.2, and pytest 7.4.3.

I'm surprised to see ddtrace within the runtime of my tests by default. It's not a behavior I opted into or was clear to me when I installed ddtrace for application tracing.

@github-actions github-actions bot removed the stale label Jan 6, 2024
@romainkomorn-exdatadog
Copy link
Contributor

@jlucas91 , I think there are two (maybe three) separate issues here:

First, I did notice today that it looks like we have a problem with Python 3.10+ that I just created #8039 to look into. It looks like that's what you're reporting in your stacktrace?

Next, just to be clear (and I do realize this is splitting hairs a bit), -p no:ddtrace does disable the ddtrace plugin itself. For example:

% pytest --ddtrace -p no:ddtrace
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --ddtrace
  inifile: /Users/romain.komorn/nondd/flask-shallow/pyproject.toml
  rootdir: /Users/romain.komorn/nondd/flask-shallow

shows that the --ddtrace option doesn't get added by the plugin. It still appears in the output of pytest under the plugins (eg: plugins: ddtrace-2.4.0). ddtrace will continue to appear under the list of plugins unless you -p no: all of the available "sub" plugins (ie, as of time of writing -p no:ddtrace -p no:ddtrace.pytest_bdd -p no:ddtrace.pytest_benchmark).

And lastly, the issue that remains is that the plugin file does import ddtrace, which kicks off some bits of ddtrace machinery no matter what. That said, if you import ddtrace anywhere in your application code, you'll end up with the same behavior (eg: some of ddtrace's functionality will turn get activated by import alone).

I think it may be appropriate in this case for us to lazy-load the module (ie: only import ddtrace if the plugin is enabled). I'm also wondering if folding the functionality of the pytest_bdd and pytest_benchmark plugins into the main ddtrace one would make more sense.

@github-actions github-actions bot added the stale label Mar 9, 2024
romainkomorn-exdatadog added a commit that referenced this issue Apr 19, 2024
…is enabled (#8999)

This defers importing ddtrace in the `pytest`, `pytest-bdd`, and
`pytest-benchmark` plugins until after it's been determined that the
plugin is enabled with (eg: by running `pytest --ddtrace`).

The fixtures that use `ddtrace` defer importing until the feature is
actually requested due to the fact that fixtures imported later (eg:
after plugins have loaded) are not usable.

Even though this doesn't actually address the issues reported in
#8281 or
#4845 (due to the side
effects of `ddtrace/__init__.py`), this change is still helpful in other
ways (in particular since I'll be introducing a new version of the
`pytest` plugin shortly).

Fixtures were confirmed to still be working by:
* using `--ddtrace-patch-all`
* adding tags to spans using the `ddspan` fixture
* seeing that the `ddtracer` fixture returns the tracer object.

## 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)
@romainkomorn-exdatadog
Copy link
Contributor

Given the recent changes made for this and #8281 (comment) , I'm going to mark this as closed as well.

Since there have been multiple different (but related) issues (gevent deadlocks, logging, traces being sent, etc) discussed in both this and #8281 , I'd really appreciate it if, if someone is tempted to reopen this issue, they could instead open a new one with a single topic and details.

brettlangdon pushed a commit that referenced this issue Aug 16, 2024
…s to try-except blocks (#10196)

There are two changes in this commit:

1- remove "sub plugin" strategy introduced in #8999

The sub-plugin strategy was originally implemented to delay the import
of `ddtrace`, however that import is (in the current scenario)
inevitable because the `pyproject.toml` entry-points that tell `pytest`
where to find plugins already import `ddtrace`, and the original
motivation of the change in (#8281 and #4845) was instead addressed by
#9141 .

The motivation to revert this change is that some hooks (eg:
`pytest_load_inital_conftests`) added by the class-based plugins do not
execute when the plugin is loaded in the `pytest_configure()` hook in
the `plugin.py` file itself. All the methods in the plugin classes use
the `@staticmethod` decorator and do not benefit from being in a class.

This change allows the versioned plugin files to implement hooks by
virtue of being imported into the original plugin module.

2- move v2 functions to try-except blocks

Since CI Visibility should never affect `pytest`'s execution (or, at the
very least, should never prevent it from running to completion), the
hooks themselves should never raise exceptions. With this change, some
functions simply wrap the entire body in a try/except block, while some
others define a separate function that is then called in a try/except
block.

A decorator was not used for this purpose because some hooks yield,
while others don't, and logging relevant errors is also a little
simpler.

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

No branches or pull requests

8 participants