Refactor AttributesExtractor so that it extracts route from Context #5288
Conversation
...api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/AttributesExtractor.java
Outdated
Show resolved
Hide resolved
| set(attributes, SemanticAttributes.HTTP_SERVER_NAME, getter.serverName(request, response)); | ||
| set(attributes, SemanticAttributes.HTTP_ROUTE, httpRouteHolderGetter.apply(context)); |
There was a problem hiding this comment.
I thought about adding a route(context, request, response) to the getter interface (with a default HttpRouteHolder implementation) but I decided against it: HttpRouteHolder not only stores the route, but also renames the server span whenever the route is updated; if we were to allow somebody to implement a route(context, request, response) method that overrode the default implementation (and skipped the HttpRouteHolder altogether), that instrumentation would not rename the server span after start -- and that would differ from how the HttpRouteHolder handles things.
In other words: using route(request) is fine, because the HttpSpanNameExtractor handles that, but if you want to set the route during/at the end of the request you have to use HttpRouteHolder.
|
I made it a draft PR for now -- if this looks acceptable I'll uncomment the |
...pi/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java
Outdated
Show resolved
Hide resolved
…tion/api/instrumenter/http/HttpRouteHolder.java Co-authored-by: Trask Stalnaker <[email protected]>
| Context currentContext = span == null ? context : context.with(span); | ||
| netAttributesExtractor.onEnd(attributesBuilder, currentContext, endpoint, null, null); |
There was a problem hiding this comment.
using Context.root() seems like it would be ok for this odd usage, but ok with this too
…pen-telemetry#5288) * Refactor AttributesExtractor so that it extracts route from Context * typo * fix tests * Update instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java Co-authored-by: Trask Stalnaker <[email protected]> * fix all AttributesExtractors Co-authored-by: Trask Stalnaker <[email protected]>
I added the
Contextas a parameter to theAttributesExtractormethods - some of the other interfaces already acceptContext(e.g.SpanLinksExtractor,RequestListener) so it doesn't that look out of place. Still,onEnd()now accepts 5 params...