-
Notifications
You must be signed in to change notification settings - Fork 174
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.
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
http.route for client & server both as opposed to only server? #899
Comments
@reyang Could you please help adding [area:semantic-conventions] [semconv:HTTP] labels |
Done. |
HTTP route is not available on the client side and calculating it in the instrumentation is not possible without knowing the pattern. The best we can do here is an opt-in mechanism where it's populated manually or matched using provided regex. |
This attribute as optional is perfectly fine but this is important to be in otel semantics as it’s an important attributes. |
@dpk83 can you share some scenarios of how it can be used and why it's important? |
@lmolkova It's important piece of information to be able to monitor the health of APIs. A lot of services has dependencies on lot of external services and the reliability of individual requests is as important as the overall health of the target service. Most of our systems care about the monitoring health by request and the request route is important piece of information to be able to create dashboards and setup alerting on per request basis. How a user sets is can differ but the convention of what the name of the dimension to use and what the values should be is critical for standardization. As this is pretty core requirement for outgoing request metrics, standardizing it is as important as it is for incoming request metric. |
@dpk83 why not just add the route to your instrumentation? Adding this to the semantic conventions and enforcing all users to have their clients include the route is unreasonable. Many users do not want the high cardinality route tag of a client to pollute their observability systems. |
It's not forcing as we are talking about this attribute as I was actually also going to question making the http.scheme as required, as we have 1000s of services running in our org and not one of them utilizes this today in metrics. |
The Suggesting OTel sponsored instrumentation should include this attribute on the client side means all users will have a cardinality explosion on the distinct telemetry they send to their observability systems. It is not something we should do. |
Regarding route, I still don't understand how solution would look like. Route is not readily available on the client. Client have Information about |
How's that different than http.route for incoming requests? Why do we have it for incoming request then? |
In fact I am fine if it's not added to otel semantic convention as long as that stand doesn't change in future. i.e. we can go ahead and standardize this additional tag for our services as long as there is an assurance that in future otel semantic convention won't add it and thus we won't have to change all services to adapt to it. |
@dpk83 for incoming requests it calculated by the HTTP framework. E.g. ASP.NET Core calculates it (knowing all the routes in the server app) and instrumentation takes readily available data. HTTP spec explicitly says that route can only be populated when available.
If a pure http-listener kind of server app is used that does not support routing, it's not available on server and not populated. Any of these requirements do not prevent user apps from adding any attributes anywhere manually. WRT stability, this document is experimental and might change. |
I understand what @dpk83 said about:
And I have exactly the same issue with dashboards: aggregation. @lmolkova is right, this should not be created on clients. But I would like to validate if injecting |
What did you expect to see?
http.route is only present for server. I expect a similar tag for client also to capture the request route.
Additional context.
Add any other context about the problem here. If you followed an existing documentation, please share the link to it.
The text was updated successfully, but these errors were encountered: