-
Notifications
You must be signed in to change notification settings - Fork 435
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
contrib/urfave/negroni: add support for the negroni web framework #974
Conversation
c3c8c1b
to
9b75009
Compare
Hi, @nzin-appdirect. We should increase the test coverage to around the 90% target. You should be able to see the coverage report by following the "Details" link in the check list. |
@nzin-appdirect |
0a952d6
to
7ad392e
Compare
Hi @knusbaum , I think my PR is ready for review now! :-) |
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.
Hey, @nzin-appdirect.
Thanks for being patient while waiting for us to get to this.
The integration looks good overall, but I have a few changes for you to make.
func TestError(t *testing.T) { | ||
assertSpan := func(assert *assert.Assertions, spans []mocktracer.Span, code int) { | ||
assert.Len(spans, 1) | ||
if len(spans) < 1 { |
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.
Same here with the duplicated check.
t.Run("default", func(t *testing.T) { | ||
assert := assert.New(t) | ||
mt := mocktracer.Start() | ||
defer mt.Stop() | ||
|
||
// setup | ||
router := negroni.New() | ||
router.Use(Middleware(WithServiceName("foobar"))) | ||
|
||
code := 500 | ||
|
||
// a handler with an error and make the requests | ||
mux := http.NewServeMux() | ||
mux.HandleFunc("/err", func(w http.ResponseWriter, r *http.Request) { | ||
http.Error(w, fmt.Sprintf("%d!", code), code) | ||
}) | ||
router.UseHandler(mux) | ||
|
||
r := httptest.NewRequest("GET", "/err", nil) | ||
w := httptest.NewRecorder() | ||
router.ServeHTTP(w, r) | ||
response := w.Result() | ||
assert.Equal(response.StatusCode, code) | ||
|
||
// verify the errors and status are correct | ||
spans := mt.FinishedSpans() | ||
assertSpan(assert, spans, code) | ||
}) |
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.
I like that you're deduplicating the assert logic with assertSpan
, but it looks like the rest of the test body is duplicated between the two subtests. It looks like only the code
and the Middleware
Option
s are different between the runs. If so, maybe a table-driven test would help make this look cleaner. (See: go blog on table-driven tests and an example in our tests)
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.
I deduplicated also into a third subtests.
I understand what you are saying, but merging the 3 assert functions may be more complicated to read, so I kept them separated.
But feel free to come back to me on this.
contrib/urfave/negroni/negroni.go
Outdated
tracer.ServiceName(m.cfg.serviceName), | ||
tracer.Tag(ext.HTTPMethod, r.Method), | ||
tracer.Tag(ext.HTTPURL, r.URL.Path), | ||
tracer.Tag(ext.ResourceName, fmt.Sprintf("%s %s", r.Method, r.URL.Path)), |
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.
We can't just use the path as a resource name because we'll end up with cardinality issues. Basically, if you have a path /user/:userID
or somesuch, then you will end up with an explosion of resources, which causes problems in the backend. Other integrations provide the actual quantized route (i.e. /user/:userID
), but with this middleware we don't have access to that, or even know if there is such a thing.
I suggest we add a WithResourceNamer
option similar to Gin's and have the default not set the path in the resource name. Then users can define their own namers to get spans with meaningful resources.
24787d4
to
4b6b39c
Compare
thanks @knusbaum for all the comments (very good indeed). |
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.
Just a spare file. The other changes look good. 👍
Hi @knusbaum how are you? :-) |
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.
Looks good!
contrib/urfave/negroni: add support for the negroni web framework
Fixes #969