add url.template to client spans
#13572
Conversation
|
|
||
| internalSet(attributes, UrlAttributes.URL_SCHEME, urlScheme); | ||
| internalSet(attributes, UrlAttributes.URL_PATH, urlPath); | ||
| internalSet(attributes, UrlAttributes.URL_QUERY, urlQuery); | ||
| internalSet(attributes, AttributeKey.stringKey("url.template"), urlTemplate); |
There was a problem hiding this comment.
This constant is in the UrlIncubatingAttributes class. You will have to take a dependency on opentelemetry-semconv-incubating to reference that, and I suspect that the maintainers might frown on that. 😶
There was a problem hiding this comment.
I could not find any policies on stability, but I suspect you're right. (perhaps there is an expectation that things like this will be put behind a flag?)
There was a problem hiding this comment.
The current guidance is that incubating attribute classes such as UrlIncubatingAttributes should only be used in tests. Implementation needs to copy the constant because incubating attributes can be removed or rnamed in subsequent versions of the semantic conventions. You will have to move this code to incubating module, perhaps to https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api-incubator/src/main/java/io/opentelemetry/instrumentation/api/incubator/semconv/http/HttpExperimentalAttributesExtractor.java
...i/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpSpanNameExtractor.java
Outdated
Show resolved
Hide resolved
| @@ -34,4 +35,7 @@ internal object KtorHttpClientAttributesGetter : HttpClientAttributesGetter<Http | |||
| override fun getServerAddress(request: HttpRequestData) = request.url.host | |||
|
|
|||
| override fun getServerPort(request: HttpRequestData) = request.url.port | |||
|
|
|||
| private val urlTemplateAttributeKey = AttributeKey<String>("URL_TEMPLATE") | |||
| override fun getUrlTemplate(request: HttpRequestData): String? = request.attributes.getOrNull(urlTemplateAttributeKey) | |||
There was a problem hiding this comment.
I'd suggest you to remove the ktor changes until there is a version of ktor that would support this. Once there is a version where this works you could add it back along with a test.
| @@ -21,7 +22,8 @@ | |||
| public interface HttpClientAttributesGetter<REQUEST, RESPONSE> | |||
| extends HttpCommonAttributesGetter<REQUEST, RESPONSE>, | |||
| NetworkAttributesGetter<REQUEST, RESPONSE>, | |||
| ServerAttributesGetter<REQUEST> { | |||
| ServerAttributesGetter<REQUEST>, | |||
| UrlAttributesGetter<REQUEST> { | |||
There was a problem hiding this comment.
UrlAttributesGetter is for server instrumentations. Http client semantic conventions https://github.com/open-telemetry/semantic-conventions/blob/5816d4bb099b69eba1cb45084d7cceb51ad6bbfe/docs/http/http-spans.md#http-client don't list all of the attributes that UrlAttributesGetter provides.
There was a problem hiding this comment.
so can I add getUrlTemplate to the HttpClientAttributesGetter interface directly?
does this cause issues with incubating vs stable?
I don't see a way to add it similar to http.request.body.size because the template would not reside in the headers and would instead be pretty instrumentation specific
There was a problem hiding this comment.
so can I add getUrlTemplate to the HttpClientAttributesGetter interface directly?
As far as I understand, no you can't. You could if that attribute was declared stable in the semantic conventions. For now you'll have to approach it in a roundabout way. HttpClientAttributesGetter is part of the stable api. You could introduce an interface to the instrumentation-api-incubator module that either extends the HttpClientAttributesGetter or just provides the getUrlTemplate method. In the extractor you could test the getter with instanceof. Probably the most tricky one will be handling the span name in HttpSpanNameExtractor. For that you could register a callback in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/Experimental.java so that instrumentation-api could call into instrumentation-api-incubator.
| * <p>Examples: {@code /users/{id}}; {@code /users?q={query}} | ||
| */ | ||
| @Nullable | ||
| default String getUrlTemplate(REQUEST request) { |
There was a problem hiding this comment.
Since instrumentation-api module is marked as stable we can not introduce unstable features here. You will have to add this to instrumentation-api-incubator module.
|
I'll close for now as I've got a janky workaround on my application and it looks like it would be an uphill battle getting the incubating attribute working. I may come back to this. |
|
Support for |
Added support for url.template (plus KTOR integration)
if ktorio/ktor#4747 gets merged in. Fixes #13570.
If it does not get merged, allows applications to set the attribute.
url.templateis currently hard coded as it's not in the semantics package yet.I imagine this is due to stability stuff. I'm unsure if this makes this PR unmergable.