serviceconfig: support specifying default service config using a native go struct

kazegusuri commented 5 years ago

Use case(s) - what problem will this feature solve?

It seems WithBalancerName is deprecated in https://github.com/grpc/grpc-go/pull/2900 in favor of WithDefaultServiceConfig. WithDefaultServiceConfig accepts only raw JSON, so if we want to use specific balancer statically, we need to use JSON to specify that.

Proposed Solution

Please keep WithBalancerName as is, or another way to specify the configurations instead of using JSON.

Alternatives Considered

Additional Context

kazegusuri commented 5 years ago

@dfawley How do you think of this? Not everyone use ServiceConfig with discovery, so it would be easier if we have another way to specify configs without specifying raw json.

menghanl commented 5 years ago

The main reason to deprecate WithBalancerName is to avoid having two ways to do the same thing. Reason for json is that it's the service config format agreed upon by all the grpc languages.

It is reasonable to support specifying service using a format other than json, maybe a native go struct.

For now, please try WithDefaultServiceConfig. To set balancer to roundrobin, you can set

grpc.WithDefaultServiceConfig(`{"loadBalancingPolicy":"round_robin"}`)

yurishkuro commented 5 years ago

Accepting JSON config does not provide more backwards compatibility guarantees than accepting a config struct. Passing string configs like this is pretty weird, I would recommend code-first API, and if someone wants to drive the config from JSON or any other format, it can be done with a factory function, like ServiceConfigFromJSON() (which is still a poor API considering that there are many ways to source the JSON).

kazegusuri commented 4 years ago

What's the status on this?

zcong1993 commented 3 years ago

any update?

dfawley commented 3 years ago

This is unfortunately not something the team has resources to spend on right now. This will require us to evaluate the existing grpc.ServiceConfig to determine if we should just keep using it, or finish removing it and replace it with something more appropriate.

One reason this is not a priority is that the JSON form should be completely functional to use. Note that there is a proto representation for service config, and it's possible to serialize that to JSON and inject it, so there already technically is a native Go struct for service config.

seeruk commented 3 years ago

I've been on a bit of a wild ride with this this evening. I've seen quite a few conflicting pieces of documentation about service config, and I've seen various deprecation notices too. I'm honestly completely unsure about what I have to specify in this magic JSON string, but I'm just going to try some things and see if they work I suppose. If JSON is the service format agreed upon by all languages, then it must be documented somewhere in full, right? I've just really struggled to find that place currently (I may just be being completely blind though!)

The current solution of passing JSON is indeed quite weird. Not only weird though, as above, it's really difficult to see what you're meant to pass to this function. If it was a struct then it could have comments, deprecation warnings, etc. Right now I'm putting a magic string in that I don't even know is actually right. Significantly worse than that though, I can put anything in there and it'll still compile, and still pass static analysis. Regular Go code with deprecations or removed fields would fail one of those checks and point out exactly what is wrong.

From the above response, is ServiceConfig being deprecated too?

dfawley commented 3 years ago

From the above response, is ServiceConfig being deprecated too?

The grpc.ServiceConfig type is already deprecated, but service configs themselves are not.

For documentation, which is admittedly not ideal, the best sources would be:

Basics: https://github.com/grpc/grpc/blob/master/doc/service_config.md
Field-by-field documentation (including deprecated tags): https://github.com/grpc/grpc-proto/blob/master/grpc/service_config/service_config.proto

Note that we are happy to accept external contributions, so if you feel the documentation is lacking and you think you can help improve it, let us know and we can work with you.

I agree with the sentiments of wanting compile-time failures, but note that you can actually get that if you use the protobuf form and serialize it to JSON to input it to grpc.

I'm just going to try some things and see if they work I suppose.

What are you actually trying to do?

seeruk commented 3 years ago

I agree with the sentiments of wanting compile-time failures, but note that you can actually get that if you use the protobuf form and serialize it to JSON to input it to grpc.

That's a fair point! I'm assuming you have to use the protojson package to encode it in the correct format (similar to what that documentation is saying about field name formatting, etc.)?

Note that we are happy to accept external contributions, so if you feel the documentation is lacking and you think you can help improve it, let us know and we can work with you.

I have contributed before, and would love to do so again, but my time is really constrained right now. If I do find the time though I'll see what I can do. In the meantime I can only offer some observations based on my experience tonight in case someone else beats me to it. So here's my thoughts:

I did find that document, and it actually does do a good job of explaining it if you take the time to read it. If it were to be improved, I'd suggest putting a fleshed out set of all of the options straight in that document instead. Also, it being a markdown file on GitHub made me wonder if it was actually maintained at first - turns out it is indeed up-to-date, but maybe if it was on the website in a place that felt more likely to be maintained, with a fully fleshed out example or full list of options then there'd be no question.
Even with that document, the Go situation is far from ideal. On top of the above about compile-time checks (which you can get around, as you said), with the approach taken it's much more difficult to stay in my IDE to understand how to use the function. Most of the time I can just click through into the definition of a Go function, look at it's Godocs, look at the function argument types, and then figure things out from there. In this case the Godoc doesn't explain it's usage, and the only argument is a pointer to a string.
When you do look at that Godoc, you're also told that it's an experimental API. I'm personally not a fan of having to choose between either an experimental API that "may be changed or removed in a later release" or a deprecated API that will be removed in a later release. I think the ideal place would be to deprecate the old API only after a new canonical approach has been developed - I just think this also adds to that feeling of uncertainty about whether or not you're going with the correct solution. I completely understand not wanting to have multiple ways of doing the same thing though.

What are you actually trying to do?

I've been deploying a gRPC into Kubernetes and looking into the various options for load balancing in that environment. I found this https://github.com/sercand/kuberesolver, and they say to use the round robin balancer strategy, so was just trying to get that in place. I used the WithBalancer dial option but saw it was deprecated which led me here. I've given it a try now and the snippet from that markdown document is what I'm using and it seems to be working well.

I don't mean to have come off the wrong way with this by the way, I just want to be constructive here! I appreciate you taking the time to respond and answer those questions, and for working on gRPC in the first place.

dfawley commented 3 years ago

That's a fair point! I'm assuming you have to use the protojson package to encode it in the correct format (similar to what that documentation is saying about field name formatting, etc.)?

Yes, you'd want to use protojson, and ideally we should also export our .pb.gos for service config since OSS Go proto really needs a single source of truth.

Also, it being a markdown file on GitHub made me wonder if it was actually maintained at first

Perhaps we should link to it from our godoc comments on the WithDefaultServiceConfig function, and then it would be more discoverable and a testament to its freshness, while also improving our documentation. What do you think?

When you do look at that Godoc, you're also told that it's an experimental API.

We should probably remove this. We try to do this by default for all new APIs we add, in case they turn out to not work in practice. The one has been there long enough without being changed, so it seems to be fine to make it stable.

they say to use the round robin balancer strategy, so was just trying to get that in place

FWIW we do have an example of setting exactly this here:

https://github.com/grpc/grpc-go/blob/01ed64857e3146000ec99cdea4f2932204f17cdd/examples/features/load_balancing/client/main.go#L77

Thank you for the feedback.

seeruk commented 3 years ago

Perhaps we should link to it from our godoc comments on the WithDefaultServiceConfig function, and then it would be more discoverable and a testament to its freshness, while also improving our documentation. What do you think?

Yeah, that's a good shout. Removing that experimental comment would also help too, I agree.

FWIW we do have an example of setting exactly this here:

If I'm understanding correctly, that actually is a deprecated approach (though the solution you've posted there is the one I found most people using on GitHub from doing a search of the function name). The one I landed on was this:

grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`)

dfawley commented 3 years ago

If I'm understanding correctly, that actually is a deprecated approach

Argh, I didn't even look closely at it, but you are right. What you have is the preferred form, and we should update our example. Note that, in this case, "deprecated" means "there's another way, which we recommend using instead", not "it will be removed", since we intend to keep backward compatibility forever (for anything not originally marked w/experimental).

grpc / grpc-go