-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Request: Provide context on global error handlers #1343
Comments
@thanpolas What do you mean by global error handlers here? |
The options from the GraphQL.... variants for each service (express, koa, etc) have a key named That callback function, should also receive the
|
@thanpolas That's interesting! What would you like to do with the context in Another option is to construct an Apollo Error with the values from the context that you need. https://www.apollographql.com/docs/apollo-server/v2/features/errors.html#Other-errors |
My use case would be quite specific but I imagine there are plenty of uses one can have with the context available at the global error handler. We are using knex in our project, instead of an ORM, and want to simplify and make more robust the operation of doing SQL Transactions. The way it's done now is that you'd request a transaction object from knex, perform your SQL operations very carefully and all in one place so as to have absolute control on any of them failing so you can rollback and not have open connections (due to the open transaction) ghosting on you. This method presents a few challenges primarily because we have to concentrate all DB operations in one place. If we have the context available on the global error handler, we can assign the transaction object on the context and on the global handler roll it back if it's there. This will capture all exception cases including the ones that were not captured or controlled & generated by the codebase.... Which is the reason why assigning the transaction object on the thrown Error object of a controlled exception is not a good solution for us as it doesn't account for the exception that will get thrown outside our control or our try/catch statements. tl;dr:
|
One more point is that we'll more easily debug untraced errors. Meaning, we can flag all the errors we throw in the codebase so as to indicate they originate from a known occasion. Then on the global error handler filter for the errors that don't have that flag, which means those are errors that were thrown from unexpected places... if context is present in those cases, it would greatly help the debugging efforts as the error would log along with all the context of the |
This would be really useful for us too ! As we are injecting an instance of i18n in the context and using it to translate error messages. |
@thanpolas we do transactions too but they have to be encapsulated around execution of resolvers. I would not recommend doing rollback or transactions on formatError (which purpose is to format after the result is ready) However if you need to access the context there is also [formatResponse] that includes all server options including the context. |
yes, that is correct @ak99372, transactions should be handled within the closure that provides them in knex to benefit from automatic commit/rollback. Do you know if |
I am having the same need. In my case I want to log the error, an report it to bugsnag. However, since in the logger we print request ids that are unique per request, we inject the logger in the context. Since |
Similar to @rewop, we used this option to log the error, including the request id, query variables, etc. With Apollo 1, we were able to get access to the request params because the options were inside the scope of the function with it. Alas, once it was moved into the |
This would be super handy for my use case. I want to log the request id alongside errors like so: {
formatError(err, ctx) {
const id = ctx.requestId
logger.error({ err, id })
return { message, id, locations, path };
}
} Currently, the context is only available in resolvers. This means devs have to ensure the request id is capture at error creation time. It's easy to miss that... const someFieldResolver = async (_, __, ctx) => {
try {
await doSomething();
} catch (err) {
// This can be tedious and easy to forgot
throw new ServiceError({ message: err.message, id: ctx.requestId })
}
} |
Would be great having access to |
@kennethaasan you can attach |
I would love to have this feature because we attach some AWS X-Ray logging utilities on the context object that we use to capture the error with other data from the context. I could see this being implemented as an extension but after looking through the code I didn't see that the context was passed to any of the methods. Personally I think this makes more sense. @evans, would you be interested in a PR for something like this? If so would you prefer the functionality happen within |
* Pass the context along to all the extension methods Addresses #1343 With this change you should now be able to implement an extension like so: ```javascript class MyErrorTrackingExtension extends GraphQLExtension { willSendResponse(o) { const { context, graphqlResponse } = o context.trackErrors(graphqlResponse.errors) return o } } ``` Edit by @evans : fixes #1343 fixes #614 as the request object can be taken from context or from requestDidStart fixes #631 "" * Remove context from extra extension functions The context shouldn't be necessary for format, parse, validation, and execute. Format generally outputs the state of the extension. Parse and validate don't depend on the context. Execution includes the context argument as contextValue * Move change entry under vNext
so any way to do so. i checked extension PR but still do not know how to apply |
* Pass the context along to all the extension methods Addresses #1343 With this change you should now be able to implement an extension like so: ```javascript class MyErrorTrackingExtension extends GraphQLExtension { willSendResponse(o) { const { context, graphqlResponse } = o context.trackErrors(graphqlResponse.errors) return o } } ``` Edit by @evans : fixes #1343 fixes #614 as the request object can be taken from context or from requestDidStart fixes #631 "" * Remove context from extra extension functions The context shouldn't be necessary for format, parse, validation, and execute. Format generally outputs the state of the extension. Parse and validate don't depend on the context. Execution includes the context argument as contextValue * Move change entry under vNext
It would be very beneficial to have the request context on the global error handlers so we can have better insights as to who caused an error and have a better chance at debugging it.
The text was updated successfully, but these errors were encountered: