Include stack trace in errors

[gaelollivier]

Hey,

Currently, if a runtime error occurs during the execution of a query, the only information available looks like this:

{
  "data": null,
  "errors": [
    {
      "message": "runtime error: invalid memory address or nil pointer dereference",
      "locations": []
    }
  ]
}

Which is quite tricky to debug.

This PR adds a "StackTrace" field to the FormattedError object. This field is not exposed to the JSON version, so it doesn't expose possibly sensible debug information but can be used for logs.
Currently the field is only populated in one place but I guess it makes sense to populate it wherever there is a recover() call.
Because the call to debug.Stack() may be expensive, maybe we should add an option to enable/disable it ? Not sure how we should expose this.

Do you think this feature make sense ?

[coveralls]

Coverage increased (+0.004%) to 81.037% when pulling ba52787 on gaelduplessix:include-trace-in-errors into 1c504cf on graphql-go:master.

[coveralls]

Coverage increased (+0.03%) to 81.043% when pulling 6748b1e on gaelduplessix:include-trace-in-errors into 1e23489 on graphql-go:master.

[dvic]

I just went with the approach of letting it implement the Causer interface: https://github.com/qdentity/graphql-go/blob/master/gqlerrors/formatted.go

In this way, you can extract your own errors thrown in the resolvers and render them appropriately.

[gaelollivier]

@dvic Indeed, sounds more generic.

Still, I think the trace should be included anyway. In my case, I had a runtime exception happening inside the lib (this is probably a bug btw, if performing a mutation query when none is defined in the schema)

So to include the stack trace, we would build a custom error type that returns the trace in the Cause() method ? Or the user could retrieve the underlying error type and call Trace() on it directly ?

[dvic]

@gaelduplessix I modified that code to use github.com/pkg/errors instead of the standard library errors package. The package github.com/pkg/errors stores the stack trace for you (you can see it when you print the error with %v or %+v).

[dvic]

By the way, https://github.com/qdentity/graphql-go also contains changes w.r.t. how resolvers are called. I took the changes from #213 and added the concurrent resolving of list values #132. This is a perfect match with https://github.com/nicksrandall/dataloader. There were some data races but I managed to fix these as well, all tests are passing with the -race flag included. If anybody is interested in this I can open a PR.

[gaelollivier]

@dvic Oh, sounds neat :) Well, the error improvement seems quite an isolated issue. Would it make sense to extract it to another PR ?

[dvic]

:) Those fixes are only useful/needed if #213 or #132 are merged, however, it is not clear at the moment which path will be chosen. So it's best to wait until a decision has been made before merging those fixes into the appropriate branch.

I did however submit a PR (#278) just now that makes sure the tests check for race conditions, so the issue is caught as soon as concurrency is added to the project.

[gaelollivier]

Well, improvements on errors are useful on their own, wouldn't it make sense to have these changes isolated from the rest (adding concurrency to the resolver) so they can be merged sooner ?

Is there any ETA on when #213 or #132 are going to be merged ? (I'm quite new to this project so I don't really have any context on how the development is going)

[dvic]

I have no idea on the ETA of either #213 or #132, but I'll cook up a PR for the changes w.r.t. the errors (it's basically this commit).

[dvic]

Created PR #279 for the changes of embedding errors in FormattedError.

[dvic]

@gaelduplessix did you have time to check out #279? Will it work for your use case?