Low cardinality name for servlet span

[laurit]

Resolves #1954

Application server instrumentation and servlet filter set server span name to HTTP {METHOD}. When span is started from servlet name is set based on servlet mapping. If servlet mapping does not contain wildcards then name is same as request.getContextPath() + request.getServletPath() as it was previously. If servlet mapping has a leading wildcard, for example, *.html then name is request.getContextPath() + "*.html", except for jsp where name is request.getContextPath() + request.getServletPath(). If servlet mapping has a trailing wildcard /foo/* then if request is /context/foo name is /context/foo, if request is /context/foo/bar then name is /context/foo/*. Special mappings / and /* behave the same way as trailing wildcard. If mappings can't be used or no match is found then name is set to HTTP {METHOD}.
If span is started from application server or filter then span name will be update when request hits the first servlet.
Frameworks that run from filter like struts2 and wicket suppress servlet server span naming to ensure that it does not overwrite name generated by framework integration.

[trask]

👍

If servlet mapping has a trailing wildcard /foo/* then if request is /context/foo name is /context/foo

I think span name of /context/foo/* may be better here, to match the "route"

If mappings can't be used or no match is found then name is set to HTTP {METHOD}.

what do you think about using contextPath instead? especially for non-root contextPath that is likely more interesting than HTTP GET

[trask]

hey @laurit, no rush, but just wanted to check that you saw my questions above

also, heads up looks like there's going to be merge conflict with #2462

[laurit]

@trask the changes you requested should be done now.

[trask]

Thanks @laurit!

@open-telemetry/java-instrumentation-approvers I think this would be good to merge before javaagent 1.0.0, so if you want to review, please do soon 👍

[trask]

❤️

[anuraaga]

It looks like we lost the change I made to use CAS - are you sure you don't want to use it here @laurit? I think it makes the safety of the code more obvious, and less code in callers.

[laurit]

I changed the logic a bit. Previously span name was always updated when shouldUpdateServerSpanName was called, now we call shouldUpdateServerSpanName, if name was not already updated we generate new name and only if new name wasn't null (null means that we are currently executing a filter from which we can't get a good name) update span name. With using cas would need to generate a new span name and verify it is not null before calling shouldUpdateServerSpanName which I didn't want to do.
shouldUpdateServerSpanName is also called from integrations that set server span name from filter and need to suppress naming from servlet so that the name given from framework integration wouldn't be overwritten.
Anyway It is likely that we need to change how this works, probably need to add some kind of priorities to ensure that frameworks don't overwrite names given by each other. Currently for example when calling controller we set server span name to /foo when this controller throws an exception framework calls another controller which is mapped to /error to handle the exception. On call to second controller we set server span name to /error although it should remain as /foo. Also current approach isn't well suited for portals where you could have multiple frameworks with their controllers sitting on the same page each trying to have the final say in what the span name should be.