Skip to content

Conversation

@lionskape
Copy link

Which problem is this PR solving?

There is no way to set http.route for metrics

Short description of the changes

Added copy from span of HTTP_ROUTE attribute. It is low cordiality attribute, which is very useful on metrics.

@lionskape lionskape requested a review from a team as a code owner October 21, 2024 00:20
@linux-foundation-easycla
Copy link

CLA Not Signed

@github-actions github-actions bot requested review from david-luna and trentm October 21, 2024 00:20
@lionskape lionskape changed the title fix(instrumentation-undici) Added copy of HTTP_ROUTE to the metrics attribute fix Added copy of HTTP_ROUTE to the metrics attribute Oct 21, 2024
Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for reaching out, I think there may be a misunderstanding what this instrumentation does.

http.route is not a valid client metrics attribute according to the semantic conventions. It is only valid on server metrics. (see HTTP Semantic Conventions).

Also the span attributes don't contain http.route as this is a server attribute for Spans as well. The client woudn't know which route the server uses.

SemanticAttributes.SERVER_PORT,
SemanticAttributes.URL_SCHEME,
SemanticAttributes.ERROR_TYPE,
SemanticAttributes.HTTP_ROUTE,
Copy link
Member

Choose a reason for hiding this comment

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

http.route is not a client metrics attribute. see https://github.com/open-telemetry/semantic-conventions/blob/main/docs/http/http-metrics.md#metric-httpclientrequestduration

Also the attributes in this method don't contain http.route, so there's no point in copying them.

Copy link
Author

Choose a reason for hiding this comment

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

For sure, I have added http.route using custom attribute mechanics to the span.

@lionskape lionskape closed this Oct 22, 2024
@lionskape
Copy link
Author

lionskape commented Oct 22, 2024

@pichlermarc thank you. So, I will purpose an mechanic to add custom attributes for metrics in another PR.

It is very strange for me, that http.route is not applicable to client metrics. It is very common pattern to has low cardinality label on request metrics (to determine on "where does the request goes"). In my case - it is almost useless metric, without http.route.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants