Separate HTTP client/server AttributesExtractors #4195
Conversation
mateuszrzeszutek
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
pavolloffay,
trask and
tylerbenson
as code owners
| @@ -21,7 +21,7 @@ | |||
| * #onStart(AttributesBuilder, Object)} to have it available during sampling. | |||
| * | |||
| * @see DbAttributesExtractor | |||
| * @see HttpAttributesExtractor | |||
| * @see HttpClientAttributesExtractor | |||
There was a problem hiding this comment.
| * @see HttpClientAttributesExtractor | |
| * @see HttpClientAttributesExtractor | |
| * @see HttpServerAttributesExtractor |
|
|
||
| // TODO: these are specific to servers, should we remove those? | ||
| set(attributes, SemanticAttributes.HTTP_TARGET, target(request)); | ||
| set(attributes, SemanticAttributes.HTTP_HOST, host(request)); | ||
| set(attributes, SemanticAttributes.HTTP_SCHEME, scheme(request)); |
There was a problem hiding this comment.
+1 for removing these. We can always add them later if there's really a compelling reason.
cc: @lmolkova
|
|
||
| // TODO: this is specific to clients, should we remove this? | ||
| set(attributes, SemanticAttributes.HTTP_URL, url(request)); |
There was a problem hiding this comment.
same, +1 for removing this and we can always add it later if there's really a compelling reason.
There was a problem hiding this comment.
I am not sure that we can remove this. I arbitrary took a couple of server instrumentation's extractor, and they all returned null for attributes above
| private String extractRoute(REQUEST request) { | ||
| if (attributesExtractor instanceof HttpServerAttributesExtractor) { | ||
| return ((HttpServerAttributesExtractor<REQUEST, ?>) attributesExtractor).route(request); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
is it worth HttpClientSpanNameExtractor and HttpServerSpanNameExtractor?
There was a problem hiding this comment.
Hmm, maybe we could have different factory methods in HttpSpanNameExtractor? One accepting the client extractor, the other the server one
There was a problem hiding this comment.
instanceof seems OK too from what I can tell
| private String extractRoute(REQUEST request) { | ||
| if (attributesExtractor instanceof HttpServerAttributesExtractor) { | ||
| return ((HttpServerAttributesExtractor<REQUEST, ?>) attributesExtractor).route(request); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
instanceof seems OK too from what I can tell
|
👍 this is great, going to merge, TODOs make sense (to try) as follow-up(s) |
mateuszrzeszutek
deleted the
separate-http-client-server-attributes-extractors
branch
This PR splits
HttpAttributesExtractorintoHttpClientAttributesExtractorandHttpServerAttributesExtractor- I need to do that before I implement the http headers semantic attributes that were recently added to the spec (because we'll want separate configurations for clients/servers).And maybe, if we decide on a solution here we'll be able to get #3700 done