Translate a sampling.priority tag set to a positive value to forceKeep#11621
Conversation
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
dougqh
left a comment
There was a problem hiding this comment.
[Claude Code review]
The fix looks correct — routing positive sampling.priority values through forceKeep() instead of setSamplingPriority() is the right approach and makes it consistent with how manual.keep already behaves.
However, there is no test covering the specific regression case this PR is meant to fix. Please add a test to TagInterceptorTest (or equivalent) that pins the following scenario:
- Create a span with a propagated USER_DROP context (simulating upstream inheritance of
x-datadog-sampling-priority: -1) - Confirm the sampling priority is locked at -1
- Call
setTag("sampling.priority", 2)(i.e. a positive value) - Assert that
forceKeepis now true and the span would be kept
Without this, the exact silent-failure case from the original bug is not pinned and could regress undetected.
|
Test is coming - that's why this is still in draft ;) |
616060b to
459bc4b
Compare
…p, overriding any locked sampling priority
459bc4b to
6e7ee5c
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
fd3ed8b
into
master
What Does This Do
Translate a
sampling.prioritytag set to a positive value toforceKeep, overriding any locked sampling priorityMotivation
Since 1.34.0
sampling.prioritytags were translated tosetSamplingPrioritybut this won't override a previously locked sampling decision. When thesampling.prioritytag has a positive value we now translate that toforceKeepto ensure the trace segment is kept regardless of any previous sampling decision.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: APMS-19721