Add semantic conventions for http content size #641
Conversation
anuraaga
requested review from
arminru,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
| @@ -79,6 +79,11 @@ Note that the items marked with [1] are different from the mapping defined in th | |||
| | `http.status_text` | [HTTP reason phrase][]. E.g. `"OK"` | No | | |||
| | `http.flavor` | Kind of HTTP protocol used: `"1.0"`, `"1.1"`, `"2"`, `"SPDY"` or `"QUIC"`. | No | | |||
| | `http.user_agent` | Value of the HTTP [User-Agent][] header sent by the client. | No | | |||
| | `http.request_content_length` | The size of the request body in bytes. For requests using transport encoding, this should be the compressed size. | No | | |||
There was a problem hiding this comment.
I think using the word "content" here is misleading. The HTTP header "Content-Length" only has the length of the request/response body but you seem to want the size of the whole request including the headers and request/response line? If not, please clarify.
There was a problem hiding this comment.
Ah nope meant to add content, it's similar to the metrics I've seen in other libraries. Tried clarifying it in the text let me know if it's still ambiguous.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for finding this RFC! Yup aligned the terms with it
|
Please make sure to add an entry in the changelog: |
|
@yurishkuro Many RPC are built on top of HTTP but not all. For the ones that are though they will often have a message size that is different from the HTTP payload size, after deframing for example. How size should be filled for streaming RPC is also an open question (per message, total stream etc). So it seems to make sense to decouple RPC from HTTP for size because the two will treat them somewhat different (sometimes they're the same though). |
CHANGELOG.md
Outdated
| @@ -18,6 +18,7 @@ the release. | |||
| - Update api-metrics-user.md and api-metrics-meter.md with the latest metrics API. | |||
| - Normalize Instrumentation term for instrumentations. | |||
| - Change w3c correlation context to custom header. | |||
| - Add semantic conventions for HTTP content length | |||
There was a problem hiding this comment.
Ah this is not really obvious, sorry. v0.5.0 had been released already. Your change, which is not included in a release yet, should go into the "Unreleased" section above. @carlosalberto will then insert a new v0.6.0 header above it once he's releasing it.
There was a problem hiding this comment.
Oops - taking a second look it seems obvious yet somehow treated this as the Unreleased section. Updated
carlosalberto
added
the
area:semantic-conventions
| | `http.request_content_length` | The size of the request payload body in bytes. This is the number of bytes transferred excluding headers and is often, but not always, present as the [Content-Length][] header. For requests using transport encoding, this should be the compressed size. | No | | ||
| | `http.request_raw_content_length` | The size of the raw request payload body after transport decoding. Not set if transport encoding not used. | No | |
There was a problem hiding this comment.
Do we want to be consistent with https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/messaging.md#messaging-attributes and call this compressed ?
There was a problem hiding this comment.
I think you mean uncompressed - sure, I was thinking about that and thought the field gets too long. But I guess we do have some long fields and it's not such a big deal.
There was a problem hiding this comment.
If size of the key is a concern we can use request_size or request_body_size as shorter names.
There was a problem hiding this comment.
Is it a concern? :) I've seen in some other PRs too that it's not really clear right now, so might need to add conventions for the conventions.
Personally I'm good with the current names, especially lean towards using the word content length at least since it's so well understood in the HTTP world.
There was a problem hiding this comment.
My advise is: go with what you feel the best (request_size, request_body_size, or the original, plus compressed), and let's fill an issue so we can discuss what's the maximum recommended key length for an attribute.
There was a problem hiding this comment.
Thanks for the tips - so it sounds like people are ok with the current names so I'm not planning on changing them. Let me know if anyone still has any concerns though!
There was a problem hiding this comment.
Have a couple of approvals and some non-blocking suggestions. Is it ok to merge this?
There was a problem hiding this comment.
As mentioned above I'm fine with either the terminology we have for message sizes or the well-established terms already used in standard HTTP headers as you currently have it. 👍
There was a problem hiding this comment.
Nit: I'd put uncompressed to the end so both options would come up as suggestions while one is typing in their IDE once we have typed spans but that's probably more a matter of taste.
There was a problem hiding this comment.
Yeah that seems like a nice idea, moved to using a suffix
|
Think I hit most comments and have a couple of approvals. Can we merge it in? :) /cc @carlosalberto @bogdandrutu @yurishkuro let me know if you still have any concerns though |
* Add semantic conventions for http content size * Suffix not midfix
This adds attributes for the payload size of HTTP requests / responses. I looked at messaging too for inspiration but have ended up being inconsistent to better match HTTP conventions Whereas messaging has payload and compressed_payload, here I use content_length and raw_content_length. This is because the concept of content length for HTTP is defined by spec so matching it (i.e., using compressed size as content length for compressed payloads) seems to be clearer for users. But I'm happy to tweak as needed.
Fixes #640