Print response headers on error responses too

[rabbbit]

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.

[bryndin]

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?

[rabbbit]

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.

[bryndin]

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.

[rabbbit]

Oh, I see now.

So.

1. 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?)
2. 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 (?)
3. 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 :)

[AllenLuUber]

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).

[rabbbit]

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.

[Dogild]

Can you please update the summary of the diff with example of:

• TChannel/Thrift errors with headers output
• gRPC/proto errors with headers
• HTTP/json errors with headers

[Dogild]

The comment // Print the initial output body. is not up do date anymore, please move it to the defer function

[Dogild]

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)

[rabbbit]

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> .