vanguard-go icon indicating copy to clipboard operation
vanguard-go copied to clipboard

Feature Request: Handler callbacks

Open rauanmayemir opened this issue 11 months ago • 4 comments

Currently, transcoder handlers are opaque and there is no way to pry into what's happening until we get into grpc handler.

One use case I have in mind is opentelemetry. I start the tracer before transcoder with no knowledge about my routes so my traces explorer will be full of unique transactions like:

GET /package/version/entity/foo1?param=bar
GET /package/version/entity/foo2?param=baz

If I had access to route matches in transcoder, I could have set http.route attribute or anything else from the http rule.

I've thought about not starting the tracer at all before reaching grpc handler, but it's not always possible. Some things are meant to happen with a non-transcoded request, like logging or authz (not ideal, but that usually depends on url paths like /api/admin/entity/*). And some of those things benefit from the knowledge of transcoder metadata (like detecting route tags from http rules).

rauanmayemir avatar Jan 21 '25 06:01 rauanmayemir

This has been something we've discussed a lot. The main challenge was the potential for drastically increasing the surface area of the API and the extra surface area for bugs with how user-provided code would be interacting with the transcoder.

A very early version of callbacks existed in older versions of the repo, but we got rid of it before open-sourcing in an API overhaul that attempted to make the API more clear and also much smaller/simpler: https://github.com/connectrpc/vanguard-go/blob/5517025c211a2d763f11ef300e1dec8b551d349a/vanguard.go#L133-L152

There was also a draft PR to introduce the idea of "interceptors", which would be much more powerful than just callbacks, but also required rather significant changes to the transcoder internals: #86. (Overhaul the internals isn't necessarily a bad thing; the benchmark results are all good. But we mainly didn't want to add API surface area like this, so we didn't move forward with it.)

One thing that both an interceptor and the original callbacks could do is to abort operations with errors, with the idea being that we'd want to be able to plug validation into this. So this library could then be used to build a reverse proxy/API gateway, and it could do semantic validation of the messages before forwarding them to backends.

Your request sounds like it's more for observability, so wouldn't require that kind of support so could likely be added with very small new API. But we need to think about it and the other possible use cases to come up with the best/most coherent user-facing API for this stuff.

jhump avatar Jan 24 '25 17:01 jhump

I agree, I started learning vanguard’s architecture and having callbacks that cover all critical parts won’t be trivial and only complicate things. I believe this could be addressed by implementing some sort of event listener instead having interceptor pipeline. Listener could allow covering all stages with a smaller api surface. (I’ve even seen people calling such things ‘tracer’ in golang ecosystem)

Re interceptors, since we don’t have that now I solved it in a very simple way, I have both http middlewares that fire before transcoder and I also have grpc interceptors that fire on the grpc handler side. So e.g cors, authz happens at http stage and request validation with protovalidate happens in grpc interceptor.

It works great so far, but there’s an extra hassle to make sure we’re responding with correct output (e.g when we need to return 401), so client protocol level interceptors could help with that.

rauanmayemir avatar Jan 24 '25 19:01 rauanmayemir

Stream style interceptors would provide lots of flexibility to users allowing interceptors that change the control flow, and making the decoder and encoder side pluggable. The requests trip through vanguard decoder -> stream -> encoder is focused on net/http requests. This works well for the proxy model that vanguard-go was initial designed for, but when wrapping gRPC services we are always required to re-encode the messages. This is one of the most expensive tasks. Ideally we could implement invoking RPC handler implementation https://github.com/connectrpc/vanguard-go/issues/97 as one of these stream interceptors, removing the re-encoding step entirely. This would be more efficient and avoid any issues with gRPC's ServeHTTP method. Observability could be added ontop of a stream, effectively wrapping a stream like how a net/http handler is wrapped. Or we might need to explore a event callback solution like in connect-go https://github.com/connectrpc/connect-go/issues/665 to better provider metrics like bytes on the wire, etc.

emcfarlane avatar Jan 27 '25 14:01 emcfarlane

I'd be interested in the hooks / interceptors approach. I basically have a grpc-gateway service I was hoping to replace with this but the RESTful API side has some quirks, for example create requests need to respond with HTTP 201 status codes, I need to be able to customise the JSON error response object etc. I can't see a way to hook into to be able to do that currently unless I have missed something. I saw the hooks thing which looked cool but was disappointed to see it had been removed. I guess what I am looking for is a middleware / interceptor style thing to be able to do custom stuff if / when I need to.

krak3n avatar Apr 02 '25 16:04 krak3n