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.

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

feat(apollo-engine-reporting) Introduce rewriteError to munge errors for reporting. #2618

Merged
merged 21 commits into from
Apr 30, 2019

Conversation

abernix
Copy link
Member

@abernix abernix commented Apr 26, 2019

This PR supersedes @lpgera's #1639 by building upon the idea and foundation which that PR started.

Not to be confused with the top-level formatError function — which re-shapes the error response that is sent to the client — this newly-introduced rewriteError handler can be defined within the engine settings and used to modify or discard reporting errors to Apollo Engine.

It will receive the err as the first argument and can modify the message property of that error as it sees fit, including returning a new GraphQLError entirely, if desired.

To avoid reporting a particular error, an explicit null may be returned, rather than the error. This will cause the error to never be sent to Apollo Engine and is helpful for lower-severity errors caused by users like apollo-server-errors's AuthenticationErrors which may not make sense to flag as problematic within Apollo Engine.

Note: To avoid inadvertent mistakes, either an error that is an instanceof GraphQLError or an explicit null must be returned. Returning undefined is not permitted.

Avoid reporting lower-severity apollo-server-errors errors.

For example, if the current server is throwing the AuthenticationError provided by apollo-server-errors when a mis-typed password is supplied, an implementor might avoid reporting this to Apollo Engine (and potentially burying higher-severity errors) by defining this as:

const { AuthenticationError } = require("apollo-server-errors");
const server = new ApolloServer({
  typeDefs,  // (Not defined in this example)
  resolvers, //    "      "      "     "
  engine: {
    rewriteError(err) {
      // Return `null` to avoid reporting `AuthenticationError`s
      if (err instanceof AuthenticationError) {
        return null;
      }

      // All other errors will be reported.
      return err;
    }
  },
});

Avoid reporting an error based on other attributes.

const server = new ApolloServer({
  typeDefs,  // (Not defined in this example)
  resolvers, //    "      "      "     "
  engine: {
    rewriteError(err) {
      // Generally, using a more stable property, like `code`, would be
      // preferable to checking the `message` property, but it can be done!
      if (err.message && err.message.match(/^Known error message/) {
        return null;
      }

      // All other errors should still be reported!
      return err;
    }
  },
});

Redacting the error message.

If it is necessary to change the error prior to reporting it to Apollo Engine – for example, if there is personally identifiable information in the error message — the rewriteError function can also help:

const server = new ApolloServer({
  typeDefs,  // (Not defined in this example)
  resolvers, //    "      "      "     "
  engine: {
    rewriteError(err) {
      // Make sure that a specific pattern is removed from all error messages.
      err.message = err.message.replace(/x-api-key:[A-Z0-9-]+/, "REDACTED");
      return err;
    }
  },
});

TODO

lpgera and others added 15 commits April 8, 2019 23:22
...though it's currently not functional as of this commit.
Even though `maskErrorDetails` is going away, it seems important to continue
to test it along with the new functionality introduced by `filterErrors`.

Note: These tests highlight some failures in the current `filterErrors` logic
which need to be addressed in follow-up commits.
The intention is to rename the `filterErrors` function to `rewriteError` in
a follow-up commit, but this maintains the previous name during the
re-implementation phase.
The new implementation of `rewriteError` (previously `formatErrors` and
prior to that `maskErrorDetails`, do nothing to ensure that the `stack`
property (i.e. `Error.prototype.stack`) is regenerated using the new
combination of `${err.name}: ${err.message`, suffixed with the rest of
`Error.captureStackTrace`.  That's okay, but we should at least guard
against that and make sure that no future code starts to add the `stack`
property, without properly redacting it within `rewriteError` internals.

That redaction is _slightly_ less than performant (string manipulation), so
it didn't seem worth the effort, since we don't actually send it anywhere.
While the new `filterErrors` functionality did allow complete filtering of
errors prior to reporting them to Apollo Engine, it also provides the
ability to change the error entirely.  This more thorough functionality
deserves a name which more accurately defines its functionality.

The use-defined `rewriteError` function (defined within the `engine`
configuration on the `ApolloServer` constructor options) can still be used
to "filter" an error from going to Engine by re-writing the error to `null`
To accomplish this nullification of an error, an explicit `null` can be
returned from the `rewriteError` function rather than the normal practice of
returning a `GraphQLError` (in a presumably modified form).
Because prettier doesn't do it!
- Deprecate `formatError` in docs in favor of `rewriteError`
- lint-fix
This is no longer necessary after the changes in @jbaxleyiii's
#2574 (and
@martijnwalraven's
#2416), which changed the
error reporting to be captured via the newly introduced
`didEncounterErrors`, rather than in `willResolveField` where it used to
happen.  Yay!
I don't think this is providing much value at this point and it's been
skipped for over a year.  It appears that the test was intended to test
`reportErrorFunction`, though the description claims that it's trying to
test something else entirely.

Regardless, the testing of traces is actually handled elsewhere now, so this
is good to remove.
@abernix abernix mentioned this pull request Apr 28, 2019
4 tasks
abernix added 3 commits April 28, 2019 21:19
This partially reverts commit 21b8b86,
which inadvertently changed `formatError`, which is a top-level function
which can be used to adjust the response to the client, which is
intrinsically different than the new `rewriteError` which modifies the
shape of the error in the tracing data which is sent to the Apollo Platform.
@abernix abernix force-pushed the abernix/finish-pr-1639 branch from cd96cc9 to e99a99a Compare April 28, 2019 18:19
michael-watson and others added 3 commits April 30, 2019 11:04
* (docs) Add information on `rewriteError` to the `errors.md`.

This adds additional clarity fro the new functionality provided in #1639.

* Fix syntax error and use `String.prototype.startsWith` rather `St.pr.match`.

Regular expressions are just not as approachable!

* Update errors example.

The previous example here wasn't a wonderful example since it was suggesting a pattern which is unnecessary when `NODE_ENV=production`, where stack traces are automatically hidden.

* Update errors.md

* Update errors.md
@abernix abernix merged commit 6f2b693 into release-2.5.0 Apr 30, 2019
@abernix abernix deleted the abernix/finish-pr-1639 branch April 30, 2019 10:19
@abernix abernix added this to the Release 2.5.0 milestone Apr 30, 2019
@abernix abernix mentioned this pull request Apr 30, 2019
1 task
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants