Skip to content

fix(ddtrace/tracer): keep correct trace+span sampling with propagated sampling decision#3856

Merged
dd-mergequeue[bot] merged 4 commits into
mainfrom
dario.castane/incident-41436/apmapi-1545
Aug 19, 2025
Merged

fix(ddtrace/tracer): keep correct trace+span sampling with propagated sampling decision#3856
dd-mergequeue[bot] merged 4 commits into
mainfrom
dario.castane/incident-41436/apmapi-1545

Conversation

@darccio

@darccio darccio commented Aug 7, 2025

Copy link
Copy Markdown
Member

What does this PR do?

Locks trace if sampling priority is set on context propagation.

It also fixes the precedence of global trace rate over trace sampling rules by making sure trace sampling rules are checked before than the global trace rate, as explained here:

// traceRulesSampler allows a user-defined list of rules to apply to traces.
// These rules can match based on the span's Service, Name or both.
// When making a sampling decision, the rules are checked in order until
// a match is found.
// If a match is found, the rate from that rule is used.
// If no match is found, and the DD_TRACE_SAMPLE_RATE environment variable
// was set to a valid rate, that value is used.
// Otherwise, the rules sampler didn't apply to the span, and the decision
// is passed to the priority sampler.

Note for future travelers: our internal logic for sampling works on the basis that a span can be re-sampled during the lifespan of a local span, as we allow sampling rules over tags and we support a bunch of cases where it's also allowed. It's not allowed to re-sample a propagated sampling priority in a downstream service (it'd render head-based sampling not reliable). Thus making the need to lock the trace from the root span if it already has a sampling decision in it.

Motivation

Fixes DataDog/system-tests#5014.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running ./scripts/lint.sh locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@darccio
darccio requested a review from a team as a code owner August 7, 2025 14:16
@darccio
darccio force-pushed the dario.castane/incident-41436/apmapi-1545 branch from f94ed33 to 3dc5c38 Compare August 7, 2025 14:17
Comment thread ddtrace/tracer/rules_sampler.go Outdated
@pr-commenter

pr-commenter Bot commented Aug 7, 2025

Copy link
Copy Markdown

Benchmarks

Benchmark execution time: 2025-08-19 08:29:53

Comparing candidate commit c8fdff5 in PR branch dario.castane/incident-41436/apmapi-1545 with baseline commit e1389d7 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics.

@datadog-datadog-prod-us1

datadog-datadog-prod-us1 Bot commented Aug 7, 2025

Copy link
Copy Markdown

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: c8fdff5 | Docs | Was this helpful? Give us feedback!

@darccio
darccio marked this pull request as draft August 7, 2025 14:26
@darccio
darccio force-pushed the dario.castane/incident-41436/apmapi-1545 branch 9 times, most recently from 21bc8cb to 608490a Compare August 7, 2025 20:11
@darccio
darccio requested a review from hannahkm August 7, 2025 20:38
@darccio
darccio marked this pull request as ready for review August 7, 2025 20:38
Comment thread ddtrace/tracer/tracer.go
Comment thread ddtrace/tracer/rules_sampler.go
Comment thread ddtrace/tracer/tracer.go
Comment thread ddtrace/tracer/tracer.go Outdated
@darccio
darccio requested a review from genesor August 8, 2025 13:38

@genesor genesor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread ddtrace/tracer/tracer.go Outdated
@darccio
darccio force-pushed the dario.castane/incident-41436/apmapi-1545 branch from 608490a to 18f6913 Compare August 14, 2025 15:08

@mtoffl01 mtoffl01 left a comment

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.

Approving, but with the request that we use the test logic in #3890 instead.

darccio pushed a commit that referenced this pull request Aug 19, 2025
@darccio
darccio force-pushed the dario.castane/incident-41436/apmapi-1545 branch from ea94a70 to c8fdff5 Compare August 19, 2025 08:15
@darccio

darccio commented Aug 19, 2025

Copy link
Copy Markdown
Member Author

/merge

@dd-devflow-routing-codex

dd-devflow-routing-codex Bot commented Aug 19, 2025

Copy link
Copy Markdown

View all feedbacks in Devflow UI.

2025-08-19 08:40:43 UTC ℹ️ Start processing command /merge


2025-08-19 08:40:48 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in main is approximately 14m (p90).


2025-08-19 08:51:09 UTC ℹ️ MergeQueue: This merge request was merged

@dd-mergequeue
dd-mergequeue Bot merged commit 547cf8f into main Aug 19, 2025
223 of 225 checks passed
@dd-mergequeue
dd-mergequeue Bot deleted the dario.castane/incident-41436/apmapi-1545 branch August 19, 2025 08:51
darccio added a commit that referenced this pull request Aug 19, 2025
@darccio darccio mentioned this pull request Aug 19, 2025
7 tasks
darccio pushed a commit that referenced this pull request Aug 19, 2025
@darccio darccio mentioned this pull request Aug 26, 2025
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants