Skip to content

Conversation

@k3v1n1k88
Copy link

Class ServletHttpAttributesGetteralways returns null for the getHttpRoute() function because it does not override the getHttpRoute() method. As a result, the class uses the default function from its superclass and http_route will be assigned to null

…on `getHttpRoute()`

Class `ServletHttpAttributesGetter`always returns null for the getHttpRoute() function because it does not override the getHttpRoute() method. As a result, the class uses the default function from its superclass.
@k3v1n1k88 k3v1n1k88 requested a review from a team as a code owner September 29, 2024 05:51
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@laurit
Copy link
Contributor

laurit commented Sep 29, 2024

The description of http.route in https://opentelemetry.io/docs/specs/semconv/attributes-registry/http/#http-attributes states that MUST NOT be populated when this is not supported by the HTTP server framework as the route attribute should have low-cardinality and the URI path can NOT substitute it.
If you are using a higher level framework you should instead check whether that framework provides something that could be used as a route and write instrumentation that sets the route for that framework.

@laurit laurit added the needs author feedback Waiting for additional feedback from the author label Sep 29, 2024
@k3v1n1k88
Copy link
Author

The description of http.route in https://opentelemetry.io/docs/specs/semconv/attributes-registry/http/#http-attributes states that MUST NOT be populated when this is not supported by the HTTP server framework as the route attribute should have low-cardinality and the URI path can NOT substitute it. If you are using a higher level framework you should instead check whether that framework provides something that could be used as a route and write instrumentation that sets the route for that framework.

Hi @laurit, I see your idea. But if this attribute is missing, the metric will not be able to display the corresponding request URIs in the Server metric metric (all). This thing make us confused and can not use server metric of default opentelemetry. We have to custom another server metric for this label. Have you any idea for it?

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Oct 1, 2024
@laurit
Copy link
Contributor

laurit commented Oct 1, 2024

Hi @laurit, I see your idea. But if this attribute is missing, the metric will not be able to display the corresponding request URIs in the Server metric metric (all). This thing make us confused and can not use server metric of default opentelemetry. We have to custom another server metric for this label. Have you any idea for it?

Metrics may not use high cardinality attributes such as URI, that is why we have the route. If you are using a higher level framework that builds on top of servlets it may be possible to extract route from there.

@laurit laurit added the needs author feedback Waiting for additional feedback from the author label Oct 1, 2024
@k3v1n1k88
Copy link
Author

Metrics may not use high cardinality attributes such as URI, that is why we have the route. If you are using a higher level framework that builds on top of servlets it may be possible to extract route from there.

Well, I think you mean we need to customize another HTTP server metric instead of using the default one provided by OpenTelemetry if we're using any framework that relies on a servlet version below v3, right?

@github-actions github-actions bot removed the needs author feedback Waiting for additional feedback from the author label Oct 1, 2024
@laurit
Copy link
Contributor

laurit commented Oct 1, 2024

Well, I think you mean we need to customize another HTTP server metric instead of using the default one provided by OpenTelemetry if we're using any framework that relies on a servlet version below v3, right?

No. Framework specific instrumentation provides the route that is used by the standard metrics. Have a look at https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md in Semantic Conventions column some frameworks have Provides http.route [2]. For these frameworks we have instrumentation that updates the route on the SERVER span.

@k3v1n1k88
Copy link
Author

No. Framework specific instrumentation provides the route that is used by the standard metrics. Have a look at https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/supported-libraries.md in Semantic Conventions column some frameworks have Provides http.route [2]. For these frameworks we have instrumentation that updates the route on the SERVER span.

I see your point. So I think need to build another instrument (as extension) for server metric with servlet instead of use server metric of open telemetry in this situation.

Thanks for your review!

@k3v1n1k88 k3v1n1k88 closed this Oct 2, 2024
@laurit
Copy link
Contributor

laurit commented Oct 2, 2024

I see your point. So I think need to build another instrument (as extension) for server metric with servlet instead of use server metric of open telemetry in this situation.

What web framework are you using? Instrumenting the web framework to update the route would also work. Or you could call one of the update methods in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java from your application code.

@k3v1n1k88
Copy link
Author

What web framework are you using? Instrumenting the web framework to update the route would also work. Or you could call one of the update methods in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java from your application code.

Currently, we are integrating with Jetty embedded (v9.x), which uses Servlet v3 to publish HTTP endpoints. While the update method mentioned above can address this issue, the default histogram with 10 buckets still causes the metrics to grow excessively. Do you have any ideas about customizing the configuration for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants