-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for distributed tracing #1019
Merged
aeneasr
merged 16 commits into
ory:master
from
aaslamin:add-support-for-distributed-tracing
Sep 14, 2018
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
fd1c4b3
tracing: add go-opentracing library and update lock file
aaslamin d32ebc0
tracing: add config for loading tracer with sensible defaults
aaslamin 3aeb7e2
tracing: initialize tracer and handle graceful shutdown
aaslamin d768d25
tracing: update dockerfile to load up the jaeger tracer to aid in dev…
aaslamin b6c1673
tracing: adds support to Hydra for the jaeger distributed tracing system
aaslamin b13b45d
tracing: adds middleware for creating/following (root) span with basi…
aaslamin 6473b97
tracing: address complaint of CI by running goimports
aaslamin be50468
tracing: don't use default hostnames in config. update tracing defaul…
aaslamin bca09de
tracing: missing return in tracer setup on error
aaslamin 5db64b3
tracing: when matching on the tracing provider, convert config value …
aaslamin fb45d2c
tracing: remove config for tracing from the default docker-compose fi…
aaslamin 0e9a313
tracing: add tests for tracing setup
aaslamin 7faa69a
tracing: should exit if tracing setup fails plus minor simplifications.
aaslamin 3d76f1b
tracing: add unit test coverage for the tracing middleware. checks th…
aaslamin 0caedb4
tracing: move tracing middleware test to separate package. adds unit …
aaslamin 90847ef
tracing: check for possible error on call to GetTracer
aaslamin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package tracing | ||
|
|
||
| import ( | ||
| "net/http" | ||
|
|
||
| "github.com/opentracing/opentracing-go" | ||
| "github.com/opentracing/opentracing-go/ext" | ||
| "github.com/urfave/negroni" | ||
| ) | ||
|
|
||
| func (t *Tracer) ServeHTTP(rw http.ResponseWriter, r *http.Request, next http.HandlerFunc) { | ||
| var span opentracing.Span | ||
| opName := r.URL.Path | ||
|
|
||
| // It's very possible that Hydra is fronted by a proxy which could have initiated a trace. | ||
| // If so, we should attempt to join it. | ||
| remoteContext, err := opentracing.GlobalTracer().Extract( | ||
| opentracing.HTTPHeaders, | ||
| opentracing.HTTPHeadersCarrier(r.Header), | ||
| ) | ||
|
|
||
| if err != nil { | ||
| span = opentracing.StartSpan(opName) | ||
| } else { | ||
| span = opentracing.StartSpan(opName, opentracing.ChildOf(remoteContext)) | ||
| } | ||
|
|
||
| defer span.Finish() | ||
|
|
||
| r = r.WithContext(opentracing.ContextWithSpan(r.Context(), span)) | ||
|
|
||
| next(rw, r) | ||
|
|
||
| ext.HTTPMethod.Set(span, r.Method) | ||
| if negroniWriter, ok := rw.(negroni.ResponseWriter); ok { | ||
| statusCode := uint16(negroniWriter.Status()) | ||
| if statusCode >= 400 { | ||
| ext.Error.Set(span, true) | ||
| } | ||
| ext.HTTPStatusCode.Set(span, statusCode) | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| package tracing_test | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "net/http/httptest" | ||
|
|
||
| "github.com/opentracing/opentracing-go/ext" | ||
| "github.com/opentracing/opentracing-go/mocktracer" | ||
|
|
||
| "testing" | ||
|
|
||
| "github.com/opentracing/opentracing-go" | ||
| "github.com/ory/hydra/tracing" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/urfave/negroni" | ||
| ) | ||
|
|
||
| var mockedTracer *mocktracer.MockTracer | ||
| var tracer *tracing.Tracer = &tracing.Tracer{ | ||
| ServiceName: "Ory Hydra Test", | ||
| Provider: "Mock Provider", | ||
| } | ||
|
|
||
| func init() { | ||
| mockedTracer = mocktracer.New() | ||
| opentracing.SetGlobalTracer(mockedTracer) | ||
| } | ||
|
|
||
| func TestTracingServeHttp(t *testing.T) { | ||
| expectedTagsSuccess := map[string]interface{}{ | ||
| string(ext.HTTPStatusCode): uint16(200), | ||
| string(ext.HTTPMethod): "GET", | ||
| } | ||
|
|
||
| expectedTagsError := map[string]interface{}{ | ||
| string(ext.HTTPStatusCode): uint16(400), | ||
| string(ext.HTTPMethod): "GET", | ||
| "error": true, | ||
| } | ||
|
|
||
| testCases := []struct { | ||
| httpStatus int | ||
| testDescription string | ||
| expectedTags map[string]interface{} | ||
| }{ | ||
| { | ||
| testDescription: "success http response", | ||
| httpStatus: http.StatusOK, | ||
| expectedTags: expectedTagsSuccess, | ||
| }, | ||
| { | ||
| testDescription: "error http response", | ||
| httpStatus: http.StatusBadRequest, | ||
| expectedTags: expectedTagsError, | ||
| }, | ||
| } | ||
|
|
||
| for _, test := range testCases { | ||
| t.Run(test.testDescription, func(t *testing.T) { | ||
| defer mockedTracer.Reset() | ||
| request := httptest.NewRequest(http.MethodGet, "https://apis.somecompany.com/endpoint", nil) | ||
| next := func(rw http.ResponseWriter, _ *http.Request) { | ||
| rw.WriteHeader(test.httpStatus) | ||
| } | ||
|
|
||
| tracer.ServeHTTP(negroni.NewResponseWriter(httptest.NewRecorder()), request, next) | ||
|
|
||
| spans := mockedTracer.FinishedSpans() | ||
| assert.Len(t, spans, 1) | ||
| span := spans[0] | ||
|
|
||
| assert.Equal(t, test.expectedTags, span.Tags()) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestShouldContinueTraceIfAlreadyPresent(t *testing.T) { | ||
| defer mockedTracer.Reset() | ||
| parentSpan := mockedTracer.StartSpan("some-operation").(*mocktracer.MockSpan) | ||
| ext.SpanKindRPCClient.Set(parentSpan) | ||
| request := httptest.NewRequest(http.MethodGet, "https://apis.somecompany.com/endpoint", nil) | ||
| carrier := opentracing.HTTPHeadersCarrier(request.Header) | ||
| // this request now contains a trace initiated by another service/process (e.g. an edge proxy that fronts Hydra) | ||
| mockedTracer.Inject(parentSpan.Context(), opentracing.HTTPHeaders, carrier) | ||
|
|
||
| next := func(rw http.ResponseWriter, _ *http.Request) { | ||
| rw.WriteHeader(http.StatusOK) | ||
| } | ||
|
|
||
| tracer.ServeHTTP(negroni.NewResponseWriter(httptest.NewRecorder()), request, next) | ||
|
|
||
| spans := mockedTracer.FinishedSpans() | ||
| assert.Len(t, spans, 1) | ||
| span := spans[0] | ||
|
|
||
| assert.Equal(t, parentSpan.SpanContext.SpanID, span.ParentID) | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Not sure about this line, are you sure this is
negroni? I think it would be better do define your own interface here or use one from the standard libraryThere 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.
I am sure that this is the negroni response writer because of https://github.com/urfave/negroni/blob/master/negroni.go#L96
https://github.com/urfave/negroni/blob/master/response_writer.go#L31
With that said, to be extra defensive, I capture the boolean returned from the type assertion to verify if the
ResponseWriteris indeed of typenegroini.ResponseWriter. All of this is so we can add the tag in our span for the http status that was returned: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.
Ok I see how this works now. This is a negroni middleware so we're getting the correct RW here. I though it worked differently, my bad :)