ci(serverless): force reimport of modules during serverless test #11888
Conversation
This file contains hidden or 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
|
|
Datadog ReportBranch report: ✅ 0 Failed, 130 Passed, 1468 Skipped, 4m 38.31s Total duration (35m 39.08s time saved) |
BenchmarksBenchmark execution time: 2025-02-20 20:24:44 Comparing candidate commit 2657a25 in PR branch Found 0 performance improvements and 3 performance regressions! Performance is the same for 415 metrics, 2 unstable metrics. scenario:iast_aspects-ospathbasename_aspect
scenario:iast_aspects-ospathjoin_aspect
scenario:iast_aspects-ospathsplit_aspect
|
emmettbutler
approved these changes
Jan 10, 2025
07c753d to
6977fcd
Compare
6977fcd to
ee55cae
Compare
purple4reina
commented
Jan 10, 2025
Datadog ReportBranch report: ✅ 0 Failed, 130 Passed, 1184 Skipped, 3m 24.55s Total duration (25m 24.67s time saved) |
|
This change is especially tough to merge because the change to |
|
fixing the profile suite issue here #12187 |
8000
2 tasks
|
here's a fix for the llmobs failures #12191 |
…le (#12035) ## Motivation Refactors all web server integrations still using `tracer.trace` to instead use `core.context_with_data`. This is in preparation for supporting AWS API Gateway to ensure all web servers share the same code path. ## 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)
…t config (#12089) This PR makes a change to our shared distributed tracing header injection method to dispatch signals/events instead of relying on the global config settings, which is only modifiable via env vars. This fixes distributed tracing for users that might rely solely on the `LLMObs.enable()` setup config. Programmatic `LLMObs.enable()/disable()` calls do not set the global `config._llmobs_enabled` boolean setting, which is only controlled by the `DD_LLMOBS_ENABLED` env var. This was problematic for users that relied on manual `LLMObs.enable()` setup (i.e. no env vars) because our distributed tracing injection code only checks the global config to inject llmobs parent IDs into request headers. If users manually enabled LLMObs without any env vars, then this would not be reflected in the global config value and thus LLMObs parent IDs would never be injected into the request headers. We can't check directly if LLMObs is enabled in the http injection module because: 1. This would require us to import significant product-specific LLMObs-code into the shared http injector helper module which would impact non-LLMObs users' app performance 2. Circular imports in LLMObs which imports http injector logic to use in its own helpers Instead of doing our check based on the global `config._llmobs_enabled` setting, we now send a tracing event to our shared product listeners, and register a corresponding `LLMObs._inject_llmobs_context()` hook to be called for all inject() calls if LLMObs is enabled (we check the LLMObs instance, not the global config setting value). ~One risk and why I don't like changing global config settings is because this then implies that it is no longer global or tied to an env var (I want to push for env var configuration where possible over manual overriding/enabling). If a global enabled config can be toggled indiscriminately then this could open a can of worms for enabling/disabling logic in our LLMObs service, which isn't really designed to be toggled on/off multiple times in the app's lifespan. However if some users cannot rely on env vars, then I don't see any other solution that does not couple tracer internal code with LLMObs code which is a no-option.~ (UPDATE: we avoided this issue by using signal dispatching) ## 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)
ddtrace v3.0 is set to remove `ddtrace.providers` module from the public API. However we should still allow users to use their own ContextProviders. This PR ensures the BaseContextProvider remains in the public API and can be used in `tracer.configure(...)`. - [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)) - [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)
3618be1 to
502a4fb
Compare
There was a problem hiding this comment.
#12314 was merged, so the rewriting of version file should not be necessary anymore.
Co-authored-by: Brett Langdon <brett.langdon@datadoghq.com>
There are several binary files that are removed from ddtrace when it is packaged as a AWS Lambda Layer (see https://github.com/DataDog/datadog-lambda-python/blob/main/Dockerfile). This PR adds testing to ensure that none of those files are imported in certain cases. Note that the `ddtrace.contrib.requests` package currently attempts to import a removed file. This test has been marked xfail.
11 tasks
purple4reina
commented
Feb 18, 2025
87fcee0 to
2657a25
Compare
ZStriker19
reviewed
Feb 21, 2025
ZStriker19
approved these changes
Feb 21, 2025
1 task
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.
Commit 12655ee removed the
@pytest.mark.subprocess()from the serverless slow imports test. This effectively rendered the test useless asddtraceand all required modules had already been imported in the parent process.Adding the subprocess mark back will ensure that the sys modules are fresh. We must still go through and delete entries because
@pytest.mark.subprocess()relies onmultiprocessing, so it must be removed so as to ensure it can be reimported if needed.Checklist
Reviewer Checklist