Proposed new routing format: /package/service/method about twirp HOT 36 CLOSED

Internally at Twitch having the /twirp prefix is useful to easily tell apart twirp services from others, but yeah, it makes sense to remove this prefix on other organizations. The same can be achieved by using some common prefix on package names.

I like the proposed path schema: <package>/<service>/<method> 👍

from twirp.

@spenczar I think the difference between having the "address" of the service ashttp(s)://host:port/ vs http(s)://host:port/path is negligible. They still need one string, one piece of info, not two. I meant that they would be combined anyway. There is really 4 pieces of info there as well, as you may be running this over HTTPS or HTTP.

I am slightly biased as I am thinking of using twirp exposed over the public internet from the same web server as the rest of a web app, so would want a customizable prefix.

from twirp.

IMHO we should just not enforce a prefix -- as commented here you can mount your twirp service wherever you want (including at /twirp). Then it would be a runtime/compile-time decision, not code-generation time decision.

Of course, as this is a backwards incompatible change in itself (unless you do choose to mount at /twirp) it make little difference if other breaking changes occur as well - such as the change of delimiter, although in my mind that doesn't accomplish anything.

EDIT: The client would need a baseurl anyway, it would now just need the mounted prefix as well.

from twirp.

I agree that Twirp services should be "mountable" on any prefix. For consistency, we can call it "baseURL" in the client (instead of "addr" or "host").

This will also solve a somewhat surprising behavior with the current router switch, that only matches requests in the root url even if the twirp httpHandler was mounted with a mux on top of another base path. I think it makes sense to asume that Twirp services are mountable on any "baseURL", and then clients receive that "baseURL" as parameter. This also makes sense for other languages, not just Go.

If for v6 we keep the URL format for Twirp in the form <package>.<Service>/<Method> and allow to mount on any url prefix at the same time, the change upgrade will be easier: to support v5 clients just mount the v6 service on /twirp. But now any new v6+ service can be mounted anywhere, which also solves the problem of collisions with other services (e.g. gRPC)

from twirp.

@doapp-ryanp I think we have consensus here, we will change the routing in the way listed. We should make a PROTOCOL_v6_DRAFT.md document, or something - clearly labeled as a draft, it would be our plans for v6.

However, when it comes to actually implementing that new spec, I would like a v6 release to include streaming support. That requires two things:

• Agree upon a wire protocol, which is #3. I think we have consensus there.
• Establish the Go API, which is #70. This is pretty unclear.

I think that, for now, we do a v6-alpha release which includes just routing changes and a sketch for a possible streaming API, with very loud notices that the streaming API be broken. We'll need to maintain a separate branch until we lock in the streaming API.

So, immediate action items:

• Make a new v6 draft of the spec, both in docs/spec.md and PROTOCOL_V6_DRAFT.md, which includes routing changes. (#80)
• Make a new v6_prerelease branch.
• Implement v6 routing in the v6_prerelease branch.
• Add a description the new streaming protocol to the v6 spec draft.
• Implement streaming in the v6_prerelease branch.

You should be good to use twirp after step 3, @doapp-ryanp, if you're comfortable working with a prerelease version while the streaming API gets some use. The only unstable part of the prerelease would be streaming APIs; we would promise not to break other stuff.

from twirp.

I reached out to @spenczar about this via DM. Just sharing some of my thoughts on this.

As part of my dta.gov.au team, we're evaluating some API toolkits/specs/etc. that could be used in various whole of government solutions.

Whilst tooling/SKDs should mask the path most of the time, the reality is that if we build APIs that are used by other gov departments/services, it's possible that they won't or can't use any of the SDKs that would mask the path.

Instead we'd give them some sort of "documentation" for how to construct HTTP requests to call the API.

Putting something like:

Make a POST request to https://a-thing.gov.au/twirp/x.y.UserService/CreateUser

in the documentation might not be received that well even though you and I know it's just a word. It may not give confidence that your service is taking things seriously, etc.

I don't have a strong opinion on what the best format is for the path, but I wonder if at the least it could just use something else instead of /twirp (including /twirp.meta/Reflector), e.g. trpc.

from twirp.

from twirp.

I have a request and a suggestion for the migration, regardless of the format we choose.

First, we release a version (v5.x.0) where servers can understand both v5 and v6 URIs but where clients still send v5 URIs. An org that is currently using v5.1.0 could safely upgrade all clients and servers to v5.x.0 in any order. After finishing that migration, they could then upgrade their clients and servers to v6.0.0, again in any order.

Second, I suggest that v5 path prefixes be named "v5" rather than "legacy". Someday v6 may be legacy too. For example:

const (
  HaberdasherV6PathPrefix = "/notreal.example/Haberdasher/"
  HaberdasherV5PathPrefix = "/twirp/notreal.example.Haberdasher/"

  HaberdasherPathPrefix = HaberdasherV6PathPrefix
  // or `... = HaberdasherV5PathPrefix` during the transition
)

from twirp.

@mocax, I really don't want to add a boolean flag. Flags make the behavior dependent on the letters somebody typed in on the command line when they generated the code rather than dependent on the proto document + the Twirp spec. That makes the system much less predictable.

If we do this, we'll just do it for clients, and servers will be backwards compatible. I agree that we should do it sooner than later, while Twirp is still young enough to handle spec changes.

from twirp.

Terrible idea for migration simplicity: a v5 RoundTripper in a box that tries the old path if the new path doesn't work.

from twirp.

One advantage of the /twirp/ prefix is that it made routing and middleware layers easier to use. For example, I could forward all of /twirp/* to one middleware layer and still put other HTTP endpoints on the same http.Server

from twirp.

@cep21 That's true, you'd need to wrap each twirp service with your middleware wrapper, instead of being able to do it once.

Most programs only serve one Twirp service, but some might serve multiple. This would mean an extra line of code for each one. That seems like a pretty mild loss.

You could encapsulate it with a function that loops over your services if it's really bad, like this:

type middleware func(http.Handler) http.Handler

func mountTwirpServices(mux *http.ServeMux, svcs map[string]http.Handler, m middleware) {
  for prefix, svc := range svcs {
    mux.Handle(prefix, m(svc))
  }
}

from twirp.

Two new concerns with /<package>/<service>:

• It's legal to have no package. Should we really be going to /<service> then? We run a much higher risk of colliding with other stuff.
• Lots of .proto files are already written now in prominent projects like Envoy. There is a real risk of collision for them, as they probably didn't pick package names with this in mind. That could easily lead to collisions in the first path element, at least (imagine /envoy, for example).

In light of this, @wora suggested still using a prefix. Two options:

• /$rpc/<package>/<service>/<method>
• /trpc/<package>/<service>/<method>

We can be pretty sure that $rpc will not collide. Most people would think it even needs to be escaped (it doesn't). But it's a little ugly.

trpc is prettier, but has a small risk of collision, and returns us to trademark fears, advertising concerns, etc.

I lean a little towards /$rpc as a prefix to just be done with this whole thing. What do others think?

from twirp.

I prefer to have a customizable prefix (does that what $rpc mean?). One of the use case is if we want to have multiple Twirp services in one app. Each can handle different prefix. I personally don't mind about how the path for package and service is structured.

from twirp.

If consensus is use prefix, why not just stick with twirp or twirpc? The latter is a "cute" way of conveying both rpc and twirp.

I have no opinion on the naming, just as long as / is delimiter instead of the .s that are used today.

from twirp.

@fajran No, $rpc wouldn't be configurable. It would be a hardcoded string.

@doapp-ryanp, twirp and twirpc both have problems for Very Serious environments (see #55 (comment) above). They also raise eyebrows about trademark concerns at big companies.

from twirp.

@wora, a significant problem with $rpc: because people don't expect $ to be legal in URLs, lots of tools don't handle them properly. This is especially bad because $ is a special character in PHP, regular expressions, and many shells. Putting it in a URL makes life significantly more difficult for people.

Many examples of confused people dealing with dollar signs in URLs that pop up on google:

• Can't figure out how to curl a URL because dollar sign
• Can't get a redirect to work because they don't know that their backend is treating input as a regex
• URLs containing $ are not highlighted properly on stackexchange because link autodetection doesn't understand them
• Can't mount a handler correctly because a mux interprets input as a regex

I am sure there would be more. This is enough for me to say $ in a URL is a bad idea.

from twirp.

@thechriswalker Yes, the issue is making sure clients know the right prefix. It would be best if clients only need a hostport, not a prefix too.

Your point that the server can be mounted at a special place, and the client can include that prefix if they must, is a really good one, though. That might mean we have coverage for the exceptional cases where we would collide, and where a collision is unavoidable because changing the package is considered unacceptable.

from twirp.

Trpc feels like it had clear advantages with little downside. I think it is enough away from twirp or twitch that serious business won't mind

from twirp.

@thechriswalker, another good point re: sharing the scheme, host, and port anyway. Your use case, exposing it next to other web app routes, is one we want to support well.

from twirp.

I agree with @thechriswalker. A client needs at least one piece of information to locate the server, and we can standardize the address as the base URL as http(s)://host:port/path. This would solve all issues raised here. I would be super happy with it.

from twirp.

Yes. We have an agreement here. I am drafting the wire protocol spec with Spencer. Stay tuned.

from twirp.

@marioizquierdo, good catch that the router's current switch doesn't permit remounting. If we advise people to mount on a separate prefix if they need to avoid collisions, we will also need to either:

• tell them to use http.StripPrefix, or
• route things based on the requested URL's suffix, which should be in the formal specification.

The second seems more reasonable to me.

from twirp.

@rhysh, your suggestion on names of the consts is clearly better. We'll do it the way you described.

The two-step migration sounds good and interesting too. It's kinda weird that we'd be making a v5 release that handles v6 behavior, though. I suppose HaberdasherPathPrefix would be set to the v5 prefix in that intermediate release?

from twirp.

@rhysh @spenczar Related to my previous comment, if we keep the current path format <package>.<Service>/<Method>, and change Twirp service routing to be based on path postfix, then there's really no need for migration on the clients side.

The v6 upgrade instructions would tell services to also mount twirp on the /twirp route for backwards compatibility.

from twirp.

I think we should change the server code to accept both package.service/method and package/service/method, and encourage people to use later. I think the server change is very trivial and the change does offer long term value to users.

The routing can be done by different proxy layers, there is no chance for us to update them. It is a very tiny cost for us to avoid the pain for customers.

from twirp.

Right, @wora, if v6 servers accept both package/service and package.service, then users can mount at both / and /twirp to get compatibility with v5 clients.

But as @marioizquierdo points out, v6 clients can talk to v5 servers only if we generate them to use package.service.

I think we have agreement that we should drop the /twirp prefix, but let's talk more about the conversion to using a slash between package and service. The cost of this switch is that you cannot upgrade a client to v6 without first upgrading the server (either hopping through an intermediate version like @rhysh said, or all the way to v6). What are the benefits?

@Xe is the one who suggested this - could you talk a bit about the benefits of using slashes here?

from twirp.

A proto package typically represents a single API. You can write a single routing rule per package, without worrying about adding new interfaces to the same package. With the same URL length, / is much better for dispatching than .

For upgrade, we can add a boolean flag to the v6 client generator to produce the old . separator, with a transition window of 6 months. The usage of Twirp probably allows changing the spec now. Or we can stay as is forever.

from twirp.

I don't like to have flag at all. If we decide to switch, we should just do it.

from twirp.

Yeah, no flags, because it also makes multi-language support a lot harder, then all service generators in all other languages will also need to support the flag!

In terms of backwards compatibility, it is fairy easy to make the Go server generator to support both v5 and v6 routes, but that also imposes extra burden on other languages. Should all other languages support v5 routes as well? Ideally Twirp would support scenarios like a client generated in twirp-v5 in Go talking to a service generated in twirp-v8 in Rust. In any case, it is probably early enough to consider this kind of change (actually, is now or never!!!). It is probably totally OK for the future server generator in Rust to only support v6+ clients.

from twirp.

@spenczar close to any decision on this? My team would like to use Twirp in production, however I'm waiting for this change to occur (we are not fans of the v5 routing scheme, for reasons brought up in this thread).

from twirp.

On the subject of changing the routes, what are your thoughts on lowercasing the service and method names? It seems a bit odd to have case sensitivity in a URL.

from twirp.

@nullbio That doesn't bother me too much. It's unusual, for sure, but it shouldn't matter, as you shouldn't be seeing URLs too often anyway. If you want to discuss more, could you open an issue for that topic?

from twirp.

from twirp.

The protocol v6 was archived. The are no current plans to update the route path formats.

For reference, there is more talk about this in #179

from twirp.

PR #263 implements a solution for the twirp prefix in a backwards compatible way. It updates the wire protocol from:

/twirp/[<package>.]<Service>/<Method>

to:

[<prefix>]/[<package>.]<Service>/<Method>

The <prefix> is now configurable in clients and servers, using "/twirp" by default.

from twirp.