Skip to content

Implement OpenTelemetry metrics#160

Merged
anuraaga merged 5 commits intoconnectrpc:mainfrom
anuraaga:otel-metrics
Mar 10, 2026
Merged

Implement OpenTelemetry metrics#160
anuraaga merged 5 commits intoconnectrpc:mainfrom
anuraaga:otel-metrics

Conversation

@anuraaga
Copy link
Collaborator

@anuraaga anuraaga commented Mar 9, 2026

Signed-off-by: Anuraag Agrawal <[email protected]>
@anuraaga anuraaga requested a review from a team March 9, 2026 05:54
anuraaga added 2 commits March 9, 2026 14:56
Signed-off-by: Anuraag Agrawal <[email protected]>
Copy link
Member

@stefanvanburen stefanvanburen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm based on the upstream RPC metrics spec, just wondering about the error case for clients (I might have just missed that on the first PR; still need more coffee)

client_attrs = spans[1].attributes
assert client_attrs is not None
# Client just sees a ConnectError
assert "error.type" not in client_attrs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this hasn't landed in this PR, but based on https://opentelemetry.io/docs/specs/semconv/rpc/rpc-metrics/#metric-rpcclientcallduration, I guess I'd expect error.type to be set on the client on failure?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this - I had read the details on https://opentelemetry.io/docs/specs/semconv/registry/attributes/error/ but apparently misread them, it does seem clear error.type is populated along with any domain-specific code

@anuraaga anuraaga merged commit e6d572d into connectrpc:main Mar 10, 2026
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants