-
Notifications
You must be signed in to change notification settings - Fork 5.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
Added default configuration for DataDog APM Tracer #3655
Conversation
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.
LGTM
configuration/configuration.go
Outdated
Debug: false, | ||
} | ||
} | ||
if gc.Tracing.DataDog != 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.
Should be
if gc.Tracing.Zipkin != nil {
log.Warn("Zipkin configuration will be ignored")
gc.Tracing.Zipkin = nil
}
if gc.Tracing.Jaeger != nil {
log.Warn("Jaeger configuration will be ignored")
gc.Tracing.Jaeger = nil
}
or
log.Warn("Zipkin & Jaeger configuration will be ignored")
gc.Tracing.Jaeger = nil
gc.Tracing.Zipkin = nil
According to the chosen solution, you also need to update the Zipkin
and Jaeger
case
f4e94fc
to
87b79ab
Compare
I am not very a fan to have multiple conditional check for each tracer. I would propose different solution. Solution 1 case jaeger.Name:
if gc.Tracing.Jaeger == nil {
gc.Tracing.Jaeger = &jaeger.Config{
SamplingServerURL: "http://localhost:5778/sampling",
SamplingType: "const",
SamplingParam: 1.0,
LocalAgentHostPort: "127.0.0.1:6831",
}
}
if gc.Tracing.Zipkin != nil || gc.Tracing.DataDog != nil {
log.Warn("Zipkin & DataDog configuration will be ignored")
gc.Tracing.Zipkin = nil
gc.Tracing.DataDog = nil
} Solution 2 log.Info("Using Jaeger configuration other tracing configurations will be ignored")
if gc.Tracing.Jaeger == nil {
gc.Tracing.Jaeger = &jaeger.Config{
SamplingServerURL: "http://localhost:5778/sampling",
SamplingType: "const",
SamplingParam: 1.0,
LocalAgentHostPort: "127.0.0.1:6831",
}
}
gc.Tracing.Zipkin = nil
gc.Tracing.DataDog = nil Personally I prefer the second solution. WDYT? |
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.
LGTM
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.
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.
LGTM :)
Fixed DataDog APM initialization error due to missing default configs.