Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

v2: remove sampling priority #2906

Merged
merged 3 commits into from
Oct 9, 2024
Merged

v2: remove sampling priority #2906

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b319af6
remove sampling priority
hannahkm Oct 2, 2024
0da1205
use ext constants over numbers
hannahkm Oct 3, 2024
9b50388
remove unnecessary setMetric case
hannahkm Oct 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions appsec/appsec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func TrackUserLoginSuccessEvent(ctx context.Context, uid string, md map[string]s
for k, v := range md {
span.SetTag(tagPrefix+k, v)
}
span.SetTag(ext.SamplingPriority, ext.PriorityUserKeep)
span.SetTag(ext.ManualKeep, true)
return SetUser(ctx, uid, opts...)
}

Expand All @@ -113,7 +113,7 @@ func TrackUserLoginFailureEvent(ctx context.Context, uid string, exists bool, md
for k, v := range md {
span.SetTag(tagPrefix+k, v)
}
span.SetTag(ext.SamplingPriority, ext.PriorityUserKeep)
span.SetTag(ext.ManualKeep, true)
}

// TrackCustomEvent sets a custom event as service entry span tags. This span is
Expand All @@ -130,7 +130,7 @@ func TrackCustomEvent(ctx context.Context, name string, md map[string]string) {

tagPrefix := "appsec.events." + name + "."
span.SetTag(tagPrefix+"track", true)
span.SetTag(ext.SamplingPriority, ext.PriorityUserKeep)
span.SetTag(ext.ManualKeep, true)
for k, v := range md {
span.SetTag(tagPrefix+k, v)
}
Expand Down
2 changes: 1 addition & 1 deletion contrib/dimfeld/httptreemux.v5/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func Example_withSpanOpts() {
router := httptrace.New(
httptrace.WithService("http.router"),
httptrace.WithSpanOptions(
tracer.Tag(ext.SamplingPriority, ext.PriorityUserKeep),
tracer.Tag(ext.ManualKeep, true),
),
)

Expand Down
2 changes: 1 addition & 1 deletion contrib/gorilla/mux/mux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ func TestSpanOptions(t *testing.T) {
assert := assert.New(t)
mt := mocktracer.Start()
defer mt.Stop()
mux := NewRouter(WithSpanOptions(tracer.Tag(ext.SamplingPriority, 2)))
mux := NewRouter(WithSpanOptions(tracer.Tag(ext.ManualKeep, true)))
mux.Handle("/200", okHandler()).Host("localhost")
r := httptest.NewRequest("GET", "http://localhost/200", nil)
w := httptest.NewRecorder()
Expand Down
2 changes: 1 addition & 1 deletion contrib/julienschmidt/httprouter/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func Example_withSpanOpts() {
router := httptrace.New(
httptrace.WithService("http.router"),
httptrace.WithSpanOptions(
tracer.Tag(ext.SamplingPriority, ext.PriorityUserKeep),
tracer.Tag(ext.ManualKeep, true),
),
)

Expand Down
4 changes: 0 additions & 4 deletions ddtrace/ext/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,6 @@ const (
// NetworkDestinationPort is the remote port number of the outbound connection.
NetworkDestinationPort = "network.destination.port"

// SamplingPriority is the tag that marks the sampling priority of a span.
// Deprecated in favor of ManualKeep and ManualDrop.
SamplingPriority = "sampling.priority"

// SQLType sets the sql type tag.
SQLType = "sql"

Expand Down
2 changes: 1 addition & 1 deletion ddtrace/mocktracer/mocktracer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestTracerStop(t *testing.T) {
}

func TestTracerStartSpan(t *testing.T) {
parentTags := map[string]interface{}{ext.ServiceName: "root-service", ext.SamplingPriority: -1}
parentTags := map[string]interface{}{ext.ServiceName: "root-service", ext.ManualDrop: true}
// Need to round the monotonic clock so parsed UnixNano values are equal.
// See time.Time documentation for details:
// https://pkg.go.dev/time#Time
Expand Down
8 changes: 4 additions & 4 deletions ddtrace/tracer/span.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,10 +530,10 @@ func (s *Span) setMetric(key string, v float64) {
if v == float64(samplernames.AppSec) {
s.setSamplingPriorityLocked(ext.PriorityUserKeep, samplernames.AppSec)
}
case ext.SamplingPriority:
// ext.SamplingPriority is deprecated in favor of ext.ManualKeep and ext.ManualDrop.
// We have it here for backward compatibility.
s.setSamplingPriorityLocked(int(v), samplernames.Manual)
case ext.ManualDrop:
if v == float64(samplernames.AppSec) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@Julio-Guerra Any idea why this there?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

s.setSamplingPriorityLocked(ext.PriorityUserReject, samplernames.AppSec)
Copy link
Member

Choose a reason for hiding this comment

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

@hannahkm Shouldn't we support ext.ManualKeep too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean as a case? In which case (no pun intended) it's on line 529 (it had existed already before)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I missed it. Ok, good to go! 😁

}
default:
s.metrics[key] = v
}
Expand Down
6 changes: 3 additions & 3 deletions ddtrace/tracer/span_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func TestAddSpanLink(t *testing.T) {

// Test adding a link with a sampling decision
linkedSpanSampled := newSpan("linked_sampled", "service", "res", 3, 4, 0)
linkedSpanSampled.Context().setSamplingPriority(2, samplernames.Manual)
linkedSpanSampled.Context().setSamplingPriority(ext.PriorityUserKeep, samplernames.Manual)
rootSpan.AddLink(linkedSpanSampled.Context(), map[string]string{})
assert.Equal(len(rootSpan.spanLinks), 2)
spanLinkSampled := rootSpan.spanLinks[1]
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

quick question, why default here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

s.SetTag(ext.EventSampleRate, tt.rate)
atomic.StoreInt32(&s.context.errors, tt.errors)
assert.Equal(t, shouldKeep(s), tt.want)
Expand Down Expand Up @@ -836,7 +836,7 @@ func TestSpanSamplingPriority(t *testing.T) {
ext.PriorityUserKeep,
999, // not used, but we should allow it
} {
span.SetTag(ext.SamplingPriority, priority)
span.setSamplingPriority(priority, samplernames.Default)
v, ok := span.metrics[keySamplingPriority]
assert.True(ok)
assert.EqualValues(priority, v)
Expand Down
18 changes: 9 additions & 9 deletions ddtrace/tracer/spancontext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func testAsyncSpanRace(t *testing.T) {
for i := 0; i < 100; i++ {
// The test has 100 iterations because it is not easy to reproduce the race.
t.Run("", func(t *testing.T) {
root, ctx := StartSpanFromContext(context.Background(), "root", Tag(ext.SamplingPriority, ext.PriorityUserKeep))
root, ctx := StartSpanFromContext(context.Background(), "root", Tag(ext.ManualKeep, true))
var wg sync.WaitGroup
done := make(chan struct{})
wg.Add(1)
Expand Down Expand Up @@ -116,7 +116,7 @@ func testAsyncSpanRace(t *testing.T) {
for i := 0; i < 50; i++ {
// to trigger the bug, the child should be created after the root was finished,
// as its being flushed
child, _ := StartSpanFromContext(ctx, "child", Tag(ext.SamplingPriority, ext.PriorityUserKeep))
child, _ := StartSpanFromContext(ctx, "child", Tag(ext.ManualKeep, true))
child.Finish()
}
}()
Expand Down Expand Up @@ -306,12 +306,12 @@ func TestSpanFinishPriority(t *testing.T) {

root := tracer.StartSpan(
"root",
Tag(ext.SamplingPriority, 1),
)
root.setSamplingPriority(ext.PriorityAutoKeep, samplernames.Manual)
child := tracer.StartSpan(
"child",
ChildOf(root.Context()),
Tag(ext.SamplingPriority, 2),
Tag(ext.ManualKeep, true),
)
child.Finish()
root.Finish()
Expand Down Expand Up @@ -837,35 +837,35 @@ func TestSetSamplingPriorityLocked(t *testing.T) {
tr := trace{
propagatingTags: map[string]string{},
}
tr.setSamplingPriorityLocked(0, samplernames.RemoteRate)
tr.setSamplingPriorityLocked(ext.PriorityAutoReject, samplernames.RemoteRate)
assert.Empty(t, tr.propagatingTags[keyDecisionMaker])
})
t.Run("UnknownSamplerIsIgnored", func(t *testing.T) {
tr := trace{
propagatingTags: map[string]string{},
}
tr.setSamplingPriorityLocked(0, samplernames.Unknown)
tr.setSamplingPriorityLocked(ext.PriorityAutoReject, samplernames.Unknown)
assert.Empty(t, tr.propagatingTags[keyDecisionMaker])
})
t.Run("NoPriorAndP1IsAccepted", func(t *testing.T) {
tr := trace{
propagatingTags: map[string]string{},
}
tr.setSamplingPriorityLocked(1, samplernames.RemoteRate)
tr.setSamplingPriorityLocked(ext.PriorityAutoKeep, samplernames.RemoteRate)
assert.Equal(t, "-2", tr.propagatingTags[keyDecisionMaker])
})
t.Run("PriorAndP1AndSameDMIsIgnored", func(t *testing.T) {
tr := trace{
propagatingTags: map[string]string{keyDecisionMaker: "-1"},
}
tr.setSamplingPriorityLocked(1, samplernames.AgentRate)
tr.setSamplingPriorityLocked(ext.PriorityAutoKeep, samplernames.AgentRate)
assert.Equal(t, "-1", tr.propagatingTags[keyDecisionMaker])
})
t.Run("PriorAndP1DifferentDMAccepted", func(t *testing.T) {
tr := trace{
propagatingTags: map[string]string{keyDecisionMaker: "-1"},
}
tr.setSamplingPriorityLocked(1, samplernames.RemoteRate)
tr.setSamplingPriorityLocked(ext.PriorityAutoKeep, samplernames.RemoteRate)
assert.Equal(t, "-2", tr.propagatingTags[keyDecisionMaker])
})
}
Expand Down
5 changes: 3 additions & 2 deletions ddtrace/tracer/sqlcomment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/DataDog/dd-trace-go/v2/ddtrace/ext"
"github.com/DataDog/dd-trace-go/v2/internal/globalconfig"
"github.com/DataDog/dd-trace-go/v2/internal/samplernames"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -180,7 +181,7 @@ func TestSQLCommentCarrier(t *testing.T) {
if tc.injectSpan {
traceID = uint64(10)
root := tracer.StartSpan("service.calling.db", WithSpanID(traceID))
root.SetTag(ext.SamplingPriority, tc.samplingPriority)
root.setSamplingPriority(tc.samplingPriority, samplernames.Default)
spanCtx = root.Context()
}

Expand Down Expand Up @@ -346,7 +347,7 @@ func BenchmarkSQLCommentExtraction(b *testing.B) {
func setupBenchmark() (*tracer, *SpanContext, SQLCommentCarrier) {
tracer, _ := newTracer(WithService("whiskey-service !#$%&'()*+,/:;=?@[]"), WithEnv("test-env"), WithServiceVersion("1.0.0"))
root := tracer.StartSpan("service.calling.db", WithSpanID(10))
root.SetTag(ext.SamplingPriority, 2)
root.SetTag(ext.ManualKeep, true)
spanCtx := root.Context()
carrier := SQLCommentCarrier{Query: "SELECT 1 FROM dual", Mode: DBMPropagationModeFull, DBServiceName: "whiskey-db"}
return tracer, spanCtx, carrier
Expand Down
8 changes: 4 additions & 4 deletions ddtrace/tracer/textmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -717,9 +717,9 @@ func (*propagatorB3SingleHeader) extractTextMap(reader TextMapReader) (*SpanCont
case "":
break
case "1", "d": // Treat 'debug' traces as priority 1
ctx.setSamplingPriority(1, samplernames.Unknown)
ctx.setSamplingPriority(ext.PriorityAutoKeep, samplernames.Unknown)
case "0":
ctx.setSamplingPriority(0, samplernames.Unknown)
ctx.setSamplingPriority(ext.PriorityAutoReject, samplernames.Unknown)
default:
return ErrSpanContextCorrupted
}
Expand Down Expand Up @@ -1201,11 +1201,11 @@ func parseTracestate(ctx *SpanContext, header string) {
}
if parentP == 1 && stateP <= 0 {
// Auto keep (1) and set the decision maker to default
ctx.setSamplingPriority(1, samplernames.Default)
ctx.setSamplingPriority(ext.PriorityAutoKeep, samplernames.Default)
}
if parentP == 0 && stateP > 0 {
// Auto drop (0) and drop the decision maker
ctx.setSamplingPriority(0, samplernames.Unknown)
ctx.setSamplingPriority(ext.PriorityAutoReject, samplernames.Unknown)
dropDM = true
}
} else if key == "p" {
Expand Down
24 changes: 12 additions & 12 deletions ddtrace/tracer/textmap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ func TestTextMapPropagatorInjectHeader(t *testing.T) {

root := tracer.StartSpan("web.request")
root.SetBaggageItem("item", "x")
root.SetTag(ext.SamplingPriority, 0)
root.setSamplingPriority(ext.PriorityAutoReject, samplernames.Default)
ctx := root.Context()
headers := http.Header{}

Expand Down Expand Up @@ -411,7 +411,7 @@ func Test257CharacterDDTracestateLengh(t *testing.T) {
defer tracer.Stop()
assert := assert.New(t)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, ext.PriorityUserKeep)
root.SetTag(ext.ManualKeep, true)
ctx := root.Context()
ctx.origin = "rum"
ctx.traceID = traceIDFrom64Bits(1)
Expand Down Expand Up @@ -574,7 +574,7 @@ func TestTextMapPropagator(t *testing.T) {
defer tracer.Stop()
assert.NoError(t, err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
headers := TextMapCarrier(map[string]string{})
Expand Down Expand Up @@ -1053,7 +1053,7 @@ func TestEnvVars(t *testing.T) {
defer tracer.Stop()
assert.NoError(t, err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(tc.in[0])
Expand Down Expand Up @@ -1573,7 +1573,7 @@ func TestEnvVars(t *testing.T) {
assert := assert.New(t)
assert.Nil(err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, tc.priority)
root.setSamplingPriority(tc.priority, samplernames.Default)
ctx := root.Context()
ctx.origin = tc.origin
ctx.traceID = tc.tid
Expand Down Expand Up @@ -1603,7 +1603,7 @@ func TestEnvVars(t *testing.T) {
assert := assert.New(t)
assert.Nil(err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, ext.PriorityUserKeep)
root.SetTag(ext.ManualKeep, true)
ctx := root.Context()
ctx.origin = "old_tracestate"
ctx.traceID = traceIDFrom64Bits(1229782938247303442)
Expand Down Expand Up @@ -1962,7 +1962,7 @@ func TestNonePropagator(t *testing.T) {
defer tracer.Stop()
assert.NoError(t, err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(1)
Expand All @@ -1985,7 +1985,7 @@ func TestNonePropagator(t *testing.T) {
// reinitializing to capture log output, since propagators are parsed before logger is set
tracer.config.propagator = NewPropagator(&PropagatorConfig{})
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(1)
Expand All @@ -2008,7 +2008,7 @@ func TestNonePropagator(t *testing.T) {
defer tracer.Stop()
assert.NoError(err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
headers := TextMapCarrier(map[string]string{})

Expand All @@ -2025,7 +2025,7 @@ func TestNonePropagator(t *testing.T) {
defer tracer.Stop()
assert.NoError(t, err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(1)
Expand All @@ -2046,7 +2046,7 @@ func TestNonePropagator(t *testing.T) {
assert.NoError(t, err)
defer tracer.Stop()
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(1)
Expand All @@ -2070,7 +2070,7 @@ func TestNonePropagator(t *testing.T) {
defer tracer.Stop()
assert.NoError(t, err)
root := tracer.StartSpan("web.request")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
root.SetBaggageItem("item", "x")
ctx := root.Context()
ctx.traceID = traceIDFrom64Bits(1)
Expand Down
6 changes: 3 additions & 3 deletions ddtrace/tracer/tracer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ func TestTracerStartSpan(t *testing.T) {
tracer, err := newTracer()
defer tracer.Stop()
assert.NoError(t, err)
span := tracer.StartSpan("web.request", Tag(ext.SamplingPriority, ext.PriorityUserKeep))
span := tracer.StartSpan("web.request", Tag(ext.ManualKeep, true))
assert.Equal(t, float64(ext.PriorityUserKeep), span.metrics[keySamplingPriority])
assert.Equal(t, "-4", span.context.trace.propagatingTags[keyDecisionMaker])
})
Expand Down Expand Up @@ -892,7 +892,7 @@ func TestPropagationDefaults(t *testing.T) {
assert.Nil(err)
root := tracer.StartSpan("web.request")
root.SetBaggageItem("x", "y")
root.SetTag(ext.SamplingPriority, -1)
root.SetTag(ext.ManualDrop, true)
ctx := root.Context()
headers := http.Header{}

Expand Down Expand Up @@ -935,7 +935,7 @@ func TestTracerSamplingPriorityPropagation(t *testing.T) {
tracer, err := newTracer()
defer tracer.Stop()
assert.Nil(err)
root := tracer.StartSpan("web.request", Tag(ext.SamplingPriority, 2))
root := tracer.StartSpan("web.request", Tag(ext.ManualKeep, true))
child := tracer.StartSpan("db.query", ChildOf(root.Context()))
assert.EqualValues(2, root.metrics[keySamplingPriority])
assert.Equal("-4", root.context.trace.propagatingTags[keyDecisionMaker])
Expand Down
Loading