From ddcc93c764d01d2e0082e4d2caeab11e796a2019 Mon Sep 17 00:00:00 2001 From: newrelic Date: Wed, 28 Oct 2020 12:19:09 -0700 Subject: [PATCH] Update Starlette transaction naming hierarchy. --- newrelic/core/config.py | 20 ++++++++++-- newrelic/core/environment.py | 12 +++---- newrelic/hooks/framework_starlette.py | 31 ++++++++++++++----- tests/agent_unittests/test_environment.py | 31 +++++++++++++++++++ .../test_region_aware_settings.py | 16 ++++++++++ 5 files changed, 92 insertions(+), 18 deletions(-) diff --git a/newrelic/core/config.py b/newrelic/core/config.py index 3bbddbd7cc..cb5a5e90a5 100644 --- a/newrelic/core/config.py +++ b/newrelic/core/config.py @@ -90,6 +90,21 @@ def create_settings(nested): return type('Settings', (Settings,), {'nested': nested})() +class TopLevelSettings(Settings): + _host = None + + @property + def host(self): + if self._host: + return self._host + else: + return default_host(self.license_key) + + @host.setter + def host(self, value): + self._host = value + + class AttributesSettings(Settings): pass @@ -303,7 +318,7 @@ class EventHarvestConfigHarvestLimitSettings(Settings): nested = True -_settings = Settings() +_settings = TopLevelSettings() _settings.attributes = AttributesSettings() _settings.thread_profiler = ThreadProfilerSettings() _settings.transaction_tracer = TransactionTracerSettings() @@ -502,8 +517,7 @@ def default_host(license_key): _settings.ssl = _environ_as_bool('NEW_RELIC_SSL', True) -_settings.host = os.environ.get('NEW_RELIC_HOST', - default_host(_settings.license_key)) +_settings.host = os.environ.get('NEW_RELIC_HOST') _settings.port = int(os.environ.get('NEW_RELIC_PORT', '0')) _settings.agent_run_id = None diff --git a/newrelic/core/environment.py b/newrelic/core/environment.py index 1a79306c9e..ef12ca3b9d 100644 --- a/newrelic/core/environment.py +++ b/newrelic/core/environment.py @@ -185,16 +185,14 @@ def environment_settings(): plugins = [] - # Using six to create create a snapshot of sys.modules can occassionally + # Using any iterable to create a snapshot of sys.modules can occassionally # fail in a rare case when modules are imported in parallel by different - # threads. This is because list(six.iteritems(sys.modules)) results in - # list(iter(sys.modules.iteritems())), which means sys.modules could change - # between the time when the iterable is handed over from the iter() to - # list(). + # threads. # - # TL;DR: Do NOT use six module for the following iteration. + # TL;DR: Do NOT use an iterable on the original sys.modules to generate the + # list - for name, module in list(sys.modules.items()): + for name, module in sys.modules.copy().items(): # If the module isn't actually loaded (such as failed relative imports # in Python 2.7), the module will be None and should not be reported. if module is None: diff --git a/newrelic/hooks/framework_starlette.py b/newrelic/hooks/framework_starlette.py index d3048457c5..acbd88a0af 100644 --- a/newrelic/hooks/framework_starlette.py +++ b/newrelic/hooks/framework_starlette.py @@ -51,7 +51,7 @@ def route_naming_wrapper(wrapped, instance, args, kwargs): with RequestContext(bind_request(*args, **kwargs)): transaction = current_transaction() if transaction: - transaction.set_transaction_name(callable_name(wrapped), priority=3) + transaction.set_transaction_name(callable_name(wrapped), priority=2) return wrapped(*args, **kwargs) @@ -83,14 +83,21 @@ def wrap_background_method(wrapped, instance, args, kwargs): return wrapped(*args, **kwargs) -@function_wrapper -def wrap_middleware(wrapped, instance, args, kwargs): - result = wrapped(*args, **kwargs) +async def middleware_wrapper(wrapped, instance, args, kwargs): + transaction = current_transaction() + if transaction: + transaction.set_transaction_name(callable_name(wrapped), priority=1) - dispatch_func = getattr(result, "dispatch_func", None) + dispatch_func = getattr(wrapped, "dispatch_func", None) name = dispatch_func and callable_name(dispatch_func) - return FunctionTraceWrapper(result, name=name) + return await FunctionTraceWrapper(wrapped, name=name)(*args, **kwargs) + + +@function_wrapper +def wrap_middleware(wrapped, instance, args, kwargs): + result = wrapped(*args, **kwargs) + return FunctionWrapper(result, middleware_wrapper) def bind_middleware(middleware_class, *args, **kwargs): @@ -159,6 +166,14 @@ def wrap_add_exception_handler(wrapped, instance, args, kwargs): return wrapped(exc_class_or_status_code, handler, *args, **kwargs) +def error_middleware_wrapper(wrapped, instance, args, kwargs): + transaction = current_transaction() + if transaction: + transaction.set_transaction_name(callable_name(wrapped), priority=1) + + return FunctionTraceWrapper(wrapped)(*args, **kwargs) + + def instrument_starlette_applications(module): framework = framework_details() version_info = tuple(int(v) for v in framework[1].split(".", 3)[:3]) @@ -178,7 +193,7 @@ def instrument_starlette_requests(module): def instrument_starlette_middleware_errors(module): - wrap_function_trace(module, "ServerErrorMiddleware.__call__") + wrap_function_wrapper(module, "ServerErrorMiddleware.__call__", error_middleware_wrapper) wrap_function_wrapper(module, "ServerErrorMiddleware.__init__", wrap_server_error_handler) @@ -188,7 +203,7 @@ def instrument_starlette_middleware_errors(module): def instrument_starlette_exceptions(module): - wrap_function_trace(module, "ExceptionMiddleware.__call__") + wrap_function_wrapper(module, "ExceptionMiddleware.__call__", error_middleware_wrapper) wrap_function_wrapper(module, "ExceptionMiddleware.http_exception", wrap_exception_handler) diff --git a/tests/agent_unittests/test_environment.py b/tests/agent_unittests/test_environment.py index 38964b2666..ef5c5e4488 100644 --- a/tests/agent_unittests/test_environment.py +++ b/tests/agent_unittests/test_environment.py @@ -43,6 +43,37 @@ def test_plugin_list(): assert "newrelic.hooks.newrelic" not in plugin_list +class NoIteratorDict(object): + def __init__(self, d): + self.d = d + + def copy(self): + return self.d.copy() + + def get(self, *args, **kwargs): + return self.d.get(*args, **kwargs) + + def __getitem__(self, *args, **kwargs): + return self.d.__getitem__(*args, **kwargs) + + def __contains__(self, *args, **kwargs): + return self.d.__contains__(*args, **kwargs) + + +def test_plugin_list_uses_no_sys_modules_iterator(monkeypatch): + modules = NoIteratorDict(sys.modules) + monkeypatch.setattr(sys, 'modules', modules) + + # If environment_settings iterates over sys.modules, an attribute error will be generated + environment_info = environment_settings() + + for key, plugin_list in environment_info: + if key == "Plugin List": + break + else: + assert False, "'Plugin List' not found" + + @pytest.mark.parametrize( "loaded_modules,dispatcher,dispatcher_version,worker_version", ( diff --git a/tests/agent_unittests/test_region_aware_settings.py b/tests/agent_unittests/test_region_aware_settings.py index fd017aecf7..7b47640497 100644 --- a/tests/agent_unittests/test_region_aware_settings.py +++ b/tests/agent_unittests/test_region_aware_settings.py @@ -81,3 +81,19 @@ def test_region_aware_license_keys(ini, env, expected_host, settings = global_settings() assert settings.host == expected_host assert settings.license_key == expected_license_key + + +@pytest.mark.parametrize("ini,license_key,host_override,expected_host", ( + (INI_FILE_WITHOUT_LICENSE_KEY, NO_REGION_KEY, None, "collector.newrelic.com"), + (INI_FILE_WITHOUT_LICENSE_KEY, EU01_KEY, None, "collector.eu01.nr-data.net"), + (INI_FILE_WITHOUT_LICENSE_KEY, NO_REGION_KEY, "foo.newrelic.com", "foo.newrelic.com"), +)) +def test_region_aware_global_settings(ini, license_key, host_override, + expected_host, global_settings): + settings = global_settings() + + if host_override: + settings.host = host_override + + settings.license_key = license_key + assert settings.host == expected_host