Add option to filter errors from being reported #1639

lpgera · 2018-09-10T12:21:21Z

Add errorFilter option to make it possible to filter certain errors from being reported to the Engine service. (For user errors, like invalid password, etc...)

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

apollo-cla · 2018-09-10T12:21:23Z

@lpgera: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

martijnwalraven · 2018-09-11T02:10:16Z

Thanks for opening this PR! I think the ability to filter reported errors is definitely useful, but it seems allowing maskErrorDetails to take either a boolean or a function (error: Error) => boolean would work well here, as opposed to introducing an additional option. What do you think?

lpgera · 2018-09-11T14:50:35Z

The problem with that approach is that the maskErrorDetails still reports the error to the Engine, it just masks it's content.
In our use case we would like to be able to filter some errors from being reported at all. Maybe we can merge the two options into one by creating something like mapErrors: (error: Error) => Error | null. This could be used to implement the masking of the error and for filtering too, by returning null from it.
What are your thoughts about that?

Edit:
Thinking about it a bit more, renaming the maskErrorDetails would be a breaking change, so I think the additional option is needed for this separate functionality.

martijnwalraven · 2018-09-12T22:59:28Z

@lpgera I like that suggestion. But maybe we could call it filterErrors instead?

Since maskErrorDetails was only merged a week ago, I don't think a breaking change is that much of an issue. But to ease the transition, we could just take it out of the docs and make it work on top of filterErrors for now.

lpgera · 2018-09-13T13:52:59Z

I updated the option to have a return type of GraphQLError | null, but I could not figure out how to implement the masking with it. I can not modify the properties of the received GraphQLError, and creating a new one captures a stack trace from the point of creation.
Is maskErrorDetails really needed? (It was added after a misunderstanding of our feature request.) And if so, can you help me out with the problem above?

zionts · 2018-09-20T15:42:47Z

docs/source/api/apollo-server.md

@@ -345,6 +345,9 @@ addMockFunctionsToSchema({
   'sendReport()' on other signals if you'd like. Note that 'sendReport()'
   does not run synchronously so it cannot work usefully in an 'exit' handler.

-*  `maskErrorDetails`: boolean
+* `filterErrors`: (err: Error) => Error | null


I agree with @martijnwalraven that it would be nice if this type could also be a "boolean" so that we could introduce a sane default for masking error details such as removing all string literals other than the approved ApolloError code.

abernix · 2018-09-21T17:39:42Z

@lpgera

I updated the option to have a return type of GraphQLError | null, but I could not figure out how to implement the masking with it.

Are you suggesting that you were unable to modify the message property of the error before returning it?

e.g.

  filterErrors: (err) => {
    err.message = “Redacted”;
    return err;
  }

?

lpgera · 2018-10-02T11:45:04Z

@abernix To be exact, I could only modify the message property and nothing else, because the type of err has to be GraphQLError, and that class defines every property except the message as readonly.

luisalduucin · 2019-01-10T14:35:45Z

Definitely a useful feature ! Hope it gets merged soon !

daliadefelipee

It's already a new year and this is a great feature, is there any update?

angelmartinez-wz · 2019-01-10T17:16:24Z

This PR is going to be merged soon? An error filter would be useful to avoid a lot of false positive.

abernix · 2019-01-15T18:39:21Z

I'm looking at moving this forward soon. Just as a heads up, I've branched off this PR and introduced a few additional commits which built upon the work by @lpgera above. The additional commits introduce new tests for this new functionality, resolves the previous failing tests and renames it (again!) to rewriteError, which I suspect more accurately describes the final implementation.

There's still an edge case bug which seems to crop up in the apollo-engine-reporting module when the error is mutated and differs from that of what was delivered in the response that still needs to be figured out, but hopefully nothing too difficult.

That work currently lives here https://github.com/apollographql/apollo-server/compare/abernix/finish-pr-1639, and will hopefully surface soon. I'll merge that branch into this PR before we finalize.

codecov-io · 2019-02-06T14:34:35Z

Codecov Report

Merging #1639 into master will decrease coverage by 16.13%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           master    #1639       +/-   ##
===========================================
- Coverage   90.15%   74.01%   -16.14%     
===========================================
  Files           5       29       +24     
  Lines         193     1170      +977     
  Branches       39      297      +258     
===========================================
+ Hits          174      866      +692     
- Misses         18      293      +275     
- Partials        1       11       +10

Impacted Files	Coverage Δ
packages/apollo-engine-reporting/src/extension.ts	`61.32% <0%> (ø)`
packages/apollo-server-koa/src/ApolloServer.ts	`90.76% <0%> (-0.66%)`	⬇️
packages/apollo-server-express/src/ApolloServer.ts	`90% <0%> (-0.63%)`	⬇️
...ackages/apollo-server-express/src/expressApollo.ts	`86.36% <0%> (ø)`	⬆️
packages/apollo-server-express/src/index.ts	`100% <0%> (ø)`	⬆️
...kages/apollo-datasource-rest/src/RESTDataSource.ts	`86.41% <0%> (ø)`
...s/apollo-server-cloud-function/src/ApolloServer.ts	`43.63% <0%> (ø)`
...ackages/apollo-server-cache-memcached/src/index.ts	`100% <0%> (ø)`
packages/apollo-server-core/src/graphqlOptions.ts	`75% <0%> (ø)`
packages/apollo-server-env/src/polyfills/url.js	`100% <0%> (ø)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b1f6dd...5798424. Read the comment docs.

abernix · 2019-04-26T14:07:53Z

@lpgera Sorry for the long delay here!

I've opened #2618 to supersede this and kept all your original commits to maintain your contributorship! Input appreciated over there, but as soon as I finish the docs (today or Monday, hopefully!), I intend on shipping this with Apollo Server 2.5.0.

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

* (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

@jbaxleyiii

…s for reporting. (#2618) * Add errorFilter option * update changelog * Rename errorFilter to filterErrors, return GraphQLError or null * Add back `maskErrorDetails` type to maintain support. ...though it's currently not functional as of this commit. * Introduce additional tests for `filterErrors` and `maskErrorDetails`. 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. * Finish up new `filterErrors` functionality, sans renaming to a new name. 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. * Add a test that ensures that the `stack` is not transmitted. 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. * Rename new `filterErrors` function to a more accurate `rewriteError`. 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). * Adjust the line lengths of some comments. Because prettier doesn't do it! * Add CHANGELOG.md for `rewriteError`. * Update deprecated to use rewriteError * Update rewriteError in docs - Deprecate `formatError` in docs in favor of `rewriteError` - lint-fix * Remove now-unnecessary guard which checks for `errors`. 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! * Remove long-skipped test that was never close to working. 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. * Fix test structure after merge. * Partially Revert "Update rewriteError in docs" 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. * Update the API section with more clarity. * Update docs for rewriteError. (#2585) * (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 * Update CHANGELOG.md Co-authored-by: GerA <[email protected]>

ghost added the ⛲️ feature New addition or enhancement to existing solutions label Sep 10, 2018

daniman requested review from martijnwalraven and evans September 11, 2018 02:06

zionts reviewed Sep 20, 2018

View reviewed changes

abernix self-assigned this Sep 20, 2018

lpgera added 4 commits October 2, 2018 13:57

Add errorFilter option

28cf18e

update changelog

3a25208

Rename errorFilter to filterErrors, return GraphQLError or null

42171d4

allow filterErrors option to accept true

5798424

apollographql deleted a comment from codecov-io Nov 22, 2018

daliadefelipee reviewed Jan 10, 2019

View reviewed changes

lpgera requested a review from abernix as a code owner February 6, 2019 14:34

abernix mentioned this pull request Apr 9, 2019

tests: Expand helper for trace tests. #2554

Merged

abernix mentioned this pull request Apr 26, 2019

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

Merged

1 task

abernix closed this Apr 26, 2019

abernix added a commit that referenced this pull request Apr 28, 2019

(docs) Add information on rewriteError to the errors.md.

1e0ed07

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

abernix added a commit that referenced this pull request Apr 28, 2019

(docs) Add information on rewriteError to the errors.md.

b7bd0a3

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

github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to filter errors from being reported #1639

Add option to filter errors from being reported #1639

lpgera commented Sep 10, 2018 •

edited

Loading

apollo-cla commented Sep 10, 2018

martijnwalraven commented Sep 11, 2018

lpgera commented Sep 11, 2018 •

edited

Loading

martijnwalraven commented Sep 12, 2018

lpgera commented Sep 13, 2018

zionts Sep 20, 2018

abernix commented Sep 21, 2018

lpgera commented Oct 2, 2018

luisalduucin commented Jan 10, 2019

daliadefelipee left a comment

angelmartinez-wz commented Jan 10, 2019

abernix commented Jan 15, 2019 •

edited

Loading

codecov-io commented Feb 6, 2019

abernix commented Apr 26, 2019

Add option to filter errors from being reported #1639

Add option to filter errors from being reported #1639

Conversation

lpgera commented Sep 10, 2018 • edited Loading

apollo-cla commented Sep 10, 2018

martijnwalraven commented Sep 11, 2018

lpgera commented Sep 11, 2018 • edited Loading

martijnwalraven commented Sep 12, 2018

lpgera commented Sep 13, 2018

zionts Sep 20, 2018

Choose a reason for hiding this comment

abernix commented Sep 21, 2018

lpgera commented Oct 2, 2018

luisalduucin commented Jan 10, 2019

daliadefelipee left a comment

Choose a reason for hiding this comment

angelmartinez-wz commented Jan 10, 2019

abernix commented Jan 15, 2019 • edited Loading

codecov-io commented Feb 6, 2019

Codecov Report

abernix commented Apr 26, 2019

lpgera commented Sep 10, 2018 •

edited

Loading

lpgera commented Sep 11, 2018 •

edited

Loading

abernix commented Jan 15, 2019 •

edited

Loading