newrelic / newrelic-java-agent

The New Relic Java agent
Apache License 2.0
202 stars 143 forks source link

expected_status_codes is not honored when there is an exception in the transaction #376

Closed meiao closed 2 years ago

meiao commented 3 years ago

Description

Setting a expected_status_code will not mark an error as expected when there is an exception in the transaction.

Expected Behavior

If a transaction returns with a status code that is in the expected_status_code an error from this transaction should be marked as expected regardless of the existence of an exception.

This is how ignored_status_code works.

meiao commented 3 years ago

This is likely due to the difference between ThrowableError and HttpTracedError.

meiao commented 3 years ago

The repository below has a simple app to reproduce the problem. https://github.com/meiao/repro-nr-376

sdmcintyre commented 3 years ago

Any idea when this might get looked at? This causes many false alarms in our monitoring due to things like input validation errors (HTTP bad request).

jasonjkeller commented 3 years ago

If an exception is thrown that has an associated HTTP error response code then simply configuring the error response code as expected (expected_status_code) is not sufficient, you have to configure the exception class as expected (expected_classes).


This works for the repro app that was previously posted:

  error_collector:
    # force agent to override default of ignoring 404s
    ignore_status_codes: 0
    expected_classes:
      -
        class_name: "org.springframework.web.server.ResponseStatusException"
        message: "404 NOT_FOUND"

If the controller is modified in the repro app so that it only returns an error response code instead of throwing an exception:

    @RequestMapping("/")
    public ResponseEntity<String> fourOFour() {
//        throw new ResponseStatusException(HttpStatus.NOT_FOUND);
        return new ResponseEntity<>(HttpStatus.NOT_FOUND);
    }

then the 404 HTTP response code is recorded as error.expected=true with just the following config:

  error_collector:
    # force agent to override default of ignoring 404s
    ignore_status_codes: 0
    expected_status_codes: 404
jasonjkeller commented 3 years ago

Some additional details from a deep dive: https://newrelic.atlassian.net/browse/GTSE-10856?focusedCommentId=980682

tbradellis commented 2 years ago

I think the problem starts here with markedExpected: https://github.com/newrelic/newrelic-java-agent/blob/bf6cb39760c3d5e66d8614cd698a95bcb381c347/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java#L441

We only check whether it's expected on the Throwable. As @meiao hinted on ThrowableError and HttpTracedError. When we get to HttpTracedError we don't consult the status code to see if we need to set markedExpected differently based on status. I have a local change and will run some tests with a change for this. Essentially, I think we should just check if the status code is expected and reset markedExpected here: https://github.com/newrelic/newrelic-java-agent/blob/bf6cb39760c3d5e66d8614cd698a95bcb381c347/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java#L467

tbradellis commented 2 years ago

Correction...we also need to check this for ThrowableError.

tbradellis commented 2 years ago

https://github.com/newrelic/newrelic-java-agent/blob/53dc9bbd1d96875311fa00c0e016e4a74e59d06b/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java#L459

sdmcintyre commented 2 years ago

Do we need to update our New Relic Agent to apply the fix?

meiao commented 2 years ago

Yes. Agent 7.5.0 has the fix.