Errors that are expected by way of expected_status_codes should not impact Apdex

[fallwith]

We have reports that errors with status codes matching specified expected_status_codes values are correctly appearing in the Errors Inbox as "expected", but are negatively impacting Apdex.

This relevant specification text confirms that expected errors should not impact Apdex.

Expected errors MUST create traced errors and error events, but MUST NOT increment any of the Errors/* metrics in order to prevent error rate and Apdex modifications.

We'll need to determine where in the agent or back-end the denotation of an error as expected fails to prevent that error from impacting Apdex.

NOTE: The use of ignore_status_codes works as a workaround. Ignored errors are seen as not impacting Apdex. The drawback of the workaround is that ignored errors are not delivered to the Errors Inbox.

[workato-integration]

https://new-relic.atlassian.net/browse/NR-262162

[florianpilz]

Thank you so much for taking the time and digging into the problem. :) While the linked PR #2619 would be good enough for what we need, as far as I can see this would still let expected_classes and expected_messages impact the Apdex score -- right? As the PR has added a check specifically for expected_status_codes but not the other "expected" configuration options. See https://github.com/newrelic/newrelic-ruby-agent/blob/dev/newrelic.yml#L309-L322

[fallwith]

Thank you so much for taking the time and digging into the problem. :) While the linked PR #2619 would be good enough for what we need, as far as I can see this would still let expected_classes and expected_messages impact the Apdex score -- right? As the PR has added a check specifically for expected_status_codes but not the other "expected" configuration options. See https://github.com/newrelic/newrelic-ruby-agent/blob/dev/newrelic.yml#L309-L322

Hi @florianpilz! I'm very glad to hear that the linked PR will be helpful!

As for expected error classes and error messages, I believe those are now covered as well. I think the code changes cover everything, but my text summary was insufficient.

Previously, the only way an expected error would not impact Apdex would be if the notice_error API was used with an expected: true key/value pair argument. With PR #2619, that check for an expected: true attribute on the error is still being checked, but we are now additionally performing an ErrorCollector#expected? check on the exception and the HTTP response code (which can be nil). The exception and optional HTTP response code are sent to ErrorFilter#expected? which looks like this:

def expected?(ex, status_code = nil)
  @expected_classes.include?(ex.class.name) ||
    (@expected_messages.key?(ex.class.name) &&
    @expected_messages[ex.class.name].any? { |m| ex.message.include?(m) }) ||
    @expected_status_codes.include?(status_code.to_i)
end

That method will take care of checking for configured expected classes and messages by looking at the exception argument, and then check for an expected HTTP status code by looking at the optional status code argument.

The inclusion of these #expected? methods is the big change the PR introduced.

So I think what happened is that the code in the PR does everything we want, but my description was limited by only mentioning the status codes. If after taking another look you disagree, please let me know and I'd be happy to investigate farther.

We'll keep you posted via comments on this issue for when these changes make it into a new agent gem release.

[fallwith]

@florianpilz I have updated the PR description text, the CHANGELOG, and the source code comments. Thanks very much for pointing out that the error classes and error messages are also possible ways to expect errors! See acb8297

[fallwith]

Hi @florianpilz. The PR has now been approved and merged, so the bugfix should go out in the next release of the New Relic Ruby agent. Thank you again for bringing this issue to our attention!

[florianpilz]

Really nice! You are right, I only got distracted by HTTP codes being explicitly passed. Makes all sense now and looking forward to the next release. 🎉 ❤️