Print response headers on error responses too #344
Conversation
This would be super helpfull during debugging. It is slightly unclear whether this should be printed on stdout or stderr .. so I'm opting for the easiest option and printing it as is.
| } | ||
| bs, err = json.MarshalIndent(outSerialized, "", " ") | ||
| if err != nil { | ||
| out.Fatalf("Failed to convert map to JSON: %v\nMap: %+v\n", err, outSerialized) |
There was a problem hiding this comment.
Would that cause yab to quit? Some combination of non-serializable header could break otherwise working yab. Sould it be a non-quitting warn instead?
There was a problem hiding this comment.
This is a defer - so we still print it out last. I don't see how this could affect yab qutiting.
Moving it up here to achieve the opposite - out.Fatalf("Failed while making call: %s\n", buffer.String()) will cause a quit, but since we're in a defer I'm gonna get my headers out.
There was a problem hiding this comment.
We have headers+body now. If body marshals fine but header fails, that will change the original functionality and nothing will be printed (unless I'm misreading something somewhere).
It might be better to have separate marshaling for each, and if header fails skip it, printing body only to preserve backward compatibility.
There was a problem hiding this comment.
Oh, I see now.
So.
- We'll always print the whole response - it just won't be serialized. This in itself might be good enough for most cases? (most structs should be serializable?)
- Headers are strings, so the serialization would need to fail for strings - I tried to break it (https://play.golang.org/p/7DsOl61mQRU), failed, but I guess it's possible (?)
- Most importantly - moving headers to a separate output can be a breaking change too, right? They're currently printed as the same dict.
Unless we do something like:
out.Printf("{", bs)
if len(headers) {
out.Printf(headers)
}
if len (body) {
if len(headers) {
out.Printf(",", bs)
}
out.Printf(headers)
}
out.Printf("}", bs)
It would be tricky to preseve the same output - we'd need a json output template of style.
Given the low likelihood of headers not being serializable, the hacks above feel like an overkill - it feels like non-serializable headers are rare enough that it shouldn't be a problem?
Also see the code above that blindly fatalfs if it fails to parse proto error details - it does not even try to handle this case, and print the output.
Happy to make a change if you find it necessary though. If so, please advise how you'd see it - it feels like we'll break some backwards compatibility either way :)
| errs = multierr.Append(errs, err) | ||
|
|
||
| r, err := yarpcResponseToResponse(transportResponse) | ||
| errs = multierr.Append(errs, err) | ||
|
|
||
| return r, errs |
There was a problem hiding this comment.
why do we make this change? This implies yarpcResponseToResponse now has to be able to handle error cases of transportResponse now (nil, or some object with unknown state).
There was a problem hiding this comment.
Well, this because, previously, on error we should just bubble up error and ignore the response altogether.
Since I now want to have response headers to be properly build/populated, we always want to build a proper response object, and return it together with the error.
So, we want yarpcResponseToResponse always to be called, I think.
| if len(response.Headers) > 0 { | ||
| outSerialized["headers"] = response.Headers | ||
| } | ||
| outSerialized["body"] = responseMap |
There was a problem hiding this comment.
The comment // Print the initial output body. is not up do date anymore, please move it to the defer function
| if response != nil && len(response.Headers) > 0 { | ||
| outSerialized["headers"] = response.Headers | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
nit: even if previously we had this piece of code in the function makeInitialRequest, can we expose in another method for better readability? For instance defer printOutputInJSON(outSerialized)
|
Ack, I'll get to this once I'm blocked on something (may be a few
days/weeks)
…On Tue, 6 Jul 2021 at 05:50, Alexandre Wilhelm ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In main.go <#344 (comment)>:
> @@ -472,6 +472,23 @@ func makeContextWithTrace(ctx context.Context, t transport.Transport, request *t
func makeInitialRequest(out output, transport transport.Transport, serializer encoding.Serializer, req *transport.Request) {
response, err := makeRequestWithTracePriority(transport, req, 1)
+
+ var bs []byte
+ outSerialized := map[string]interface{}{}
+ if response != nil && len(response.Headers) > 0 {
+ outSerialized["headers"] = response.Headers
+ }
+ defer func() {
nit: even if previously we had this piece of code in the function
makeInitialRequest, can we expose in another method for better
readability? For instance defer printOutputInJSON(outSerialized)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#344 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACG3UXV5SSDTYD5XN5MOWDTWL3Z5ANCNFSM47KWMQJQ>
.
|
This would be super helpfull during debugging.
It is slightly unclear whether this should be printed on stdout or
stderr .. so I'm opting for the easiest option and printing it as is.