Skip to content
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

Merged
merged 3 commits into from Jun 28, 2018

Conversation

aantono
Copy link
Contributor

@aantono aantono commented Jun 19, 2018

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

@mmatur
Copy link
Member

mmatur commented Jun 20, 2018

Thanks @aantono for this PR, I will take a look ;)

@mmatur mmatur added this to the 1.7 milestone Jun 20, 2018
@mmatur mmatur added the kind/enhancement a new or improved feature. label Jun 20, 2018
#
backend = "datadog"

# Service name used in Zipkin backend
Copy link
Member

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

[tracing.datadog]
# Local Agent Host Port instructs reporter to send spans to datadog-tracing-agent at this address
#
# Default: "127.0.0.1:6831"
Copy link
Member

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

# Default: ""
#
globalTag = "foo:bar"

Copy link
Member

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"
Copy link
Member

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"`
Copy link
Member

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"`
Copy link
Member

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?

Copy link
Member

@mmatur mmatur left a 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
Copy link
Member

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"

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@mmatur mmatur left a 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 👏

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
:shipit:

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👏 👏 🐶 📈

@ldez ldez removed the bot/no-merge label Jun 28, 2018
@traefiker traefiker merged commit 3192307 into traefik:master Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants