v2: remove sampling priority #2906
Conversation
BenchmarksBenchmark execution time: 2024-10-04 13:45:17 Comparing candidate commit 9b50388 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 0 unstable metrics. scenario:BenchmarkInjectW3C-24
|
ddtrace/tracer/span.go
Outdated
| case ext.ManualDrop: | ||
| if v == float64(samplernames.AppSec) { | ||
| s.setSamplingPriorityLocked(ext.PriorityUserReject, samplernames.AppSec) |
There was a problem hiding this comment.
@hannahkm Shouldn't we support ext.ManualKeep too?
There was a problem hiding this comment.
Do you mean as a case? In which case (no pun intended) it's on line 529 (it had existed already before)
There was a problem hiding this comment.
Oh, I missed it. Ok, good to go! 😁
| @@ -214,7 +214,7 @@ func TestShouldDrop(t *testing.T) { | |||
| } { | |||
| t.Run("", func(t *testing.T) { | |||
| s := newSpan("", "", "", 1, 1, 0) | |||
| s.SetTag(ext.SamplingPriority, tt.prio) | |||
| s.setSamplingPriority(tt.prio, samplernames.Default) | |||
There was a problem hiding this comment.
quick question, why default here?
There was a problem hiding this comment.
afaik, the value we pass into samplernames doesn't make an impact for this test case, so I opted to keep the code clean (i.e. not adding if statements or extraneous parameters to the tt structs), since we would otherwise have to check for priorities 2 or -1 to set it to Manual.
Default says that the priority was set without a sampler, which I believe still holds true in this case. Maybe I'm missing some logic here?
ddtrace/tracer/span.go
Outdated
| // We have it here for backward compatibility. | ||
| s.setSamplingPriorityLocked(int(v), samplernames.Manual) | ||
| case ext.ManualDrop: | ||
| if v == float64(samplernames.AppSec) { |
There was a problem hiding this comment.
I am not sure we need to add this here to be honest. There is a weird use case for ManualKeep, defined by appsec rules. But they don't use the same convention for manualDrop 🤔
There was a problem hiding this comment.
@Julio-Guerra Any idea why this there?
There was a problem hiding this comment.
Did you git-blame the file? It wasn't me who wrote those lines but @Barbayar a few years ago when he worked on distributed tracing on all tracers. IIRC, it was just about the decision maker tag _dd.p.dm.
ASM doesn't rely on those details, we're good as long as we can force-keep the traces when we have security events.
The only ASM RFC where we went into such details is the standalone ASM billing RFC and they are system-tested so regressions would be found if any.
There was a problem hiding this comment.
Reading the PR description, I believe it would be great to keep a lower-level API for internal usage that provides more options. But likely bonus as far as I am aware of our product needs.
There was a problem hiding this comment.
Yes, I spoke with @Barbayar and it seems that AppSec has a few cases where we want to keep specific traces, which is why I decided to keep the case. I'll keep the ManualKeep case, but seems like we don't need the ManualDrop, so I've removed it. Thanks!
eliottness
dismissed
their stale review
Would like to hear back from @Julio-Guerra on the appsec manual drop comment before merging if you don't mind
What does this PR do?
Removes support for
ext.SamplingPriority. Users should opt to usespan.SetTag(ext.ManualKeep, true)for keeping spans/traces, andspan.SetTag(ext.ManualDrop, true)for dropping spans/traces.Motivation
Re: PR #1082 ,
ext.SamplingPriorityhas been deprecated.Reviewer's Checklist
Unsure? Have a question? Request a review!