chore(gunicorn): add gunicorn and gevent test scenarios #4810
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yun-Kim
added
the
changelog/no-changelog
Yun-Kim
force-pushed
the
yunkim/gunicorn-test-scenarios
branch
from
2aa8eda to
05c5405
Compare
| from tests.webclient import PingFilter | ||
|
|
||
|
|
||
| DynamicInstrumentation.enable() |
There was a problem hiding this comment.
@Yun-Kim can you help me understand why you added this to the test app? I think it could play an important role in the tests I'm writing and I'm wanting to check my understanding. Thanks!
There was a problem hiding this comment.
@emmettbutler good question! I added Dynamic Instrumentation here while tinkering around with the test scenarios to see if we could see any unexpected behavior. If I recall correctly @P403n1x87 mentioned there were known issues with Dynamic Instrumentation with gevent, although I'm not sure if it was enabling Dynamic instrumentation alone that caused the issues.
There was a problem hiding this comment.
I think the idea was to start a service that would register after-fork hooks that re-start background threads. DI is an example where this happens, since it requires RCM and the uploader thread to be started soon after fork. The general pattern is also in this test
dd-trace-py/tests/internal/test_forksafe.py
Line 299 in 58249cf
Codecov Report
@@ Coverage Diff @@
## 1.x #4810 +/- ##
==========================================
+ Coverage 74.64% 74.74% +0.09%
==========================================
Files 810 817 +7
Lines 62276 63461 +1185
==========================================
+ Hits 46489 47436 +947
- Misses 15787 16025 +238
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
11 tasks
|
#4962 builds on this work. I'm going to close this PR to reduce confusion - please reopen if you want. |
emmettbutler
added a commit
that referenced
this pull request
Jan 31, 2023
…y configs (#4962) ## Description This pull request tries to answer two questions: * Is the [public documentation about gunicorn](https://ddtrace.readthedocs.io/en/stable/advanced_usage.html#gunicorn) accurate and complete? * What problematic ddtracepy behavior was [this commit](e1ab88c) fixing? It does so by implementing a series of integration tests against a real gunicorn server. Each test configures the server and ddtracepy in a different way. These tests check each configuration against known failure modes. The set of configurations that avoid all known failure modes is the one that the updated documentation recommends. There were a lot more tests exercising various buggy configurations, but I removed them to make CI run faster. Let me know if you think some of those other configurations need to be included in automated testing. By "known failure modes", I mean degraded performance, duplicated work, or crashes that I found through experimentation based on following logical threads from the relevant issues below. The big comment in the diff explains it in detail. ## Checklist - [x] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) - [ ] update these https://docs.datadoghq.com/ - [ ] update these https://app.datadoghq.com/apm/service-setup?architecture=host-based&language=python (I'll do the other documentation locations once we're in agreement about the documentation decisions made in this PR) ## Relevant issue(s) #2894 #4810 #4070 #1799 ## Testing strategy After the experimentation process that informed the integration test writing, the testing strategy is the tests themselves. ## Reviewer Checklist - [ ] Title is accurate. - [ ] Description motivates each change. - [ ] No unnecessary changes were introduced in this PR. - [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Release note has been added and follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines), or else `changelog/no-changelog` label added. - [ ] All relevant GitHub issues are correctly linked. - [ ] Change contains telemetry where appropriate (logs, metrics, etc.). - [ ] Telemetry is meaningful, actionable and does not have the potential to leak sensitive data. --------- Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Brett Langdon <[email protected]>
jbertran
pushed a commit
that referenced
this pull request
Feb 1, 2023
…y configs (#4962) ## Description This pull request tries to answer two questions: * Is the [public documentation about gunicorn](https://ddtrace.readthedocs.io/en/stable/advanced_usage.html#gunicorn) accurate and complete? * What problematic ddtracepy behavior was [this commit](e1ab88c) fixing? It does so by implementing a series of integration tests against a real gunicorn server. Each test configures the server and ddtracepy in a different way. These tests check each configuration against known failure modes. The set of configurations that avoid all known failure modes is the one that the updated documentation recommends. There were a lot more tests exercising various buggy configurations, but I removed them to make CI run faster. Let me know if you think some of those other configurations need to be included in automated testing. By "known failure modes", I mean degraded performance, duplicated work, or crashes that I found through experimentation based on following logical threads from the relevant issues below. The big comment in the diff explains it in detail. ## Checklist - [x] [Library documentation](https://github.com/DataDog/dd-trace-py/tree/1.x/docs) - [ ] update these https://docs.datadoghq.com/ - [ ] update these https://app.datadoghq.com/apm/service-setup?architecture=host-based&language=python (I'll do the other documentation locations once we're in agreement about the documentation decisions made in this PR) ## Relevant issue(s) #2894 #4810 #4070 #1799 ## Testing strategy After the experimentation process that informed the integration test writing, the testing strategy is the tests themselves. ## Reviewer Checklist - [ ] Title is accurate. - [ ] Description motivates each change. - [ ] No unnecessary changes were introduced in this PR. - [ ] Avoid breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [ ] Tests provided or description of manual testing performed is included in the code or PR. - [ ] Release note has been added and follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/contributing.html#Release-Note-Guidelines), or else `changelog/no-changelog` label added. - [ ] All relevant GitHub issues are correctly linked. - [ ] Change contains telemetry where appropriate (logs, metrics, etc.). - [ ] Telemetry is meaningful, actionable and does not have the potential to leak sensitive data. --------- Co-authored-by: Munir Abdinur <[email protected]> Co-authored-by: Brett Langdon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR builds off #4776 in adding test scenarios involving the gunicorn test fixture introduced in #4776.
This PR adds tests outlining our library's documented supported use cases with gevent workers in gunicorn servers, specifically testing the following scenarios:
DD_GEVENT_PATCH_ALL=1in theddtrace-runcommand to have gevent patched immediately.DD_GEVENT_PATCH_ALL=0in theddtrace-runcommand. This will result in unexpected behaviors due to gevent not being patched immediately.import ddtrace.bootstrap.sitecustomizein the first line of the application instead ofddtrace-runand theDD_GEVENT_PATCH_ALLenvironment variable.ddtrace.bootstrap.sitecustomize.Checklist
featandfixpull requests.Motivation
Design
Testing strategy
Relevant issue(s)
Testing strategy
Reviewer Checklist
changelog/no-changeloglabel added.