-
Notifications
You must be signed in to change notification settings - Fork 986
Response/Request Size GRPC Metrics Instrumentation #14342
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
base: main
Are you sure you want to change the base?
Response/Request Size GRPC Metrics Instrumentation #14342
Conversation
...test/java/io/opentelemetry/javaagent/instrumentation/armeria/grpc/v1_14/ArmeriaGrpcTest.java
Outdated
Show resolved
Hide resolved
...test/java/io/opentelemetry/javaagent/instrumentation/armeria/grpc/v1_14/ArmeriaGrpcTest.java
Outdated
Show resolved
Hide resolved
1256e3d
to
4387287
Compare
equalTo(RPC_RESPONSE_BODY_SIZE, requestSerializedSize), | ||
equalTo(RPC_REQUEST_BODY_SIZE, requestSerializedSize), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anyone know why the server spans are using request size for rpc.response.body.size
and response size for rpc.request.body.size
? This is how the original PR was written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, are the two of you working together on this? I just want to understand why we have two PRs for the same thing and make sure we're all on the same page. I think the reason the other PR stalled is because there were some open questions about semantic conventions, and as @crossoverJie said we might need to revisit and push on that before we can move forward with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the two of you working together on this?
No, the other PR is just very out of date I and I wanted to get this done so I rebased it myself. I assumed it was abandoned :)
I think the reason the other PR stalled is because there were some open questions about semantic conventions, and as @crossoverJie said we might need to revisit and push on that before we can move forward with this.
AFAIK these are already in the semantic conventions:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK these are already in the semantic conventions
What you have in the semantic conventions is rpc.server.request.size
, rpc.server.response.size
, rpc.client.request.size
and rpc.client.response.size
metrics. Semantic conventions don't seem to define the span attributes rpc.request.body.size
and rpc.response.body.size
. Either these attributes would need to be added to semantic conventions or the api that is used to produce the metrics would need to be changed so that it could include values that are not span attributes. Our convention is not to emit telemetry that is not in the spec in default configuration.
To me
equalTo(RPC_RESPONSE_BODY_SIZE, requestSerializedSize),
equalTo(RPC_REQUEST_BODY_SIZE, requestSerializedSize)
looks like a bug since both the request and response size are the same.
@jaydeluca looks like the failing tests are dead links unrelated to these changes. I'm still unsure of this though. |
Adds
rpc.request.body.size
andrpc.response.body.size
attributes. This is just an updated version of #11833.