Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add url.template to client spans #13572

Closed
wants to merge 5 commits into from
Closed

add url.template to client spans #13572

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1e04539
added url template.
MarcusDunn Mar 21, 2025
0534721
added a test
MarcusDunn Mar 21, 2025
ae719d5
added source compatability check
MarcusDunn Mar 21, 2025
2b8828f
spotless
MarcusDunn Mar 21, 2025
55c0267
do not fetch template if there will be an early return
MarcusDunn Mar 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion docs/apidiffs/current_vs_latest/opentelemetry-instrumentation-api.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,10 @@
Comparing source compatibility of opentelemetry-instrumentation-api-2.15.0-SNAPSHOT.jar against opentelemetry-instrumentation-api-2.14.0.jar
No changes.
*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.instrumentation.api.semconv.http.HttpClientAttributesGetter (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
GENERIC TEMPLATES: === REQUEST:java.lang.Object, === RESPONSE:java.lang.Object
+++ NEW INTERFACE: io.opentelemetry.instrumentation.api.semconv.url.UrlAttributesGetter
*** MODIFIED INTERFACE: PUBLIC ABSTRACT io.opentelemetry.instrumentation.api.semconv.url.UrlAttributesGetter (not serializable)
=== CLASS FILE FORMAT VERSION: 52.0 <- 52.0
GENERIC TEMPLATES: === REQUEST:java.lang.Object
+++ NEW METHOD: PUBLIC(+) java.lang.String getUrlTemplate(java.lang.Object)
+++ NEW ANNOTATION: javax.annotation.Nullable
4 changes: 3 additions & 1 deletion ...in/java/io/opentelemetry/instrumentation/api/semconv/http/HttpClientAttributesGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import io.opentelemetry.instrumentation.api.semconv.network.NetworkAttributesGetter;
import io.opentelemetry.instrumentation.api.semconv.network.ServerAttributesGetter;
import io.opentelemetry.instrumentation.api.semconv.url.UrlAttributesGetter;
import javax.annotation.Nullable;

/**
Expand All @@ -21,7 +22,8 @@
public interface HttpClientAttributesGetter<REQUEST, RESPONSE>
extends HttpCommonAttributesGetter<REQUEST, RESPONSE>,
NetworkAttributesGetter<REQUEST, RESPONSE>,
ServerAttributesGetter<REQUEST> {
ServerAttributesGetter<REQUEST>,
UrlAttributesGetter<REQUEST> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Returns the absolute URL describing a network resource according to <a
Expand Down
3 changes: 2 additions & 1 deletion ...rc/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpSpanNameExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,11 @@ static final class Client<REQUEST> implements SpanNameExtractor<REQUEST> {
@Override
public String extract(REQUEST request) {
String method = getter.getHttpRequestMethod(request);
String template = getter.getUrlTemplate(request);
if (method == null || !knownMethods.contains(method)) {
return "HTTP";
}
return method;
return template == null ? method : method + " " + template;
}
}

Expand Down
10 changes: 10 additions & 0 deletions ...i/src/main/java/io/opentelemetry/instrumentation/api/semconv/url/UrlAttributesGetter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,14 @@ default String getUrlPath(REQUEST request) {
default String getUrlQuery(REQUEST request) {
return null;
}

/**
* Returns the template used to build the full URL (if available)
*
* <p>Examples: {@code /users/{id}}; {@code /users?q={query}}
*/
@Nullable
default String getUrlTemplate(REQUEST request) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return null;
}
}
3 changes: 3 additions & 0 deletions ...pentelemetry/instrumentation/api/semconv/url/internal/InternalUrlAttributesExtractor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import static io.opentelemetry.instrumentation.api.internal.AttributesExtractorUtil.internalSet;

import io.opentelemetry.api.common.AttributeKey;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.instrumentation.api.semconv.url.UrlAttributesGetter;
import io.opentelemetry.semconv.UrlAttributes;
Expand All @@ -31,10 +32,12 @@ public void onStart(AttributesBuilder attributes, REQUEST request) {
String urlScheme = getUrlScheme(request);
String urlPath = getter.getUrlPath(request);
String urlQuery = getter.getUrlQuery(request);
String urlTemplate = getter.getUrlTemplate(request);

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😶

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

private String getUrlScheme(REQUEST request) {
Expand Down
8 changes: 8 additions & 0 deletions ...est/java/io/opentelemetry/instrumentation/api/semconv/http/HttpSpanNameExtractorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,12 @@ void nothing() {
assertThat(HttpSpanNameExtractor.create(clientGetter).extract(Collections.emptyMap()))
.isEqualTo("HTTP");
}

@Test
void templateAndMethod() {
when(clientGetter.getUrlTemplate(anyMap())).thenReturn("/cats/{id}");
when(clientGetter.getHttpRequestMethod(anyMap())).thenReturn("GET");
assertThat(HttpSpanNameExtractor.create(clientGetter).extract(Collections.emptyMap()))
.isEqualTo("GET /cats/{id}");
}
}
4 changes: 4 additions & 0 deletions ...otlin/io/opentelemetry/instrumentation/ktor/v2_0/common/KtorHttpClientAttributesGetter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package io.opentelemetry.instrumentation.ktor.v2_0.common

import io.ktor.client.request.*
import io.ktor.client.statement.*
import io.ktor.util.AttributeKey
import io.opentelemetry.instrumentation.api.semconv.http.HttpClientAttributesGetter

internal object KtorHttpClientAttributesGetter : HttpClientAttributesGetter<HttpRequestData, HttpResponse> {
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
Loading