Drop @type from Connect error detail debug string #688
Conversation
…e in error detail debug info
akshayjshah
changed the title
akshayjshah
changed the title
protocol_connect.go
Outdated
| debug, err := codec.Marshal(d.pb) | ||
| if err == nil && len(debug) > 2 { // don't bother sending `{}` | ||
| wire.Debug = json.RawMessage(debug) | ||
| if wire.Value != "" { // don't bother sending `{}` |
There was a problem hiding this comment.
You'd know better than me, but I think that the zero value of some of the WKTs (like timestamps) produces useful JSON output. I think we'd be okay without this check: sending the zero value of an error detail feels like an uncommon enough case that we don't need to optimize for it.
There was a problem hiding this comment.
🤷, it's debatable how useful that is. But I had changed it because a value that is two or fewer characters isn't always {} with WKTs, too. While it may be a contrived (and not realistic) case, a google.protobuf.IntValue could serialize as the JSON value 1.
I guess I can change the comparison to directly compare the resulting JSON output to "{}", but had done this hoping it was a convenient way to skip the serialization step. (If you see that the value property for a WKT is "", it is intuitive enough that it's a zero value whose debug value is not particularly valuable?)
There was a problem hiding this comment.
Also, I just realized that a non-zero value for google.protobuf.Value could still produce {}, so it might be confusing that we just drop the debug key in this case. (Admittedly, I can't think of a legit reason to use an error detail of type google.protobuf.Value...).
But perhaps the best check is to skip the key if both wire.Value == "" && serializedJSON == "{}".
There was a problem hiding this comment.
I implemented what I put in that last comment. WDYT?
There was a problem hiding this comment.
I think my initial urge to skip sending {} was misguided. On balance, it seems better to just send the debug info, whatever it may be; I don't want to rely on end users understanding the semantics of zero values and protojson.
…pose pbInner; improve condition for when to skip debug key
@akshayjshah, sadly, yes. This will be fixed in the next release candidate of conformance tests. I noticed this happening in connect-kotlin, too, and merged a change to relax the timing of this test, to make it less likely to flake: https://github.com/connectrpc/conformance/pull/761/files#r1462481695 |
This makes the output of a connect-go server look more like the examples in the docs. For example, this section shows this example:
{ "code": "unavailable", "message": "overloaded: back off and retry", "details": [ { "type": "google.rpc.RetryInfo", "value": "CgIIPA", "debug": {"retryDelay": "30s"}, } ] }However, actual output from a connect-go server would instead have a "debug" entry that looked like this:
{"@type": "type.googleapis.com/google.rpc.RetryInfo", "retryDelay": "30s"}The type was redundantly stated as it was marshaling the
google.protobuf.Anywrapper to JSON instead of the contained message.This removes the discrepancy. Error output from a connect-go server will be a little more concise and more closely resemble the docs example with this change.