Span naming flexibility#796
Conversation
|
very nice description!
|
|
Here is an example of path template based naming powered by Gorilla mux using the new features of this PR: |
plugin/ochttp/server.go
Outdated
| // NameFromRequest holds the function to use for generating the span name | ||
| // from the information found in the incoming HTTP Request. By default the | ||
| // name equals the URL Path. | ||
| NameFromRequest func(*http.Request) string |
There was a problem hiding this comment.
I think we should remove this. Seems like anything you can accomplish with NameFromRequest you can always accomplish by installing a middleware that calls SetName.
And I think encouraging the use of SetName is actually preferable, let me explain. We want to encourage users to install ochttp.Handler as the outer-most handler (so we measure as much as possible). However, the proper span name is only available after the routing middleware has executed, hence the need for changing it later. If we provide NameFromRequest, it may encourage users to install ochttp.Handler after the routing middleware in order to capture the routing key. This would be bad because we want to measure as much of the application processing time as possible (including the routing middleware execution).
There was a problem hiding this comment.
I appreciate the concern with respect to potential misuse in order to capture routing details and yes SetName can overwrite the span name to the preferred name for all use cases.
Unfortunately that comes at a cost for people that want to use a different naming scheme which can be identified from the upfront raw http request details. As an example, people in polyglot brownfield environments where the name of a call is preferred to be low cardinality likehttp:post because the used backend due to indexing really dislikes high cardinality path based naming.
By removing this feature from the PR we now force them to add another middleware layer in their service just to update the span name and incur the cost of the lock when updating Span.data with SetName.
I suggest keeping this functionality but being very clear about how ochttp should be used when composing the multi handler flow. I think this mitigates your concerns enough to warrant keeping this functionality.
There was a problem hiding this comment.
Alright. If it were me, I would probably err on the side of having only one way to do things but I can see your points too so I happy to merge.
plugin/ochttp/client.go
Outdated
| // NameFromRequest holds the function to use for generating the span name | ||
| // from the information found in the outgoing HTTP Request. By default the | ||
| // name equals the URL Path. | ||
| NameFromRequest func(*http.Request) string |
There was a problem hiding this comment.
Could we have a unit test for this new functionality?
trace/trace_test.go
Outdated
| Status: Status{}, | ||
| HasRemoteParent: true, | ||
| } | ||
| if !reflect.DeepEqual(got, want) { |
There was a problem hiding this comment.
Just check the name here to make it clear what you are testing.
There was a problem hiding this comment.
Also would be good to test what happens if you call SetName when not collecting events and also after the Span has ended.
|
I just found this related issue, not sure if you were aware of it: #663 In that issue @rakyll suggests calling the method |
3 participants
This PR enables more flexibility in span naming for various use cases.
span.SetName() allows one to update the span name after creation time. This allows routing middlewares to update the span name to the matched route providing lower cardinality then path names (think REST services).
ochttp client and server handlers now allow for the span name to be generated from pluggable functions defaulting to the already existing spanNameFromURL. This can be used if span naming conventions in a brown field deployment are different from OpenCensus defaults or if using URL constructors for REST services that can also report route templates instead of expanded URL path.