Add attributes request/response body size for rpc #1281
Conversation
model/registry/rpc.yaml
Outdated
| type: int | ||
| stability: experimental | ||
| brief: "The size of the client request payload body in bytes." | ||
| brief: "The size of the request payload body in bytes. For gRPC, this value is calculated through protobuf (including unary and streaming calls)." |
There was a problem hiding this comment.
you can add grpc-specific note on grpc only by referencing that attribute and updating the brief in
Also, please provide the description of what the size is (not how it's calculated). If there is some protobuf documentation that's useful to link, let's link it.
There was a problem hiding this comment.
I add some information but only found link related to Java, I cound't find a description of the body.size on the protobuf official website.
Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Liudmila Molkova <[email protected]>
Co-authored-by: Liudmila Molkova <[email protected]>
|
|
||
| **[1]:** Instrumentations SHOULD require an explicit configuration of which metadata values are to be captured. Including all request metadata values can be a security risk - explicit configuration helps avoid leaking sensitive information. | ||
| **[1]:** The request body size represents the serialized size of the message payload that is sent over the wire. In case of streaming calls, it accounts for the original message in the request and does not include size of the consequent message stream. This value can usually be obtained from protobuf API such as [AbstractMessage#getSerializedSize](https://protobuf.dev/reference/java/api-docs/com/google/protobuf/AbstractMessage.html#getSerializedSize--). |
There was a problem hiding this comment.
I'm curious what gRPC team (summoning @yashykt) think about this.
I know we avoided having this value as a metric because of possible misinterpretation of behavior between streaming/unary, but having this only used for Span generation may be ok.
Is it possible that users could try to aggregate over this value and lose valuable o11y because of it? Should we have a "is_streaming" or some such flag on Spans so we can make sure we don't aggregate things and make false assumptions?
There was a problem hiding this comment.
Assuming we record metrics, we'd need to record direction as a part of the metric name - something like rpc.sent.message.size, rpc.received.message.size (histograms).
We could use similar naming (rpc.sent.message.size, rpc.received.message.size ) for these attributes and replace existing ones (rpc.message.*_size) populated on the events.
Unary calls then could be recorded without events - there is nothing(?) useful on the unary messages besides size anyway (direction is now part of the name and index is always 0). Having everything on one span would be easy and convenient for the simple case.
Streaming calls would not have size attributes on spans - they would always be reported on events to avoid potential ambiguity/confusion.
Would this work? @jsuereth @crossoverJie @yashykt
There was a problem hiding this comment.
The wording here that says "message payload that is sent over the wire" is kinda confusing because after a payload is serialized, we could compress it, translate it to HTTP2 using https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md and then encrypt it as well.
The part where it says "In case of streaming calls, it accounts for the original message in the request and does not include size of the consequent message stream." also seems like a big restriction.
I'm slightly confused whether this is meant for metrics or tracing, but I'll assume that this is for tracing. Refer to https://github.com/grpc/proposal/blob/master/A72-open-telemetry-tracing.md, for the gRPC team's design. Essentially, the gRPC team decided to always use events for recording message sizes irrespective of whether we are using unary or streaming. This keeps things sane and as a side-benefit, if we can record separate events for the compressed and uncompressed size, this also allows us to gain some insight on when a message was compressed/decompressed and how much time that took.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Any updates? |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Ref: open-telemetry/opentelemetry-java-instrumentation#11833 (comment)
Changes
Add attributes:
Merge requirement checklist
[chore]