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.

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

make a pass at the changes for implementing issue-781 integrating pkg/errors #792

Merged
merged 3 commits into from Sep 27, 2016

Conversation

Projects
None yet
2 participants
@maleck13
Copy link
Contributor

maleck13 commented Sep 26, 2016

@raphael This is a pass at the changes I proposed for issue-781 #781

One concern I have here is backwards compatibility. If users are relying on type checks for errors they will stop working.
For example if they were relying on checks like

if err, ok := err.(goa.ServiceError); ok {
}

From a usability point of view there is no difference however. A user of goa can still do something like

return goa.ErrInternal("something strange")

and it will work as expected except now you will see a trace in the logs

@maleck13 maleck13 changed the title make a pass at the changes make a pass at the changes for implementing issue-781 integrating pkg/errors Sep 26, 2016

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 26, 2016

Thank you the PR looks great! I was thinking we would let the user call errors.Wrap themselves. This way they can provide a meaningful message and only wrap errors they care to wrap. This also helps with the backwards compatibility issue (since the type test is actually documented in the Error Handling docs). There could be a comment in the middleware header explaining that it will take care of logging the callstack if the error is wrapped.

@maleck13

This comment has been minimized.

Copy link
Contributor

maleck13 commented Sep 26, 2016

@raphael Thanks for feedback. I will look at updating based on your comment

@maleck13 maleck13 force-pushed the maleck13:issue-781 branch from 272d7b2 to 5e87f09 Sep 26, 2016

@maleck13

This comment has been minimized.

Copy link
Contributor

maleck13 commented Sep 26, 2016

@raphael Made the changes that I think address your comment. I added a test based on the other error_handler tests and on some of the logging tests. I am not particularly familiar with the test framework so not sure I have implemented the test as would be expected. Happy for any feedback

@@ -38,7 +40,7 @@ func ErrorHandler(service *goa.Service, verbose bool) goa.Middleware {
reqID = shortID()
ctx = context.WithValue(ctx, reqIDKey, reqID)
}
goa.LogError(ctx, "uncaught error", "id", reqID, "msg", respBody)
goa.LogError(ctx, fmt.Sprintf("uncaught error : %+v", e), "id", reqID, "msg", respBody)

This comment has been minimized.

@raphael

raphael Sep 27, 2016

Member

Is there a reason for not using a key/value pair to log the error? something like goa.LogError(ctx, "uncaught error", "err", e, "id", reqID, "msg", respBody)

This comment has been minimized.

@maleck13

maleck13 Sep 27, 2016

Contributor

This seems reasonable, I think it is just a bit of unfamiliarity. Will update PR to be

goa.LogError(ctx, "uncaught error", "err", fmt.Sprintf("%+v"e), "id", reqID, "msg", respBody)

We need the %+v to get the stack trace to print

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 27, 2016

Thank you! the changes look good. I left one comment related to how the logging of uncaught error is done but other than that LGTM.

@maleck13

This comment has been minimized.

Copy link
Contributor

maleck13 commented Sep 27, 2016

@raphael do you normally squash commits before merging or are these separate commits ok?

@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 27, 2016

Yes I'll squash the commits. Is this ready for merge?

@maleck13

This comment has been minimized.

Copy link
Contributor

maleck13 commented Sep 27, 2016

👍

@raphael raphael merged commit 5336ae6 into goadesign:master Sep 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@raphael

This comment has been minimized.

Copy link
Member

raphael commented Sep 27, 2016

Thank you! do you think you could create a PR against the v1 branch as well?

@maleck13

This comment has been minimized.

Copy link
Contributor

maleck13 commented Sep 27, 2016

@raphael absolutely happy to

raphael added a commit that referenced this pull request Sep 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment