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

WIP: Prototype simple flag

Open bufdev opened this issue 6 months ago • 11 comments

This is re: https://github.com/connectrpc/connect-go/issues/848 https://github.com/connectrpc/connect-go/discussions/421. This is the simplest possible version, and is just done as a prototype for discussion. Very little here should make it to merge: the doubling-up of the methods to add Simple is pretty lazy, the naming isn't overly consistent, etc. The code as-is is just a demonstration.

The general concept is to effectively just model what people know already. The interface is very similar to grpc-go in this case. It's a misuse of context.Context in the general sense, but basically every RPC library (except connect-go, but including grpc-go, which is what people are typically migrating from) does this. This results in unary-only Handler interfaces matching grpc-go and Twirp, and Client interfaces almost matching grpc-go (and matching Twirp). This is (in my opinion) generally a feature, but there are caveats: people using the grpc-go metadata package will be tripped up when migrating, when they realize their headers/trailers aren't being propagated. Given how few people actually interact with headers and trailers, however, this feels like a minor downside that can be documented as part of migration.

A final version of this will likely not be flag-based, we'll want to carefully choose how to expose this:

  • A new plugin, such as protoc-gen-connect-gosimple.
  • Generate to a separate package, such as .*connectsimple.
  • Generate with a new interface name in the same package, such as .*SimpleClient.

Whatever we do, we do not want to v2 and will never break existing users.

EDIT: This is closer to good to go now, and I'm actually more in favor of the flag option than I was. However, we're still not there. If we end up doing this, better documentation and testing is needed. We'll also need to update documentation on connectrpc.com, and on the Buf side we'll want to release a new plugin connect/gosimple that uses this flag.

bufdev avatar Jun 07 '25 05:06 bufdev

Strong +1 from me.

After a good few years and many servers/clients written for work and personal projects, my take is:

  1. Unfamiliar and verbose migrating from twirp-go/grpc-go
  2. Wrapping with New{Request,Response} is tedious, and so daily ergonomics suffer
  3. Optimizes for the rare case, most code doesn't touch headers. So every call site pays the complexity cost
  4. Context propagation is fine. For the rare times you need headers, passing via context is acceptable!

TL;DR - developer ergonomics matter more (esp. in this case) than avoiding the "context misuse" when that misuse is already the established patterns developers expect.

mfridman avatar Jun 07 '25 14:06 mfridman

Strong +1 from me as well. Wrapping causes a lot of extra, unneeded boilerplate code.

Alfus avatar Jun 08 '25 20:06 Alfus

Generate to a separate package, such as .*connectsimple.

Does it make sense to also use this as an opportunity, likely configurable since I understand why we do it this way currently, given this "simple" interface to generate into the same parent package by default to mirror gRPC behavior?

mattrobenolt avatar Jun 08 '25 22:06 mattrobenolt

Speaking for myself, I still feel relatively strongly about generating to a separate package by default, but maybe could be convinced otherwise. I think this was a big miss on grpc-go's part, it sets up a whole host of problems w.r.t. plugin interactions. I get where your mind is though - grpc-go migration, import simplicity.

Note that with the implementation as-is in this PR now, it is just flag-based and generates to the same package as before. And this is configurable via package_suffix, including setting to empty to copy grpc-go semantics of generating to same package. I'd argue to keep it this way - two independent flags - as opposed to introducing special semantics w.r.t package if you set the simple flag.

bufdev avatar Jun 08 '25 22:06 bufdev

A separate overall note: the implementation as-is fully allows and supports multiple protoc-gen-connect-go calls to independent packages, so for users who want both interfaces available, this is trivial. In fact, this is what this repository is now doing as of this PR: look at buf.gen.yaml, which generates to internal/gen/generics and internal/gen/simple.

bufdev avatar Jun 08 '25 22:06 bufdev

Oh yeah, I agree with the choices entirely to keep it a separate package, but more thinking in lines of expectations that users have.

This API that is being addressed here and the separate packages are the biggest real complains I've observed from discussions.

Even though we agree a separate package is better, I'm more looking at it from a lense of default expectations.

If I'm a user and I want this "simple" API, I likely am just wanting a grpc-go experience to align and be as much drop-in as possible.

I definitely don't hold a strong opinion here tho. I'm just pushing for a flip in the default behavior when opting into this simple API. But I think it's fine either way.

mattrobenolt avatar Jun 08 '25 22:06 mattrobenolt

Given this isn't a Buf product, I'm not as concerned with that. But with respect to the BSR, Buf would just host it as a new plugin "connect/gosimple" that called this option.

I don't want to add a suffix to the interfaces - that's making the simple interfaces more complicated. Presenting a single package with two different types of clients and handlers makes the problem worse IMO.

bufdev avatar Jun 09 '25 14:06 bufdev

Coincidentally, this would also solve #850 since it provides a way for unary handlers to explicitly set header or trailer metadata, independent of the metadata set on a returned error.

jhump avatar Jun 09 '25 14:06 jhump

I was thinking a bit more about this: a flag named "simple" may not be the most intuitive way to turn this on. It's not that it's "simpler" than the other way; it's just an alternate approach. While it may feel simpler to not have the wrappers, it's actually less simple/less obvious when you do need to interact with metadata.

So maybe it's more like "metadata_mode=context" and the existing/default behavior is "metadata_mode=wrapper". (I don't particularly like "metadata_mode", but I'm having a hard time deciding what to call it. I similarly don't actually like the name RequestInfo in my other suggestion/proposal, but am not sure what the best name would be...)

jhump avatar Jun 09 '25 15:06 jhump

I'd have to hard disagree on this one.

The whole thing here is that almost no one interacts with the metadata. It's significantly more complicated for the vast majority of users to have a Request/Response wrapper - if it were simpler to use the existing system, no one would be clamoring for this feature.

The option should be clear, concise, and the closest thing to get users to use it as a suggested default (short of v2'ing connect-go and making it the actual default, which we do not want to do). A boolean option simple accomplishes that.

When I read context vs wrapper as two option values, I do not know what these mean or why I should choose one or the other without reading the docs - and it's a fair assumption that most users won't read the docs. The vast majority of developers will be served best by using the simple API - it gets them what they need, in the way they expect it. Calling this option simple will pull gravity in this direction.

bufdev avatar Jun 10 '25 00:06 bufdev

This looks great - removing the request/response wrappers would remove a lot of boilerplate. My main feedback was related to supporting Peer/Spec and avoiding storing multiple values on the context, but @jhump covered that above: https://github.com/connectrpc/connect-go/pull/851#discussion_r2135765436.

pkwarren avatar Jun 10 '25 19:06 pkwarren

Looking at the code, I do not understand what sentinelContextKey does - this needs explanation, everything else I think is ready.

bufdev avatar Aug 18 '25 20:08 bufdev

Looking at the code, I do not understand what sentinelContextKey does - this needs explanation, everything else I think is ready.

Added some explanation in https://github.com/connectrpc/connect-go/pull/870

smaye81 avatar Aug 19 '25 14:08 smaye81

LGTM! Let's ship it!

bufdev avatar Aug 20 '25 19:08 bufdev

LGTM! Let's ship it!

I'll get the docs PR up to date and ready for review.

jrinehart-buf avatar Aug 20 '25 19:08 jrinehart-buf

When will we see this released?

staugaard avatar Sep 18 '25 02:09 staugaard

@staugaard this has been released in v1.19.0

emcfarlane avatar Sep 26 '25 14:09 emcfarlane