Adds support for http.route based name

openzipkin / zipkin-js

Zipkin instrumentation for Node.js and browsers

Apache License 2.0

565 stars 171 forks source link

Adds support for http.route based name #183

Open jcchavezs opened 6 years ago

jcchavezs commented 6 years ago

Currently http middlewares use the http method for naming spans. This solution is a not so bad low cardinality option, however from user perspective this could be improved by using the path instead of the verb. For that we can override the span name in the onPreResponse (see this in hapi for example) event so instead of having get as span name we get get /user/{user_id}.

[x] express
[x] koa
[ ] hapi
[ ] connect
[ ] restify

Ping @fbaiodias @adriancole

codefromthecrypt commented 6 years ago

There is some discussion about "http.template" which sounds like this change can afford.

Not sure if we should change default because it is what most zipkin instrumentation use since the beginning (ex will mix up drop down), but should be possible (ex via a hook or a naming policy config)

So regardless we should support the mechanism just not sure if it should be the default policy or not (as it isnt universally or even mostly supported across frameworks it will end up inconsistent)

jcchavezs commented 4 years ago

A test suite for http has been added and now to test it works we just need to set this to true: https://github.com/openzipkin/zipkin-js/blob/a173522b73c71f2ac330b7b2d41959c50d19762a/test/httpServerTestFixture.js#L13