-
Notifications
You must be signed in to change notification settings - Fork 897
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
Remove http.XX_content_length* semantic attributes #2028
Comments
How about filtering for requests that exceed a certain size? Or requests without body (length=0)? |
Ok, true. Still, wouldn't it better served by metrics? |
Also, having those attributes on spans will provide more help in troubleshooting or reproducing problems. You might get answers to those questions: Why do request sizes exceed a certain limit? You are much more likely to get answers to those questions from traces than from metrics. |
assuming metrics are correlated to traces with exemplars, would it be possible to answer all these questions just not for a particular request? I.e. if you have problems with big requests, you'd see an exemplar of it on metrics for request/response size and there will be no need to stamp it on every request. |
@lmolkova Probably true, but the same could be said for http.status_code: "If you have a problem with status 500 requests, you'd see an examplar of it on metrics for status codes". |
@Oberon00 I see your point, I'm not sure status code is the same as request size. Status code indicates a failure and you'd generally want to know about it. Size might be the cause of failure or big latency and I think only the correlation between size and status/latency is interesting. Size alone is rarely important. |
I've always understood there is a (unspoken?) goal to allow spans to be converted to metrics fairly robustly, and is the motivation for work on propagating sampling probabilities among others. So I always assumed the attributes on a span would always be a superset of metrics - though this may not be true. |
I guess it boils down to the stage at which metrics are reported: 1) separately, 2) in instrumentation API or 3) from spans in post-processing, e.g. collector. I think p3 leads to different challenges related to sampling and filtering and metrics reconstruction may not work great with all samplers or make them very complicated, as they start having a state. |
#1769 might be a reference on the sampling algorithm issue - the impression I got is that a hard rate limiting approach isn't appropriate in OTel for the reasons you mention, making it impossible to compute metrics from spans, so an approximate rate limiting approach is used instead. I personally don't need to compute metrics from spans and don't think instrumentation should take the potentially imprecise approach vs directly collecting metrics, but it appears to be important in general to keep the door open for span-based metrics. I guess removing attributes like these content length ones could close the door on it. |
Ah - I just remembered that I had initially added these attributes to the spec 😅 I think my only motivation was to close a gap with the X-Ray data model, which includes them too If these attributes were removed, X-Ray would probably have to use the relatively new |
Came across it again in scope of #2469. Here's how these attributes are populated: #2114 (comment) in Java, .NET, JS, Python and Go.
So one thing I'd like to do is remove *_uncompressed attributes. Then content_length: I'm trying to understand when it doesn't match Unless I'm missing something, we can remove all 4 attributes and let users/distros opt-in into http.*.header.content_length @anuraaga lmk if you still think it'd work for AWS. @Oberon00 @iNikem @pyohannes please share if you have any objections. |
I think that should work for X-Ray. As an anecdote, I have found good value from response length and uncompressed length in metrics when using the Armeria server library which exports these natively. It's true that black box instrumentation will generally not be able to handle the compression, but native instrumentation potentially could. Just food for thought on whether OTel semantic conventions are supposed to apply even when they would be hard to support in instrumentation libraries but could be in frameworks directly. Either way I would remove now and potentially add back only in metrics if needed later to avoid slowing down semantic convention stability. |
Would the opposite situation also be plausible, i.e. HTTP clients that always decompress and give no possibility to access the raw length? |
I believe HTTP clients always know the raw length (at least from Content-Length header, which is raw payload size), but if we define our attributes as something smarter, then it's up to the instrumentation author to interpret it.
Agreed and, also to @anuraaga 's point, - we probably would find metrics around content length very useful. And when it happens, we might need to return |
after some back and force, I kept But given how unreliable uncompressed length population is and that OTel HTTP instrumentations don't usually populate it, I removed Since http request and response size metrics are defined now, I believe the only open question here is if we want to have a special name for attributes ( since @iNikem thoughts? do you feel we can close thin one? |
Http semantic convention defines 4 attributes which are very different from all others. Namely
http.request_content_length
,http.request_content_length_uncompressed
,http.response_content_length
andhttp.response_content_length_uncompressed
. I think this information should be captured as metrics, not as span attributes. I cannot imagine a use-case where anybody would want to search/filter by this attribute or use it in any other "dimension"-like way. This is clearly a metric.The text was updated successfully, but these errors were encountered: