connectrpc
How to access `rpc.system` from `connect.WithRecover` callback
Is your feature request related to a problem? Please describe.
I've created a metric that counts the number of panics that occur in my handlers. I've aimed to align with the [OpenTelemetry Semantic Conventions for RPC Metrics], using the recommended attributes of rpc.service and rpc.method. For completeness, I would like to also include the required rpc.system, but see no obvious way to obtain this information from what is made available in connect.WithRecover.
I can see in otelconnect that rpc.system is derived from connect.AnyRequest.Peer().Protocol (see https://github.com/connectrpc/otelconnect-go/blob/v0.7.1/interceptor.go#L101), but the peer is not included the parameters provided by connect.WithRecover.
Describe the solution you'd like
Consider extending the information made available in connect.WithRecover to fully mirror the values accessible to otelconnect.Interceptor.
Describe alternatives you've considered None at this time
Additional context N/A
On a semi-related note, I've observed that the recover interceptor is a little awkward to compose with other interceptors. For example, I might want to add otelconnect, then recover, then validate, etc. such that I always have observability, but also have some panic protection for as many interceptors as possible. This ordering appears possible if you group interceptors by those that should be applied before and after the recover interceptor. But realising that required a bit of digging into how interceptors are chained together.
One way out of this small issue might be to make the recover interceptor public. Alternatively, one might expose the interceptor via WithRecoverInterceptor and WithRecover becomes WithInterceptors(WithRecoverInterceptor(handle)).
@mattdowdell, was this in a streaming RPC that you observed this issue? For unary RPCs, the spec that is provided to the handlers is the exact same spec that your own interceptors will get. The recover handler is just a simple interceptor and passes along the spec.
But streaming calls are different, and I'm not sure why. I'll put up a pull request to fix streaming calls.
@jhump I observed this in a UnaryRPC.
A unary interceptor seems to get a context.Context and a connect.AnyRequest (see https://pkg.go.dev/connectrpc.com/connect#Interceptor and https://pkg.go.dev/connectrpc.com/connect#UnaryFunc). The latter provides access to both connect.Peer and connect.Spec. The recover interceptor has access to both of these, but only passes on connect.Spec (see https://pkg.go.dev/connectrpc.com/connect#WithRecover).
As far as I can tell both unary and streaming only pass along the spec, but the streaming spec is empty (see https://github.com/connectrpc/connect-go/blob/main/recover.go#L41-L80). I assume that emptiness is because there's no good fallback (edit: or perhaps not given the new PR).
Ah, my bad. I was incorrectly thinking that the Peer was accessed from the Spec. (I was accidentally conflating this with a different awkwardness, how the Query is accessed from the Peer.)
This is unfortunate because adding it would be a backwards-incompatible change. Your idea about directly exposing the interceptor would indeed solve both the composition issue, but also the missing parameter since we could put the new signature there and leave WithRecover unchanged and deprecated.