-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Traces sampler env var #6306
Traces sampler env var #6306
Changes from all commits
d0c200b
9431582
d65689f
da6eccc
4873fa3
1ac6cf4
2b274ea
15066b0
0cad8cc
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 |
---|---|---|
|
@@ -97,6 +97,8 @@ config: | |
key_file: "" | ||
server_name: "" | ||
insecure_skip_verify: false | ||
sampler_type: "" | ||
sampler_param: "" | ||
Comment on lines
+100
to
+101
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. Even though we take this over from the OTEL specification, maybe we could add a few words about which values are accepted etc. in the text above? |
||
``` | ||
|
||
### Jaeger | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ package otlp | |
|
||
import ( | ||
"context" | ||
"strconv" | ||
"strings" | ||
|
||
"github.com/thanos-io/thanos/pkg/tracing/migration" | ||
|
@@ -25,6 +26,9 @@ import ( | |
const ( | ||
TracingClientGRPC string = "grpc" | ||
TracingClientHTTP string = "http" | ||
AlwaysSample string = "alwayssample" | ||
NeverSample string = "neversample" | ||
RatioBasedSample string = "traceidratiobased" | ||
Comment on lines
+29
to
+31
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. Two suggestions:
|
||
) | ||
|
||
// NewOTELTracer returns an OTLP exporter based tracer. | ||
|
@@ -59,12 +63,16 @@ func NewTracerProvider(ctx context.Context, logger log.Logger, conf []byte) (*tr | |
} | ||
|
||
processor := tracesdk.NewBatchSpanProcessor(exporter) | ||
tp := newTraceProvider(ctx, processor, logger, config.ServiceName) | ||
sampler, err := getSampler(config) | ||
if err != nil { | ||
logger.Log(err) | ||
} | ||
tp := newTraceProvider(ctx, processor, logger, config.ServiceName, sampler) | ||
|
||
return tp, nil | ||
} | ||
|
||
func newTraceProvider(ctx context.Context, processor tracesdk.SpanProcessor, logger log.Logger, serviceName string) *tracesdk.TracerProvider { | ||
func newTraceProvider(ctx context.Context, processor tracesdk.SpanProcessor, logger log.Logger, serviceName string, sampler tracesdk.Sampler) *tracesdk.TracerProvider { | ||
resource, err := resource.New( | ||
ctx, | ||
resource.WithAttributes(semconv.ServiceNameKey.String(serviceName)), | ||
|
@@ -73,8 +81,6 @@ func newTraceProvider(ctx context.Context, processor tracesdk.SpanProcessor, log | |
level.Warn(logger).Log("msg", "jaeger: detecting resources for tracing provider failed", "err", err) | ||
} | ||
|
||
sampler := tracesdk.ParentBased(tracesdk.TraceIDRatioBased(1.0)) | ||
|
||
tp := tracesdk.NewTracerProvider( | ||
tracesdk.WithSpanProcessor(processor), | ||
tracesdk.WithResource(resource), | ||
|
@@ -86,3 +92,20 @@ func newTraceProvider(ctx context.Context, processor tracesdk.SpanProcessor, log | |
) | ||
return tp | ||
} | ||
|
||
func getSampler(config Config) (tracesdk.Sampler, error) { | ||
switch strings.ToLower(config.SamplerType) { | ||
case AlwaysSample: | ||
return tracesdk.ParentBased(tracesdk.AlwaysSample()), nil | ||
case NeverSample: | ||
return tracesdk.ParentBased(tracesdk.NeverSample()), nil | ||
case RatioBasedSample: | ||
Comment on lines
+98
to
+102
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. As I pointed out above, this should be adjusted - for these samplers, we do not want to have them wrapped with
Does that make sense? |
||
arg, err := strconv.ParseFloat(config.SamplerParam, 64) | ||
if err != nil { | ||
return tracesdk.ParentBased(tracesdk.TraceIDRatioBased(1.0)), err | ||
} | ||
return tracesdk.ParentBased(tracesdk.TraceIDRatioBased(arg)), nil | ||
} | ||
|
||
return tracesdk.ParentBased(tracesdk.TraceIDRatioBased(1.0)), nil | ||
} |
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.
Small suggestion: