Skip to content

Add test for incorrect trace+span sampling with propagated sampling decision#5014

Merged
LotharSee merged 4 commits into
mainfrom
benjamin/test_single_rule_with_head_and_rule_trace_sampling_019
Aug 12, 2025
Merged

Add test for incorrect trace+span sampling with propagated sampling decision#5014
LotharSee merged 4 commits into
mainfrom
benjamin/test_single_rule_with_head_and_rule_trace_sampling_019

Conversation

@LotharSee

@LotharSee LotharSee commented Aug 1, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

I started activating some remote rules on Datadog account for Go services, and it resulted in the incorrect flagging of ingestion_reason:remote_rule for some traces which should be ingestion_reason:rum.
After digging more, I realized trace rule were applied when they shouldn't.

The added test here isn't exhaustive but catches exactly the issue I'm facing on production. I'm not sure it is the right file to put it (since it mixes spans sampling and trace sampling) but hopefully, this isn't mattering too much.

Changes

Add test test_single_rule_with_head_and_rule_trace_sampling_keep_019 and test_single_rule_with_head_and_rule_trace_sampling_drop_020 to ensure trace sampling and span sampling applies properly when there is already a parent sampling decision.

Note: CI is currently red in Python because of TestServiceActivationEnvVarMetric.test_service_activation_metric, I'll rebase whenever it gets fixed on main.

@LotharSee
LotharSee requested review from a team and mabdinur as code owners August 1, 2025 12:39
@LotharSee
LotharSee marked this pull request as draft August 1, 2025 12:39
@LotharSee
LotharSee force-pushed the benjamin/test_single_rule_with_head_and_rule_trace_sampling_019 branch from 58693a0 to be6541d Compare August 1, 2025 12:53
@LotharSee LotharSee changed the title Add test_single_rule_with_head_and_rule_trace_sampling_019 Add test for incorrect trace+span sampling with propagated sampling decision Aug 1, 2025
@LotharSee
LotharSee force-pushed the benjamin/test_single_rule_with_head_and_rule_trace_sampling_019 branch from be6541d to ab97179 Compare August 1, 2025 13:32
LotharSee
LotharSee commented Aug 1, 2025
View reviewed changes
Comment thread tests/parametric/test_span_sampling.py Outdated
assert len(traces) == 0


@bug(context.library == "cpp", reason="APMAPI-1545")

@LotharSee LotharSee Aug 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a ticket for this issue, hopefully at the right place with the right fields
https://datadoghq.atlassian.net/browse/APMAPI-1545

@dmehala dmehala Aug 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LotharSee I added e5fe5a6 to fix the issue with the C++ tracer. For short, spans created during the test were not flushed, consequently they were not received by the agent.

@LotharSee LotharSee Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!
The CI test fails, apparently on a 404 response from trace_extract_headers. Retries didn't fix it.
Does it relate to your change? Should I keep it or skip it?

LotharSee
LotharSee commented Aug 1, 2025
View reviewed changes
Comment thread tests/parametric/test_span_sampling.py Outdated
}
],
)
def test_single_rule_with_head_and_rule_trace_sampling_019(self, test_agent, test_library):

@LotharSee LotharSee Aug 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests seem to finish with a number, not sure why, so I did so too :noideawhatiamdoing:

@LotharSee
LotharSee force-pushed the benjamin/test_single_rule_with_head_and_rule_trace_sampling_019 branch from ab97179 to 3e01524 Compare August 1, 2025 13:34
@LotharSee
LotharSee force-pushed the benjamin/test_single_rule_with_head_and_rule_trace_sampling_019 branch from 3e01524 to 7dc7e54 Compare August 4, 2025 08:45
LotharSee and others added 2 commits August 4, 2025 11:23
@LotharSee
Add test_single_rule_with_head_and_rule_trace_sampling_drop_020
6d4ed3c
@dmehala
fix: ensure spans are flushed
e5fe5a6 
The `dd_extract_headers_and_make_child_span` does not flush traces,
which was causing the test to fail for C++ because the agent does not
receive the span generated.

This commit explicitly flushes spans.

Changes:
  - Explicitly flush traces.
  - Format `test_span_sampling.py`.
@darccio darccio mentioned this pull request Aug 7, 2025
Merged
7 tasks
@LotharSee
LotharSee marked this pull request as ready for review August 11, 2025 11:24
@LotharSee
LotharSee merged commit 3ad21bc into main Aug 12, 2025
580 checks passed
@LotharSee
LotharSee deleted the benjamin/test_single_rule_with_head_and_rule_trace_sampling_019 branch August 12, 2025 13:18
@rachelyangdog rachelyangdog mentioned this pull request Aug 14, 2025
Merged
2 tasks
rachelyangdog added a commit to DataDog/dd-trace-py that referenced this pull request Aug 15, 2025
@rachelyangdog
fix(sampling): skip _dd.p.dm tag when sampling priority is propagated (…
0e32eda 
…#14308)

Fixes an issue where the trace sampling decision tag (_dd.p.dm) was
being incorrectly set during header extraction even when a sampling
priority was already propagated.

This issue was discovered when [new system-tests
](DataDog/system-tests#5014) were introduced.

## 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)
juanjux pushed a commit to DataDog/dd-trace-py that referenced this pull request Sep 2, 2025
@rachelyangdog @juanjux
fix(sampling): skip _dd.p.dm tag when sampling priority is propagated (…
e914765 
…#14308)

Fixes an issue where the trace sampling decision tag (_dd.p.dm) was
being incorrectly set during header extraction even when a sampling
priority was already propagated.

This issue was discovered when [new system-tests
](DataDog/system-tests#5014) were introduced.

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

Reviewers

@genesor genesor genesor approved these changes
@mabdinur mabdinur Awaiting requested review from mabdinur
+1 more reviewer
@dmehala dmehala dmehala left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@LotharSee @dmehala @genesor