contrib/urfave/negroni: add support for the negroni web framework #974
Conversation
nzin-appdirect
force-pushed
the
negroni_support
branch
from
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 |
nzin-appdirect
force-pushed
the
negroni_support
branch
from
0a952d6
to
7ad392e
Compare
|
Hi @knusbaum , I think my PR is ready for review now! :-) |
There was a problem hiding this comment.
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.
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.
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 Options 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.
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.
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.
nzin-appdirect
force-pushed
the
negroni_support
branch
from
24787d4
to
4b6b39c
Compare
|
thanks @knusbaum for all the comments (very good indeed). |
|
Hi @knusbaum how are you? :-) |
contrib/urfave/negroni: add support for the negroni web framework
Fixes #969