feat: Allow configuring span name, and add :route_name #4
Conversation
|
I wouldn't bother with making it configurable. The http spec for OC is to use the route, so it should be just how it works. |
|
@tsloughter this code currently does method + route, like |
|
Ah, yea, the method should just be an attribute according to the spec https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#spans |
|
Yeah, I just found that when you mentioned the spec. I should get used to this not being the Wild West of Spandex where we just made things in the way that would render best in Datadog's proprietary systems. Not an insult to them, but a lot of atypical/unexpected things were required there. |
|
hehe, yea. It is nice having all that proprietary weirdness moved to the reporters. |
| defp route_info(%{method: method, request_path: request_path, host: host} = conn) do | ||
| router = conn.private[:phoenix_router] | ||
|
|
||
| apply(Phoenix.Router, :route_info, [router, method, request_path, host]) |
There was a problem hiding this comment.
Why is this an apply instead of just Phoenix.Router.route_info(router, method, request_path, host)
There was a problem hiding this comment.
I guess it was because it was generating a compiler warning, but now it won't be compiled without that function available, so I'll make it a normal call.
There was a problem hiding this comment.
I haven't (and won't for a bit) have a chance to test these on a running phoenix application so if you have one on hand we should try it out before merging.
There was a problem hiding this comment.
oh, oops, hehe, I don't but I'm asking at work if anyone does.
There was a problem hiding this comment.
I can try it out tonight, so if you confirm that it works let me know but otherwise I'll check then (8-10 hours from now).
Chris Mccord messaged me recently to let me know of an awesome new thing in a recent version of phoenix that allows us to finally get the route template :D I'm working on getting things set up to write a spandex to opencensus guide, so I'm contributing this here, instead of on our end. Right now in spandex we get the route in a very hacky/ugly way, so I'm excited to have this.
I made it configurable to retain your current default, and added the ability to configure it as an MFA, but I'm open to any and all feedback here. I just threw this together quick, so we can make any changes necessary. If you like it, I can add documentation/tests to get it up to par.