Remove a few ServerSpanNaming usages
#4900
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -31,8 +29,7 @@ public static void start(Message message) { | |||
| return; | |||
| } | |||
|
|
|||
| ServerSpanNaming.updateServerSpanName( | |||
| parentContext, CONTROLLER, CxfServerSpanNaming.SERVER_SPAN_NAME, request); | |||
| CxfServerSpanNaming.updateServerSpanName(parentContext, request); | |||
There was a problem hiding this comment.
One problem with not using ServerSpanNaming.updateServerSpanName is that when there is an exception and request is dispatched to exception handling servlet it can overwrite the name set by the original request. ServerSpanNaming avoided it by disallowing name update when new name had the same or lower priority as the name set.
There was a problem hiding this comment.
Does that really happen though? We don't really call updateServerSpanName at the end of processing; there's one invocation in finally block in OpenTelemetryHandlerMappingFilter in spring-webmvc, but it's a web controller framework - it won't run jax-ws code in a controller (probably).
There was a problem hiding this comment.
In web.xml you can set error pages where container will dispatch when there is an exception or when request status is 500 or whatever. This error handler can be a servlet and could potentially also try to update server span name.
There was a problem hiding this comment.
True, that's a valid scenario. Hmm, is this behavior that important, should we keep it? Or would it be fine to just ignore that case (since it's just jax-ws; and maybe jax-ws has its own exception handler interface or something)
There was a problem hiding this comment.
I believe that for jax-ws it is ok to ignore it. It needs to send errors back as a soap response so it can't use servlet error handling.
| @@ -77,8 +74,6 @@ public static void startSpan( | |||
|
|
|||
| Context parentContext = currentContext(); | |||
| request = new JaxWsRequest(target.getClass(), methodName); | |||
| ServerSpanNaming.updateServerSpanName( | |||
There was a problem hiding this comment.
This would have been relevant only when request is handled by a jax-ws framework that we don't have integration with (or when framework instrumentation is disabled or when there is a bug that prevents it from working properly) because jax-ws framework instrumentation runs before it and already sets the name.
There was a problem hiding this comment.
Yep -- and it sets a <class>.<method> span name, so I thought we won't lose that much (if anything) if I remove that.
* Remove a few ServerSpanNaming usages * revert vaadin changes
Another part of #442
I removed
ServerSpanNamingusages from a couple instrumentations that seemed to set something other than the route there (my selection criteria were rather arbitrary though, basically "kinda looks like a route, and is not a webservice call"). I left tapestry & vaadin unchanged because they produse a sort-of route pointing to the template used.Anyway, in the next PR I'll rename
ServerSpanNamingand actually set thehttp.routeattribute there.