[gRPC] Update gRPC Semantic Conventions to 1.42.0#4508
Conversation
Fix 1.41 Semantic Conventions for gRPC: - Remove `rpc.service`. - Update `rpc.method` to include the service name. See open-telemetry#4338 (comment).
Maybe you could add this after comma |
Add extra link.
|
i did some additional changes to adhere to 1.41 grpc semantic conventions, see this PR: martincostello#4 |
Thanks. I'll look to cherry-pick the relevant parts once #4370 is merged. |
Pull request was converted to draft
- Update to version 1.42.0 of the (g)RPC Semantic Conventions. - Address feedback. - Remove redundant code. Co-Authored-By: Matus-p <81446121+matus-p@users.noreply.github.com> Co-Authored-By: Piotr Kiełkowicz <pkielkow@cisco.com>
|
@Matus-p I incorporated your feedback from my fork. Could you sign the CLA please? Otherwise I'll need to update the latest commit message to remove your co-authorship. |
- Set the `error.type` attribute where possible. - Add a test that illustrates a solution to open-telemetry#2066.
|
Codex findings [P1] src/OpenTelemetry.Instrumentation.GrpcCore/RpcScope.cs:208 - GrpcCore still does not apply 1.42 gRPC client error rules. StopActivity(int) only writes rpc.response.status_code and stops, so a disposed/cancelled client call still exports CANCELLED with status unset and no error.type. v1.42 says client spans treat every non-OK status as an error. The same path also means exception failures reach StopActivity(...) without setting error.type. |
- Handle "other" methods correctly. - Avoid overwriting the status if already set. - Set the `error.type` attribute consistently.
|
One more |
Fix incorrect error handling.
|
Going to look at adding some weaver tests on top of the latest commit for additional verification. |
Validate gRPC semantic conventions with Weaver.
Kielek
left a comment
There was a problem hiding this comment.
One more comment from Codex... I am not sure why it cannot handle at once.:
P1 test/OpenTelemetry.Instrumentation.GrpcNetClient.Tests/GrpcTests.client.cs:374
This assertion still reads SemanticConventions.AttributeRpcGrpcStatusCode (rpc.grpc.status_code), but the PR now writes rpc.response.status_code. Nearby assertions were updated correctly. This test should assert AttributeRpcResponseStatusCode, otherwise GetTagValue(...) returns null.
Add OpenTelemetry to the tests not the library.
- Fix incorrect assertion. - Fix redundant `#if`. - Make the assertion that fails the test last.
See #4338 (comment).
Changes
rpc.service.rpc.methodto include the service name.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)