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.

Sign up for GitHub

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

Jump to bottom

Opentracing support #2587

Merged
merged 14 commits into from
Jan 10, 2018
Merged

Opentracing support #2587

merged 14 commits into from
Jan 10, 2018

Conversation

mmatur
Copy link
Member

@mmatur mmatur commented Dec 18, 2017 •
edited
Loading

What does this PR do?

This PR carries #2237 made by @tcolgate 👏

More

  • Added/updated tests
  • Added/updated documentation

Fixes #771

@mmatur mmatur mentioned this pull request Dec 18, 2017
Closed
@mmatur mmatur added this to the 1.6 milestone Dec 18, 2017
@mmatur mmatur force-pushed the feature/opentracing branch 11 times, most recently from 9f6f1a7 to 7195863 Compare December 21, 2017 17:34
charlieoleary
charlieoleary reviewed Dec 28, 2017
View reviewed changes
cmd/traefik/configuration.go Outdated
@@ -202,6 +205,23 @@ func NewTraefikDefaultPointersConfiguration() *TraefikConfiguration {
DialTimeout: flaeg.Duration(configuration.DefaultDialTimeout),
}

// default Tracing
defaultTracing := tracing.Tracing{
Backend: "jarger",
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be mistaken, but I think this should be jaeger?

juliens
juliens requested changes Dec 28, 2017
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

👏 👏

middlewares/auth/forward.go Outdated
w.WriteHeader(forwardResponse.StatusCode)
statusCode := forwardResponse.StatusCode
tracing.LogResponseCode(tracing.GetSpan(r), statusCode)
w.WriteHeader(statusCode)
Copy link
Member

Choose a reason for hiding this comment

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

why do you use a statusCode var ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is unnecessary, you right 👏

middlewares/tracing/forwarder.go Outdated
"github.com/urfave/negroni"
)

type fwdMiddleware struct {
Copy link
Member

Choose a reason for hiding this comment

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

fwdMiddleware => forwarderMiddleware ?

middlewares/tracing/forwarder.go Outdated
*Tracing
}

// NewForwarder creates a new forwarder middleware that the final outgoing request
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence

middlewares/auth/authenticator.go Outdated
next.ServeHTTP(w, r)
}
})
authenticator.handler = tracingMiddleware.NewNegroniHandlerWrapper("Auth Basic", createAuthBasicHandler(basicAuth, authConfig), false)
Copy link
Member

Choose a reason for hiding this comment

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

tracingMiddleware could be nil ?

Copy link
Member Author

@mmatur mmatur Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

tracingMiddleware can not be nil because there is a default value https://github.com/containous/traefik/pull/2587/files/f7b0da15af1a7d836f9e21cae6f252b4580c3c7e#diff-f80800db6a453c4a7fff51b05de444f2R209

@mmatur mmatur force-pushed the feature/opentracing branch from b00e3a0 to 82ec752 Compare December 29, 2017 15:15
juliens
juliens approved these changes Jan 2, 2018
View reviewed changes
Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

nmengin
nmengin approved these changes Jan 9, 2018
View reviewed changes
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.

@mmatur @tcolgate

Many thanks for this awesome feature! 👏 👏

LGTM! 🎉

@ldez ldez force-pushed the feature/opentracing branch from ee234fd to 695a2fd Compare January 10, 2018 16:15
@traefiker traefiker force-pushed the feature/opentracing branch from 695a2fd to 633eafb Compare January 10, 2018 16:32
@traefiker traefiker merged commit 30ffba7 into traefik:master Jan 10, 2018
@emilevauge
Copy link
Member

emilevauge commented Jan 10, 2018 •
edited
Loading

@tcolgate I apologise for this, we made a mistake 😔 your commits should indeed appear in the history. This PR has been merged too quickly.
Your initial work was amazing and openTracing would not have been supported by Traefik without you.

@mmatur mmatur deleted the feature/opentracing branch January 10, 2018 19:16
@mmatur mmatur restored the feature/opentracing branch January 10, 2018 20:07
@mmatur mmatur deleted the feature/opentracing branch January 10, 2018 20:07
@ldez ldez mentioned this pull request Jan 16, 2018
Merged
@ldez ldez mentioned this pull request Feb 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlieoleary charlieoleary charlieoleary left review comments

@juliens juliens juliens approved these changes

@ldez ldez ldez approved these changes

@nmengin nmengin nmengin approved these changes

Assignees
No one assigned
Labels
area/middleware/tracing kind/enhancement a new or improved feature. size/L
Projects
None yet
Milestone
1.6
Development

Successfully merging this pull request may close these issues.
8 participants
@mmatur @tcolgate @emilevauge @juliens @charlieoleary @ldez @nmengin @traefiker