-
Notifications
You must be signed in to change notification settings - Fork 704
feat: add sampling attributes for jaeger remote sampler #8073
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 9 commits
e70276c
f53b5f8
9cc7fce
76fc443
e2ce983
8e6fd09
c110c3e
65b84a2
b862da3
b7591f4
e1a3370
e8aa3fb
dbcae9e
6cccfd9
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -24,6 +24,7 @@ import ( | |||||
| "sync" | ||||||
|
|
||||||
| jaeger_api_v2 "github.com/jaegertracing/jaeger-idl/proto-gen/api_v2" | ||||||
| "go.opentelemetry.io/otel/attribute" | ||||||
| "go.opentelemetry.io/otel/sdk/trace" | ||||||
| oteltrace "go.opentelemetry.io/otel/trace" | ||||||
|
|
||||||
|
|
@@ -34,25 +35,55 @@ const ( | |||||
| defaultMaxOperations = 2000 | ||||||
| ) | ||||||
|
|
||||||
| const ( | ||||||
| samplerTypeKey = "sampler.type" | ||||||
| samplerTypeValueProbabilistic = "probabilistic" | ||||||
| samplerTypeValueRateLimiting = "ratelimiting" | ||||||
| samplerParamKey = "sampler.param" | ||||||
| ) | ||||||
|
|
||||||
| type samplerOptions struct { | ||||||
|
||||||
| attributesOn bool | ||||||
| } | ||||||
|
|
||||||
| type samplerOptionFunc func(*samplerOptions) | ||||||
|
|
||||||
| func withAttributesOn() samplerOptionFunc { | ||||||
|
||||||
| func withAttributesOn() samplerOptionFunc { | |
| func WithAttributesDisabled() samplerOptionFunc { |
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.
I wasn't sure that we want to change current default behavior - jaegerremote does not set any attributes.
This is supposed to be an internal option for creating and updating samplers which constructors also aren't exported.
For external usage I added WithAttributesOn option.
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.
I do want to change the current behavior. The current behavior is useless, the sampler does not actually work with Jaeger's adaptive backend. And the change itself is additive, it's backwards compatible (especially since the new attributes are prefixed with jaeger.).
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.
do we need an option to disable these attributes in that case? If not implementation will be simple and no need for internal sampler options too
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.
Personally I would argue you don't even need the opt-out option, OTEL SDK is already extremely verbose in terms of how many attributes it captures, having two extra ones on root span only seems irrelevant. But for 100% backwards compat you would provide an option to opt-out.
Outdated
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.
I think this flag is only used at initialization, you don't really need to store it
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.
When probabilisticSampler or ratelimitngSampler are being updated its init method is also called, so I not sure if we can omit this flag.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package jaegerremote_test | ||
|
|
||
| import ( | ||
| "encoding/binary" | ||
| "testing" | ||
| "time" | ||
|
|
||
| jaeger_api_v2 "github.com/jaegertracing/jaeger-idl/proto-gen/api_v2" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "go.opentelemetry.io/otel/attribute" | ||
| "go.opentelemetry.io/otel/sdk/trace" | ||
| oteltrace "go.opentelemetry.io/otel/trace" | ||
|
|
||
| "go.opentelemetry.io/contrib/samplers/jaegerremote" | ||
| "go.opentelemetry.io/contrib/samplers/jaegerremote/internal/testutils" | ||
| ) | ||
|
|
||
| const ( | ||
| testDefaultSamplingProbability = 0.5 | ||
| testMaxID = uint64(1) << 63 | ||
| ) | ||
|
|
||
| func TestRemotelyControlledSampler_WithAttributesOn(t *testing.T) { | ||
| agent, err := testutils.StartMockAgent() | ||
| require.NoError(t, err) | ||
|
|
||
| remoteSampler := jaegerremote.New( | ||
| "client app", | ||
| jaegerremote.WithSamplingServerURL("http://"+agent.SamplingServerAddr()), | ||
| jaegerremote.WithSamplingRefreshInterval(time.Minute), | ||
| jaegerremote.WithAttributesOn(), | ||
| ) | ||
| remoteSampler.Close() // stop timer-based updates, we want to call them manually | ||
| defer agent.Close() | ||
|
|
||
| var traceID oteltrace.TraceID | ||
| binary.BigEndian.PutUint64(traceID[8:], testMaxID-20) | ||
|
|
||
| t.Run("probabilistic", func(t *testing.T) { | ||
| agent.AddSamplingStrategy("client app", | ||
| &jaeger_api_v2.SamplingStrategyResponse{ | ||
| StrategyType: jaeger_api_v2.SamplingStrategyType_PROBABILISTIC, | ||
| ProbabilisticSampling: &jaeger_api_v2.ProbabilisticSamplingStrategy{ | ||
| SamplingRate: testDefaultSamplingProbability, | ||
| }, | ||
| }) | ||
| remoteSampler.UpdateSampler() | ||
|
|
||
| result := remoteSampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) | ||
| assert.Equal(t, trace.RecordAndSample, result.Decision) | ||
| assert.Equal(t, []attribute.KeyValue{attribute.String("sampler.type", "probabilistic"), attribute.Float64("sampler.param", 0.5)}, result.Attributes) | ||
| }) | ||
|
|
||
| t.Run("ratelimitng", func(t *testing.T) { | ||
| agent.AddSamplingStrategy("client app", | ||
| &jaeger_api_v2.SamplingStrategyResponse{ | ||
| StrategyType: jaeger_api_v2.SamplingStrategyType_RATE_LIMITING, | ||
| RateLimitingSampling: &jaeger_api_v2.RateLimitingSamplingStrategy{ | ||
| MaxTracesPerSecond: 1, | ||
| }, | ||
| }) | ||
| remoteSampler.UpdateSampler() | ||
|
|
||
| result := remoteSampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) | ||
| assert.Equal(t, trace.RecordAndSample, result.Decision) | ||
| assert.Equal(t, []attribute.KeyValue{attribute.String("sampler.type", "ratelimiting"), attribute.Float64("sampler.param", 1)}, result.Attributes) | ||
| }) | ||
|
|
||
| t.Run("per operation", func(t *testing.T) { | ||
| agent.AddSamplingStrategy("client app", | ||
| &jaeger_api_v2.SamplingStrategyResponse{OperationSampling: &jaeger_api_v2.PerOperationSamplingStrategies{ | ||
| DefaultSamplingProbability: testDefaultSamplingProbability, | ||
| DefaultLowerBoundTracesPerSecond: 1.0, | ||
| }}) | ||
| remoteSampler.UpdateSampler() | ||
|
|
||
| result := remoteSampler.ShouldSample(trace.SamplingParameters{TraceID: traceID}) | ||
| assert.Equal(t, trace.RecordAndSample, result.Decision) | ||
| assert.Equal(t, []attribute.KeyValue{attribute.String("sampler.type", "probabilistic"), attribute.Float64("sampler.param", 0.5)}, result.Attributes) | ||
| }) | ||
| } |
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.
I think it would make sense to prefix with
jaeger., This may require small change in the backend to recognize, but it separates custom attributes into clear namespace.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.
Yeah, this is reasonable but I see some difficulties when using in mixed environments (jaeger and otel services) where will be different sampler attributes in that case. Not sure if backend can support more that one key.
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.
this is a Jaeger-specific functionality and a Jaeger-specific sampler. Since it's not setting any attributes now then nothing is depending on them.
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.
So can we keep canonical keys? I did small research and using new keys will require some overhead on collector to transform these attributes.
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.
Since those are jaeger specific, prefixing them with jaeger definitely makes sense.
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.
Added prefix