From 878afb08410c9d3c1baa3a1e342d5e99872ba6d7 Mon Sep 17 00:00:00 2001 From: tbradellis Date: Tue, 20 Sep 2022 20:22:49 -0700 Subject: [PATCH] check expected error config for noticeError api use placeholder for config format rm double check still check for null corrections for config check tweak to fix Supportability metrics account for null throwable rm stand-in test handle null throwable refactor no expected strings remove hack reformat file. drop unused var tests for config+expected+noticeErrorAPI checkout APITest from main must close environment holder call for the correct count optimize imports remove bad test remove invalid test review revisions. clean code fix supportability check refactor no expectedByAPI bool --- .../test/agent/ExpectedErrorsTest.java | 41 +++++++++++++++- .../configs/expected_errors_test.yml | 13 +++++ .../api/agent/NewRelicApiImplementation.java | 47 +++++++++++++++---- 3 files changed, 89 insertions(+), 12 deletions(-) diff --git a/functional_test/src/test/java/test/newrelic/test/agent/ExpectedErrorsTest.java b/functional_test/src/test/java/test/newrelic/test/agent/ExpectedErrorsTest.java index 3a10fe8e7e..a5018cd18e 100644 --- a/functional_test/src/test/java/test/newrelic/test/agent/ExpectedErrorsTest.java +++ b/functional_test/src/test/java/test/newrelic/test/agent/ExpectedErrorsTest.java @@ -203,6 +203,37 @@ public void expectedStatusRange() throws Exception { } } + private static class UnExpectedException extends RuntimeException { + UnExpectedException(String msg) { + super(msg); + } + } + + @Test + public void testNoticeErrorAPIExpectedViaConfig() throws Exception { + EnvironmentHolder holder = setupEnvironemntHolder("expected_error_config_api_test"); + + try { + NewRelic.noticeError(new UnExpectedException("I'm not expected")); //nothing configured + NewRelic.noticeError(new ExpectedError("You should have expected me")); //only exception class is configured + NewRelic.noticeError(new RuntimeException("I should be expected by my message")); //exception class plus message is configured + + TransactionDataList transactionList = holder.getTransactionList(); + ServiceFactory.getHarvestService().harvestNow(); + StatsEngine statsEngine = holder.getStatsEngine(); + assertEquals(0, transactionList.size()); + + assertEquals(2, statsEngine.getStats("ErrorsExpected/all").getCallCount()); + assertEquals(1, statsEngine.getStats("Errors/all").getCallCount()); + + verifyExpectedErrorSupportabilityApiCalls(statsEngine, 0, 2, 1, 1); + } finally { + + holder.close(); + } + + } + @Test public void expectedStatusSoClose() throws Exception { EnvironmentHolder holder = setupEnvironemntHolder("non_expected_status_code_test"); @@ -251,6 +282,7 @@ public void nonExpectedErrorNoTransaction() throws Exception { verifyExpectedErrorSupportabilityApiCalls(statsEngine, 0, 0, 1, 0); verifyIgnoreErrorSupportabilityApiCalls(statsEngine, 0, 0, 0); } finally { + holder.close(); } } @@ -504,12 +536,17 @@ public void expectedClassMessagesFallbackNotIgnored() throws Exception { * the config file. However, due to how the environment holder is setup, the config is read twice, so you will see * ErrorCollectorConfigImpl#initExpectedErrors called twice which doubles our metrics. Therefore the values need to * be doubled. - * + *

+ * The apiThrowable metric is recorded in the noticeError API and does not suffer from the same need to double count + * in the assertion. + *

* When calling this method, pass in the number of 'actual' calls you expect to occur under normal operation. */ private void verifyExpectedErrorSupportabilityApiCalls(StatsEngine statsEngine, int apiMessage, int apiThrowable, int configClass, int configClassMessage) { assertEquals(2 * apiMessage, statsEngine.getStats("Supportability/API/ExpectedError/Api/Message/API").getCallCount()); - assertEquals(2 * apiThrowable, statsEngine.getStats("Supportability/API/ExpectedError/Api/Throwable/API").getCallCount()); + //The apiThrowable metric is not dependent on the config double read. You should pass in the actual expected count and the assertion should + //not double count + assertEquals(1 * apiThrowable, statsEngine.getStats("Supportability/API/ExpectedError/Api/Throwable/API").getCallCount()); assertEquals(2 * configClass, statsEngine.getStats("Supportability/API/ExpectedError/Config/Class/API").getCallCount()); assertEquals(2 * configClassMessage, statsEngine.getStats("Supportability/API/ExpectedError/Config/ClassMessage/API").getCallCount()); } diff --git a/functional_test/src/test/resources/configs/expected_errors_test.yml b/functional_test/src/test/resources/configs/expected_errors_test.yml index b21ef5d4d0..7027a0de8b 100644 --- a/functional_test/src/test/resources/configs/expected_errors_test.yml +++ b/functional_test/src/test/resources/configs/expected_errors_test.yml @@ -15,6 +15,19 @@ expected_error_test: - class_name: "test.newrelic.test.agent.ExpectedError" +expected_error_config_api_test: + + error_collector: + + expected_classes: + - + class_name: "test.newrelic.test.agent.ExpectedError" + - + class_name: "java.lang.RuntimeException" + message: "I should be expected by my message" + + + expected_error_bad_message_test: diff --git a/newrelic-agent/src/main/java/com/newrelic/api/agent/NewRelicApiImplementation.java b/newrelic-agent/src/main/java/com/newrelic/api/agent/NewRelicApiImplementation.java index 42275d73fa..67c07600c0 100644 --- a/newrelic-agent/src/main/java/com/newrelic/api/agent/NewRelicApiImplementation.java +++ b/newrelic-agent/src/main/java/com/newrelic/api/agent/NewRelicApiImplementation.java @@ -15,6 +15,7 @@ import com.newrelic.agent.bridge.AgentBridge; import com.newrelic.agent.bridge.PublicApi; import com.newrelic.agent.config.ConfigConstant; +import com.newrelic.agent.config.ExpectedErrorConfig; import com.newrelic.agent.dispatchers.Dispatcher; import com.newrelic.agent.service.ServiceFactory; import com.newrelic.agent.transaction.TransactionNamingPolicy; @@ -24,6 +25,7 @@ import java.util.Collections; import java.util.Map; import java.util.Map.Entry; +import java.util.Set; import java.util.TreeMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Level; @@ -56,7 +58,7 @@ public NewRelicApiImplementation() { */ @Override public void noticeError(Throwable throwable, Map params) { - noticeError(throwable, params, false); + noticeError(throwable, params, isExpectedErrorConfigured(throwable)); } /** @@ -68,14 +70,16 @@ public void noticeError(Throwable throwable, Map params) { @Override public void noticeError(Throwable throwable) { Map params = Collections.emptyMap(); - noticeError(throwable, params, false); + noticeError(throwable, params, isExpectedErrorConfigured(throwable)); + } /** * Notice an error and report it to New Relic. If this method is called within a transaction, the error message will * be reported with the transaction when it finishes. If it is invoked outside of a transaction, a traced error will * be created and reported to New Relic. - * + * Expecting Errors via configuration is not supported from the APIs that do not accept a Throwable as an attribute. + * To mark an Error that accepts a String as expected use the API that accepts a boolean. * @param message the error message * @param params Custom parameters to include in the traced error. May be null. Map is copied to avoid side effects. */ @@ -86,7 +90,8 @@ public void noticeError(String message, Map params) { /** * Report an error to New Relic. - * + * Expecting Errors via configuration is not supported from the APIs that do not accept a Throwable as an attribute. + * To mark an Error that accepts a String as expected use the API that accepts a boolean. * @param message the error message * @see #noticeError(String, Map) */ @@ -97,11 +102,19 @@ public void noticeError(String message) { } @Override + public void noticeError(Throwable throwable, boolean expected) { + Map params = Collections.emptyMap(); + noticeError(throwable, params, expected); + } + public void noticeError(Throwable throwable, Map params, boolean expected) { try { ServiceFactory.getRPMService().getErrorService().reportException(throwable, filterErrorAtts(params, attributeSender), expected); MetricNames.recordApiSupportabilityMetric(MetricNames.SUPPORTABILITY_API_NOTICE_ERROR); + //SUPPORTABILITY_API_EXPECTED_ERROR_API_THROWABLE metric is intended to be recorded independent of whether + //the expected error was defined in config or via the actual api. Read simply as the call came through the noticeError API (SUPPORTABILITY_API_NOTICE_ERROR) + //and it was expected. if (expected) { MetricNames.recordApiSupportabilityMetric(MetricNames.SUPPORTABILITY_API_EXPECTED_ERROR_API_THROWABLE); } @@ -116,16 +129,11 @@ public void noticeError(Throwable throwable, Map params, boolean expe } } - @Override - public void noticeError(Throwable throwable, boolean expected) { - Map params = Collections.emptyMap(); - noticeError(throwable, params, expected); - } - @Override public void noticeError(String message, Map params, boolean expected) { try { ServiceFactory.getRPMService().getErrorService().reportError(message, filterErrorAtts(params, attributeSender), expected); + MetricNames.recordApiSupportabilityMetric(MetricNames.SUPPORTABILITY_API_NOTICE_ERROR); if (expected) { MetricNames.recordApiSupportabilityMetric(MetricNames.SUPPORTABILITY_API_EXPECTED_ERROR_API_MESSAGE); @@ -147,6 +155,25 @@ public void noticeError(String message, boolean expected) { noticeError(message, params, expected); } + private boolean isExpectedErrorConfigured(Throwable throwable) { + if (throwable != null) { + String expectedConfigName = throwable.getClass().getName(); + String message = throwable.getMessage(); + String app_name = NewRelic.getAgent().getConfig().getValue("app_name"); + Set expectedErrors = ServiceFactory.getConfigService().getErrorCollectorConfig(app_name).getExpectedErrors(); + for (ExpectedErrorConfig errorConfig : expectedErrors) { + String errorClass = errorConfig.getErrorClass(); + String errorMessage = errorConfig.getErrorMessage(); + if ((errorClass.equals(expectedConfigName) && errorMessage == null) || + (errorMessage != null) && + (errorClass.equals(expectedConfigName) && errorMessage.equals(message))) { + return true; + } + } + } + return false; + } + private static Map filterErrorAtts(Map params, AttributeSender attributeSender) { Map attributes = new TreeMap<>(); if (params == null) {