remove python sampling tag test skips#1535
Closed
emmettbutler wants to merge 5 commits intomainfrom
Closed
Conversation
change DD_TRACE_RATE_LIMIT=0 test to expect sampling_priority -1
16 tasks
emmettbutler
added a commit
to DataDog/dd-trace-py
that referenced
this pull request
Sep 5, 2023
This change makes the tracer's sampling tagging behavior conform to the expectations described in DataDog/system-tests#1487 ## How to review Use [this view](https://github.com/DataDog/dd-trace-py/pull/6693/files?file-filters%5B%5D=.py&show-viewed-files=true) to look at the changes to Python files, since it's a lot more responsive in the browser than the default. 1. Read these [system-tests](https://github.com/DataDog/system-tests/blob/8b0bdc3392792fa45fb97845f777688abb052a85/tests/parametric/test_sampling_span_tags.py) to understand the behavior they validate. 2. Read `tests/integration/test_sampling.py` to see the library tests that perform the same validations. 3. Look at `snapshots/test_sampling_with*` to see how test expectations have changed in light of these validations. 4. Examine `ddtrace/sampler.py` to see how it implements this behavior, especially the function call `from_datadog_sampler`. 4. Glance at some of the hundreds of other snapshots to confirm that their changes make sense in light of what you've learned. ## Change Summary Filter the file list by `snapshots/test_sampling_with` to find the snapshot files that directly validate the behavior tested by [these system-tests](https://github.com/DataDog/system-tests/blob/8b0bdc3392792fa45fb97845f777688abb052a85/tests/parametric/test_sampling_span_tags.py#L74). Changes to these snapshot files include: * Removing the `.dm` tag from spans not first in their chunks * Adding the `.dm` tag to first-in-chunk spans that had their sampling decision set manually (`-4`) or by the tracer (`-3`) * Removing the `_psr` tags from spans to which it doesn't apply * Adding three test cases that involve the `DD_TRACE_RATE_LIMIT` configuration My intent is that each of these snapshot file changes maps directly on to one of the "reasons" listed for the test skips in the [system-tests](https://github.com/DataDog/system-tests/blob/8b0bdc3392792fa45fb97845f777688abb052a85/tests/parametric/test_sampling_span_tags.py#L74). Find the corresponding test code for each snapshot in `tests/integration/test_sampling.py`. `ddtrace/tracer.py` is changed to avoid propagating the `.dm` tag to child spans that exist in the same execution as their parents. `ddtrace/span.py` is changed to remove `_psr` tags from spans to which they don't apply. For example, `_dd.p.dm: -1` indicates that the sampling decision was made on the basis of a sample rate from the agent, which is the only case in which `agent_psr` is meaningful. When `dm != -1`, `agent_psr` should not be set. There are significant changes to `ddtrace/sampler.py`. The most fundamental of these is the [change](https://github.com/DataDog/dd-trace-py/pull/6693/files?file-filters%5B%5D=.py&show-viewed-files=true#diff-51bda2f09879ed6e54b15cc9da5ed1da04023d3465018dd7794d35384dd85409L357-R356) from the use of inheritance between sampler classes to the use of dependency injection via the new `from_datadog_sampler` method. This fixes a subtle bug in which executions of `sample()` were calling methods of subclasses rather than the intended superclass methods. Avoiding the use of `super()` sidesteps the situation that led to this issue and makes it straightforward to adjust the logic that chooses between the various sampler implementations. This also allows the removal of some hard-to-follow `isinstance` and `super` calls. In `sampler.py`, rate limiting is applied unconditionally as a second stage after a sampling decision has been made. In `sampler.py`, the `agent_psr` tag is only applied when a sample rate received from the agent was actually used to make the sampling decision. All changes other than the ones listed here are changes to test expectations to make existing tests conform to the new behavior. A majority of these expectation changes were made by a script that * Removed the `.dm` tag from any span with `parent_id != 0` * Removed the `agent_psr` tag from any span with `_dd.p.dm != -1` ## Risk Because these changes conform to the behavior in the system-tests, I think their risk is pretty low. The worst-case scenario is that this change causes inaccurate metrics in Datadog products, in which case it can be reverted. The performance impact indicated by the bot comment is [probably](#6693 (review)) a result of the removal of an early exit from `sample()`. In my view it's worthwhile to trade CPU time for code clarity in this case. ## Other Stuff Once this is included in a release, we can promote DataDog/system-tests#1535 to "ready for review". I've run those tests locally and told them to install the tracer from this branch. ## 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] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [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)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly 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)
Collaborator
|
Hi @emmettbutler, do you still plan to work on this ? |
Contributor
Author
|
@cbeauchesne nope, sorry I forgot to close this. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 pull request removes the skips for Python and Python HTTP from most of the tests in
tests/parametric/test_sampling_span_tags.py. It also adjusts an expectation that had been erroneously committed related to the precedence order of sampling rules and rate limits.I've run this locally against DataDog/dd-trace-py#6693 and seen that the tests pass.
Motivation
DataDog/dd-trace-py#6693 aligns the Python tracer's behavior with most of the assertions in this file. Since this change
iswill be in the latest release, we can keep this system-tests suite passing without needing to skip most tests for Python.Workflow
Once your PR is reviewed, you can merge it! ❤️
Reviewer checklist
run-parametric-scenario,run-profiling-scenario...). If this PR modifies any system-tests internal, then add therun-all-scenarioslabel (more info).build-some-imagelabel is present