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

[meiao]

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]

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

[meiao]

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

[sdmcintyre]

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]

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]

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

[tbradellis]

I think the problem starts here with markedExpected:

newrelic-java-agent/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java

Line 441 in bf6cb39

boolean markedExpected = td.getThrowable() == null ? false : td.getThrowable().expected;

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:

newrelic-java-agent/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java

Line 467 in bf6cb39

error = HttpTracedError

[tbradellis]

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

[tbradellis]

newrelic-java-agent/newrelic-agent/src/main/java/com/newrelic/agent/errors/ErrorServiceImpl.java

Line 459 in 53dc9bb

boolean responseStatusExpected = errorCollectorConfig.getExpectedStatusCodes().contains(responseStatus);

[sdmcintyre]

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

[meiao]

Yes. Agent 7.5.0 has the fix.