-
Notifications
You must be signed in to change notification settings - Fork 493
fix(ddtrace/opentelemetry): fix ignored sampling decision from otel #4238
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -166,3 +166,21 @@ func (c *otelCtxToDDCtx) SpanID() uint64 { | |
| } | ||
|
|
||
| func (c *otelCtxToDDCtx) ForeachBaggageItem(_ func(k, v string) bool) {} | ||
|
|
||
| // SamplingDecision returns the sampling decision of the span context. | ||
| func (c *otelCtxToDDCtx) SamplingDecision() uint32 { | ||
| if c.oc.IsSampled() { | ||
| return 2 // decisionKeep | ||
| } | ||
| return 1 // decisionDrop | ||
| } | ||
|
|
||
| // Priority returns the sampling priority of the span context. | ||
| func (c *otelCtxToDDCtx) Priority() *float64 { | ||
| if c.oc.IsSampled() { | ||
| p := float64(ext.PriorityAutoKeep) | ||
| return &p | ||
| } | ||
| p := float64(ext.PriorityAutoReject) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above |
||
| return &p | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -398,3 +398,24 @@ func TestMergeOtelDDBaggage(t *testing.T) { | |
| assert.Equal("otelValue", value) | ||
| }) | ||
| } | ||
|
|
||
| func Test_DDOpenTelemetryTracer(t *testing.T) { | ||
| ddOTelTracer := NewTracerProvider( | ||
| tracer.WithSamplingRules([]tracer.SamplingRule{ | ||
| {Rate: 0}, // This should be applied only when a brand new root span is started and should be ignored for a non-root span | ||
| }), | ||
| ).Tracer("") | ||
|
|
||
| parentSpanContext := oteltrace.NewSpanContext(oteltrace.SpanContextConfig{ | ||
| TraceID: oteltrace.TraceID{0xAA}, | ||
| SpanID: oteltrace.SpanID{0x01}, | ||
| TraceFlags: oteltrace.FlagsSampled, // the parent span is sampled, so its child spans should be sampled too | ||
| }) | ||
| ctx := oteltrace.ContextWithSpanContext(context.Background(), parentSpanContext) | ||
| _, span := ddOTelTracer.Start(ctx, "test") | ||
| span.End() | ||
|
|
||
| childSpanContext := span.SpanContext() | ||
| assert.Equal(t, parentSpanContext.TraceID(), childSpanContext.TraceID()) | ||
| assert.True(t, childSpanContext.IsSampled(), "parent span is sampled, but child span is not sampled") // this test fails | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you want to delete the comment on this line 😄
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be nice to have additional tests that assert the values of IsSampled have alignment with the values returned by new methods SamplingDecision() and Priority() |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://pkg.go.dev/go.opentelemetry.io/otel/trace#TraceFlags.IsSampled
The logic is fuzzy on this one. I would interpret this to mean: if c.oc.IsSampled() is false, then "we don't know anything about the sampling decision," instead of "the sampling decision is to drop."