newrelic / newrelic-node-apollo-server-plugin

Apache License 2.0
62 stars 29 forks source link

How to skip reporting certain status codes as errors #142

Closed luads closed 1 year ago

luads commented 2 years ago

Is your feature request related to a problem? Please describe.

Our applications have been reporting any 401, 403, 404 errors to New Relic. This is creating unwanted noise as those shouldn't be reported and just returned back to the user.

I couldn't find a way of configuring Apollo so that those errors wouldn't be picked up by the NR plugin. No luck using NestJS's exception filters either.

I'm not sure if the plugin is the right place for that, so I'm not opening a PR until I get some feedback.

Feature Description

The plugin would be able to receive an array of status codes or exception types to be ignored, example:

    GraphQLModule.forRoot({
      plugins: [
        newRelicApolloPlugin.default({
          ignoredStatusCodes: [
            HttpStatus.BAD_REQUEST,
            HttpStatus.UNAUTHORIZED,
            HttpStatus.FORBIDDEN,
            HttpStatus.NOT_FOUND,
          ],
        }),
      ],
      sortSchema: true,
    }),

Describe Alternatives

Additional context

I've put together a proof of concept of how it'd look like if this was implemented in the plugin: https://github.com/newrelic/newrelic-node-apollo-server-plugin/compare/main...luads:ignore-specific-status-codes?expand=1

michaelgoin commented 2 years ago

Hi @ luads,

This is actually something you'll want to configure at the agent level. The plugin reports errors through the standard agent mechanisms.

You can ignore status codes by setting configuration in your newrelic.js configuration file. https://docs.newrelic.com/docs/agents/nodejs-agent/installation-configuration/nodejs-agent-configuration/#error_ignore

error_collector: {
  ignore_status_codes: [401, 403, 404] // intended codes here.
}

Or via ENV var: NEW_RELIC_ERROR_COLLECTOR_IGNORE_ERROR_CODES="401,404,502"

I would expect 404 to already be filtered out as that is the default. The exception being self-reported errors through newrelic.noticeError(). So I am surprised you are seeing those.

luads commented 2 years ago

Hey @michaelgoin, thanks for the reply.

We're actually defining that as an env var already:

NEW_RELIC_ERROR_COLLECTOR_IGNORE_ERROR_CODES='400,401,403,404'

The odd part is that the HTTP code returned by NestJS is actually a 200, so it gets through regardless. Pretty much all errors are returning a 200, except for schema validation errors that will return a 400.

Perhaps this is just a matter of ensuring Nest/Apollo are returning the proper HTTP codes? I'll do some more research into it.

michaelgoin commented 2 years ago

@luads

Ah, yeah that is a funky thing with Apollo GraphQL, everything is returned as a 200. Great success, you have errors. :) I didn't realize (or forgot) it differentiates on the individual errors / has a status code on those still.

As a workaround, you can ignore based on error type or type+message combinations.

But to solve the problem I guess this warrants further consideration. Do we want to introduce plugin specific configuration for this or leverage the existing agent configuration? I tend to lean towards existing but... Will it be confusing to some users that they are getting 200 status codes but we are filtering out errors based on different status codes in the same request?

I'll discuss with the team.

lewisgj commented 2 years ago

I wanted to raise a similar issue I've been having - happy to move it to a separate one if you don't think it's related.

It is convention in GraphQL implementations like Apollo that we throw exceptions to return errors back to the client - such as Apollo's UserInputError. We personally use these a bit like we'd use a 400 or a 422 status code in a REST API - the request passes basic validation but it doesn't make sense logically/semantically.

We'd love to be able to mark those kinds of errors (or perhaps a custom error) as "expected" in New Relic terms - or ignore them completely as the original issue raiser suggested.

I don't think ignoring status codes is what I'd personally need - since there isn't always a backend that is throwing them in our case, but maybe this would also be suitable for that.

workato-integration[bot] commented 1 year ago

https://issues.newrelic.com/browse/NEWRELIC-4461

VRSNR commented 1 year ago

Why does error ignores or mark as expected not working for your use case? What is unique about this use that that this is not working. Please check this documentation.

luads commented 1 year ago

It's been one year since I posted this. I'm not working on this problem anymore, so I won't be able to provide more details. Feel free to close the issue if you don't deem it relevant.

workato-integration[bot] commented 1 year ago

This issue won't be actioned.

jawang35 commented 10 months ago

Any chance we can reopen this? We're having the same problem. There's no way to ignore certain error status codes because Apollo Server always returns a 200.

jawang35 commented 9 months ago

Was able to solve this by using error_collector.ignore_classes.