Json encoding

[cdevienne]

I would like to be able to use json instead of proto for data encoding.

I can see that the .pb.go files annotates the structs for json encoding, so I guess the only addition would be where the marshal/unmarshal is done.

[mdevan]

Protobuf and JSON have their pros and cos. If you're going to use JSON, you probably don't need .proto files -- anything else would be too confusing.

Anyways, using JSON is out of scope for nRPC. Feel free to fork (nrpc-json?) and try it out. Would recommend using snappy or something similar to compress the messages.

[cdevienne]

.proto files are a great way to formalize the API, even with JSON. And doing so provides a natural step to switch to protobuf encoding when all the clients are ready.

And this is exactly my use case: most of my clients (and server) will be able to use the protobuf encoding, but for a few of them it will not be possible in a near future. For those, I want the server to accept json instead of protobuf, and reply accordingly.

Since proto3 provides JSON mappings natively, I think JSON is not that out of scope for nRPC. Especially when gRPC is designed to support multiple format, and nRPC claims to be like gRPC but over NATS.

My idea is to propose json support as an option of the plugin, which I think is better than to fork and maintain 2 projects that are almost the sames.

[cdevienne]

Here is a preview of what I have in mind: https://github.com/orus-io/nrpc/tree/json-support
In this branch the json is not optional but does not get in the way; a client sending JSON just has to suffix the subject with "-json".

Using snappy is not an option in my case because the client that will need the json encoding will not be able to decode it.

[mdevan]

That looks good. Few comments:

1. Can we use ".json" instead of "-json"? That way we can leave open the possibility of subscribing to "Greeter.*.json" explicitly -- just for the future.
2. Let's use "protobuf" rather than "proto".
3. If the topic does not end in ".json", or ".protobuf", assume ".protobuf" by default.
4. In the generated client code, we can have the encoding as a field in the client, so that we can use:

// caller
c := NewGreeterClient(nc)
c.SetEncoding(nrpc.JSON)
c.SayHello(...)

// helloworld.nrpc.go
type GreeterClient struct {
	nc      *nats.Conn
	Subject string
	Timeout time.Duration
	Encoding string
}

func (c *GreeterClient) SayHello(req HelloRequest) (resp HelloReply, err error) {
	nrpc.Call(..., c.Encoding)
}

5. The Prometheus metrics will need a ConstLabel for the encoding.

[cdevienne]

Thanks for the feedback!

1. I hesitated with .json instead of -json and went for -json because I did not have to change the subscribe subject. But thinking about it, it makes a lot of sense to use .json, and I think you are right.
2. "protobuf" it is!
3. Already protobuf by default (seems natural, and improve compatibility).
4. Good idea, will do that, thanks :-)
5. Ok

I should work on that this week.

[cdevienne]

I made a proper PR that includes your feeback, expect for the prometheus ConstLabel.

I am not familiar with prometheus, but it seems that adding a ConstLabel for the encoding imply we have a metric for each encoding. Am I right?

[cdevienne]

For prometheus, wouldn't a LabelValue be better ?

[mdevan]

Feel free to leave out the Prometheus changes, I'll add it in as another commit.

And you're right, we should be adding it like so:

// The counts of requests handled by the server, classified by result type.
serverRequestsFor{{.GetName}} = prometheus.NewCounterVec(
	prometheus.CounterOpts{
		Name: "nrpc_server_requests_count",
		Help: "The count of requests handled by the server.",
		ConstLabels: map[string]string{
			"service": "{{.GetName}}",
		},
	},
	[]string{"method",  "encoding", "result_type"}) // <-- goes here


// in the handler
serverRequestsFor{{$serviceName}}.WithLabelValues("{{.GetName}}",
	encoding, "protobuf_fail").Inc()