From b13b57b9b0e011d59deae1d3104193b295e5c0e1 Mon Sep 17 00:00:00 2001 From: Justin Richert Date: Tue, 3 Jan 2023 17:04:39 -0600 Subject: [PATCH 1/3] Add aiohttp support for expected status codes --- newrelic/api/time_trace.py | 6 ++++- newrelic/hooks/framework_aiohttp.py | 18 ++++++++++++- .../framework_aiohttp/_target_application.py | 5 ++++ tests/framework_aiohttp/test_server.py | 7 +++++- tests/testing_support/fixtures.py | 25 +++++++++++++++++++ .../validators/validate_transaction_errors.py | 13 +++++++--- 6 files changed, 68 insertions(+), 6 deletions(-) diff --git a/newrelic/api/time_trace.py b/newrelic/api/time_trace.py index dc010674cb..657332af06 100644 --- a/newrelic/api/time_trace.py +++ b/newrelic/api/time_trace.py @@ -327,6 +327,10 @@ def _observe_exception(self, exc_info=None, ignore=None, expected=None, status_c if is_expected is None and callable(expected): is_expected = expected(exc, value, tb) + # Callable on transaction + if is_expected is None and hasattr(transaction, "_is_expected"): + is_expected = transaction._is_expected(exc, value, tb) + # List of class names if is_expected is None and expected is not None and not callable(expected): # Do not set is_expected to False @@ -631,7 +635,7 @@ def get_service_linking_metadata(application=None, settings=None): if application is None: from newrelic.api.application import application_instance application = application_instance(activate=False) - + if application is not None: settings = application.settings diff --git a/newrelic/hooks/framework_aiohttp.py b/newrelic/hooks/framework_aiohttp.py index de72ae0c5b..cc13894350 100644 --- a/newrelic/hooks/framework_aiohttp.py +++ b/newrelic/hooks/framework_aiohttp.py @@ -26,7 +26,7 @@ function_wrapper, wrap_function_wrapper, ) -from newrelic.core.config import should_ignore_error +from newrelic.core.config import is_expected_error, should_ignore_error SUPPORTED_METHODS = ("connect", "head", "get", "delete", "options", "patch", "post", "put", "trace") @@ -61,6 +61,19 @@ def _should_ignore(exc, value, tb): return _should_ignore +def is_expected(transaction): + settings = transaction.settings + + def _is_expected(exc, value, tb): + from aiohttp import web + + if isinstance(value, web.HTTPException): + status_code = value.status_code + return is_expected_error((exc, value, tb), status_code, settings=settings) + + return _is_expected + + def _nr_process_response_proxy(response, transaction): nr_headers = transaction.process_response(response.status, response.headers) @@ -338,6 +351,9 @@ async def _coro(*_args, **_kwargs): # Patch in should_ignore to all notice_error calls transaction._ignore_errors = should_ignore(transaction) + # Patch in is_expected to all notice_error calls + transaction._is_expected = is_expected(transaction) + import aiohttp transaction.add_framework_info(name="aiohttp", version=aiohttp.__version__) diff --git a/tests/framework_aiohttp/_target_application.py b/tests/framework_aiohttp/_target_application.py index 77d6fef6c9..f15e7fd65b 100644 --- a/tests/framework_aiohttp/_target_application.py +++ b/tests/framework_aiohttp/_target_application.py @@ -40,6 +40,10 @@ async def non_500_error(request): raise web.HTTPGone() +async def raise_403(request): + raise web.HTTPForbidden() + + async def raise_404(request): raise web.HTTPNotFound() @@ -167,6 +171,7 @@ def make_app(middlewares=None): app.router.add_route("*", "/error", error) app.router.add_route("*", "/known_error", KnownErrorView) app.router.add_route("*", "/non_500_error", non_500_error) + app.router.add_route("*", "/raise_403", raise_403) app.router.add_route("*", "/raise_404", raise_404) app.router.add_route("*", "/hang", hang) app.router.add_route("*", "/background", background) diff --git a/tests/framework_aiohttp/test_server.py b/tests/framework_aiohttp/test_server.py index aa6218c28a..6a5ef0d10e 100644 --- a/tests/framework_aiohttp/test_server.py +++ b/tests/framework_aiohttp/test_server.py @@ -19,6 +19,7 @@ from testing_support.fixtures import ( count_transactions, override_application_settings, + override_expected_status_codes, override_generic_settings, override_ignore_status_codes, ) @@ -64,6 +65,7 @@ ("/error?hello=world", "_target_application:error", "builtins:ValueError", 500), ("/non_500_error?hello=world", "_target_application:non_500_error", "aiohttp.web_exceptions:HTTPGone", 410), ("/raise_404?hello=world", "_target_application:raise_404", None, 404), + ("/raise_403?hello=world", "_target_application:raise_403", "aiohttp.web_exceptions:HTTPForbidden", 403), ], ) def test_error_exception(method, uri, metric_name, error, status, nr_enabled, aiohttp_app): @@ -79,7 +81,9 @@ async def fetch(): if error: errors.append(error) - @validate_transaction_errors(errors=errors) + @validate_transaction_errors( + errors=errors, expected_errors=["aiohttp.web_exceptions:HTTPForbidden"] + ) @validate_transaction_metrics( metric_name, scoped_metrics=[ @@ -111,6 +115,7 @@ async def fetch(): ) @validate_code_level_metrics(*metric_name.split(":")) @override_ignore_status_codes([404]) + @override_expected_status_codes([403]) def _test(): aiohttp_app.loop.run_until_complete(fetch()) diff --git a/tests/testing_support/fixtures.py b/tests/testing_support/fixtures.py index f642d1f6f1..e0a2f9abd3 100644 --- a/tests/testing_support/fixtures.py +++ b/tests/testing_support/fixtures.py @@ -1479,6 +1479,31 @@ def _override_ignore_status_codes(wrapped, instance, args, kwargs): return _override_ignore_status_codes +def override_expected_status_codes(status_codes): + @function_wrapper + def _override_expected_status_codes(wrapped, instance, args, kwargs): + # Updates can be made to expected status codes in server + # side configs. Changes will be applied to application + # settings so we first check there and if they don't + # exist, we default to global settings + + application = application_instance() + settings = application and application.settings + + if not settings: + settings = global_settings() + + original = settings.error_collector.expected_status_codes + + try: + settings.error_collector.expected_status_codes = status_codes + return wrapped(*args, **kwargs) + finally: + settings.error_collector.expected_status_codes = original + + return _override_expected_status_codes + + def code_coverage_fixture(source=None): if source is None: source = ["newrelic"] diff --git a/tests/testing_support/validators/validate_transaction_errors.py b/tests/testing_support/validators/validate_transaction_errors.py index b00b7facd1..04a34d262d 100644 --- a/tests/testing_support/validators/validate_transaction_errors.py +++ b/tests/testing_support/validators/validate_transaction_errors.py @@ -12,18 +12,20 @@ # See the License for the specific language governing permissions and # limitations under the License. -import copy - from newrelic.common.object_wrapper import ( function_wrapper, transient_function_wrapper, ) from testing_support.fixtures import catch_background_exceptions -def validate_transaction_errors(errors=None, required_params=None, forgone_params=None): + +def validate_transaction_errors( + errors=None, required_params=None, forgone_params=None, expected_errors=None +): errors = errors or [] required_params = required_params or [] forgone_params = forgone_params or [] + expected_errors = expected_errors or [] captured_errors = [] @transient_function_wrapper("newrelic.core.stats_engine", "StatsEngine.record_transaction") @@ -71,6 +73,11 @@ def _validate_transaction_errors(wrapped, instance, args, kwargs): for name, value in forgone_params: assert name not in e.custom_params, "name=%r, params=%r" % (name, e.custom_params) + if e.type in expected_errors: + assert e.expected is True + else: + assert e.expected is False + return output return _validate_transaction_errors From d661173ff6d2f371ade179b843f75cd9395ea168 Mon Sep 17 00:00:00 2001 From: Tim Pansino Date: Thu, 9 Feb 2023 16:01:22 -0800 Subject: [PATCH 2/3] Adjust naming convention --- newrelic/api/time_trace.py | 4 ++-- newrelic/hooks/framework_aiohttp.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/newrelic/api/time_trace.py b/newrelic/api/time_trace.py index 657332af06..31de73536f 100644 --- a/newrelic/api/time_trace.py +++ b/newrelic/api/time_trace.py @@ -328,8 +328,8 @@ def _observe_exception(self, exc_info=None, ignore=None, expected=None, status_c is_expected = expected(exc, value, tb) # Callable on transaction - if is_expected is None and hasattr(transaction, "_is_expected"): - is_expected = transaction._is_expected(exc, value, tb) + if is_expected is None and hasattr(transaction, "_expect_errors"): + is_expected = transaction._expect_errors(exc, value, tb) # List of class names if is_expected is None and expected is not None and not callable(expected): diff --git a/newrelic/hooks/framework_aiohttp.py b/newrelic/hooks/framework_aiohttp.py index cc13894350..68f4e70f1c 100644 --- a/newrelic/hooks/framework_aiohttp.py +++ b/newrelic/hooks/framework_aiohttp.py @@ -352,7 +352,7 @@ async def _coro(*_args, **_kwargs): transaction._ignore_errors = should_ignore(transaction) # Patch in is_expected to all notice_error calls - transaction._is_expected = is_expected(transaction) + transaction._expect_errors = is_expected(transaction) import aiohttp From 3c876cb8a704d2c20a925369c64a4570e6f5f5da Mon Sep 17 00:00:00 2001 From: Tim Pansino Date: Thu, 9 Feb 2023 16:20:30 -0800 Subject: [PATCH 3/3] Fix expected tests for new validator behavior --- tests/agent_features/test_ignore_expected_errors.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/agent_features/test_ignore_expected_errors.py b/tests/agent_features/test_ignore_expected_errors.py index 5cf61eced5..93595aa35b 100644 --- a/tests/agent_features/test_ignore_expected_errors.py +++ b/tests/agent_features/test_ignore_expected_errors.py @@ -94,8 +94,9 @@ def test_classes_error_event_inside_transaction(settings, expected, ignore): error_count = 1 if not ignore else 0 errors = _test_runtime_error if not ignore else [] + expected_errors = _runtime_error_name if expected and not ignore else None - @validate_transaction_errors(errors=errors) + @validate_transaction_errors(errors=errors, expected_errors=expected_errors) @validate_error_event_sample_data( required_attrs=attributes, required_user_attrs=False, @@ -268,8 +269,9 @@ def test_status_codes_inside_transaction(settings, expected, ignore, status_code error_count = 1 if not ignore else 0 errors = _test_teapot_error if not ignore else [] + expected_errors = _teapot_error_name if expected and not ignore else None - @validate_transaction_errors(errors=errors) + @validate_transaction_errors(errors=errors, expected_errors=expected_errors) @validate_error_event_sample_data( required_attrs=attributes, required_user_attrs=False, @@ -359,8 +361,9 @@ def test_mixed_ignore_expected_settings_inside_transaction( error_count = 1 if not ignore else 0 errors = _test_runtime_error if not ignore else [] + expected_errors = _runtime_error_name if expected and not ignore else None - @validate_transaction_errors(errors=errors) + @validate_transaction_errors(errors=errors, expected_errors=expected_errors) @validate_error_event_sample_data( required_attrs=attributes, required_user_attrs=False, @@ -428,8 +431,9 @@ def test_overrides_inside_transaction(override, result, parameter): error_count = 1 if not ignore else 0 errors = _test_runtime_error if not ignore else [] + expected_errors = _runtime_error_name if expected and not ignore else None - @validate_transaction_errors(errors=errors) + @validate_transaction_errors(errors=errors, expected_errors=expected_errors) @validate_error_event_sample_data( required_attrs=attributes, required_user_attrs=False,