-
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 integration support for DataDog APM Tracing #3517
Conversation
65b5c21
to
87fd2c9
Compare
Thanks @aantono for this PR, I will take a look ;) |
docs/configuration/tracing.md
Outdated
# | ||
backend = "datadog" | ||
|
||
# Service name used in Zipkin backend |
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.
Could you replace Service name used in Zipkin backend
by Service name used in DataDog backend
docs/configuration/tracing.md
Outdated
[tracing.datadog] | ||
# Local Agent Host Port instructs reporter to send spans to datadog-tracing-agent at this address | ||
# | ||
# Default: "127.0.0.1:6831" |
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.
Could you replace 127.0.0.1:6831
by 127.0.0.1:8126
docs/configuration/tracing.md
Outdated
# Default: "" | ||
# | ||
globalTag = "foo:bar" | ||
|
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.
Could you remove this line please
|
||
ddtracer "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/opentracer" | ||
datadog "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/tracer" | ||
"strings" |
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.
Could you reorganize your import please?
|
||
// Config provides configuration settings for a zipkin tracer | ||
type Config struct { | ||
LocalAgentHostPort string `description:"set datadog-agent's host:port that the reporter will used. Defaults to localhost:8126" export:"false"` |
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.
Could you please replace set
by Set
type Config struct { | ||
LocalAgentHostPort string `description:"set datadog-agent's host:port that the reporter will used. Defaults to localhost:8126" export:"false"` | ||
GlobalTag string `description:"Key:Value tag to be set on all the spans." export:"true"` | ||
Debug bool `description:"Enable Zipkin debug." export:"true"` |
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.
Could you replace Zipkin
by DataDog
?
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.
2 minor remarks. Could you please not amend your commit, it is more complicated to review
// Name sets the name of this tracer | ||
const Name = "datadog" | ||
|
||
// Config provides configuration settings for a zipkin tracer |
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.
Could you change zipkin
with datadog
please
|
||
"github.com/containous/traefik/log" | ||
"github.com/opentracing/opentracing-go" | ||
|
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.
Could you please remove this line
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.
Done
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
Thanks @aantono for this integration 👏
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.
LGTM 👏 👏 🐶 📈
What does this PR do?
Adds integration support for DataDog APM Tracing
Motivation
Given that DataDog Tracing is OpenTracing API compliant, and Traefik already has support for DataDog monitoring, having ability to also send tracing would be beneficial