Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Print response headers on error responses too #344

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Print response headers on error responses too #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ Changelog
=========

# Unreleased (0.21.0)
* Nothing yet
* New: Print response headers on error responses too.

# 0.20.0 (2021-05-18)
* Add `stream-delay-close-send` option which delays client send stream closure.
Expand Down
20 changes: 17 additions & 3 deletions integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ import (
yhttp "go.uber.org/yarpc/transport/http"
ytchan "go.uber.org/yarpc/transport/tchannel"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/reflection"
rpb "google.golang.org/grpc/reflection/grpc_reflection_v1alpha"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)

Expand Down Expand Up @@ -1190,6 +1190,13 @@ func TestGRPCReflectionSource(t *testing.T) {
Peers: []string{addr.String()},
},
},
wantRes: `{
"headers": {
"content-type": "application/grpc"
}
}

`,
wantErr: "Failed while making call: code:unknown message:negative input\n",
},
{
Expand All @@ -1205,6 +1212,13 @@ func TestGRPCReflectionSource(t *testing.T) {
Peers: []string{addr.String()},
},
},
wantRes: `{
"headers": {
"content-type": "application/grpc"
}
}

`,
wantErr: `Failed while making call: code:invalid-argument message:invalid username
{
"details": [
Expand All @@ -1222,8 +1236,8 @@ func TestGRPCReflectionSource(t *testing.T) {
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
gotOut, gotErr := runTestWithOpts(tt.opts)
assert.Equal(t, gotErr, tt.wantErr)
assert.Equal(t, gotOut, tt.wantRes)
assert.Equal(t, tt.wantErr, gotErr, "bad error")
assert.Equal(t, tt.wantRes, gotOut, "bad response")
})
}
}
Expand Down
29 changes: 18 additions & 11 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if len(outSerialized) == 0 {
return
}
bs, err = json.MarshalIndent(outSerialized, "", " ")
if err != nil {
out.Fatalf("Failed to convert map to JSON: %v\nMap: %+v\n", err, outSerialized)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@rabbbit rabbbit Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
out.Printf("%s\n\n", bs)
}()

if err != nil {
buffer := bytes.NewBufferString(err.Error())

Expand Down Expand Up @@ -502,20 +519,10 @@ func makeInitialRequest(out output, transport transport.Transport, serializer en
}

// Print the initial output body.
outSerialized := map[string]interface{}{
"body": responseMap,
}
if len(response.Headers) > 0 {
outSerialized["headers"] = response.Headers
}
outSerialized["body"] = responseMap
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, v := range response.TransportFields {
outSerialized[k] = v
}
bs, err := json.MarshalIndent(outSerialized, "", " ")
if err != nil {
out.Fatalf("Failed to convert map to JSON: %v\nMap: %+v\n", err, responseMap)
}
out.Printf("%s\n\n", bs)
}

// isYabTemplate is currently very conservative, it requires a file that exists
Expand Down
13 changes: 9 additions & 4 deletions transport/grpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,16 @@ func (t *grpcTransport) Call(ctx context.Context, request *Request) (*Response,

ctx, cancel := requestContextWithTimeout(ctx, request)
defer cancel()

var errs error

transportResponse, err := t.Outbound.Call(ctx, t.requestToYARPCRequest(request))
if err != nil {
return nil, err
}
return yarpcResponseToResponse(transportResponse)
errs = multierr.Append(errs, err)

r, err := yarpcResponseToResponse(transportResponse)
errs = multierr.Append(errs, err)

return r, errs
Comment on lines +135 to +140
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func (t *grpcTransport) CallStream(ctx context.Context, request *StreamRequest) (*transport.ClientStream, error) {
Expand Down