Django middleware filtering settings (#1444)

* Reduce number of spans in django framework (#779)

* Do not wrap useless middlewares

* Fixup: use frozenset

* Add config settings

* Add middleware enable/disable options

* Add testing

* Testing exclude/include settings

* Add optional fixture scope argument

* Rewrite tests to use fixtures

* Add new fixture

* Fix tests

* Fix ruff errors

* MegaLinter Fixes

* Add config file tests

* MegaLinter fixes

* Reviewer changes

* MegaLinter fixes

* Add InstrumentationMiddlewareSettings

* More exclude/include filter tests

* Megalinter fixes

* Tests to increase coverage

* Megalinter fixes

* More coverage tests

* ANOTHER TEST:

---------

Co-authored-by: Hannah Stepanek <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: 3 people

newrelic/config.py

@@ -215,6 +215,10 @@ def _map_inc_excl_attributes(s):
     return newrelic.core.config._parse_attributes(s)
 
 
+def _map_inc_excl_middleware(s):
+    return newrelic.core.config._parse_attributes(s, middleware=True)
+
+
 def _map_case_insensitive_excl_labels(s):
     return [v.lower() for v in newrelic.core.config._parse_attributes(s)]
 
@@ -510,6 +514,9 @@ def _process_configuration(section):
         section, "instrumentation.kombu.ignored_exchanges", "get", newrelic.core.config.parse_space_separated_into_list
     )
     _process_setting(section, "instrumentation.kombu.consumer.enabled", "getboolean", None)
+    _process_setting(section, "instrumentation.middleware.django.enabled", "getboolean", None)
+    _process_setting(section, "instrumentation.middleware.django.exclude", "get", _map_inc_excl_middleware)
+    _process_setting(section, "instrumentation.middleware.django.include", "get", _map_inc_excl_middleware)
 
 
 # Loading of configuration from specified file and for specified

newrelic/core/config.py

@@ -320,6 +320,14 @@ class SpanEventAttributesSettings(Settings):
     pass
 
 
+class InstrumentationMiddlewareSettings(Settings):
+    pass
+
+
+class InstrumentationDjangoMiddlewareSettings(Settings):
+    pass
+
+
 class DistributedTracingSettings(Settings):
     pass
 
@@ -507,6 +515,8 @@ class EventHarvestConfigHarvestLimitSettings(Settings):
 _settings.instrumentation.kombu = InstrumentationKombuSettings()
 _settings.instrumentation.kombu.ignored_exchanges = InstrumentationKombuIgnoredExchangesSettings()
 _settings.instrumentation.kombu.consumer = InstrumentationKombuConsumerSettings()
+_settings.instrumentation.middleware = InstrumentationMiddlewareSettings()
+_settings.instrumentation.middleware.django = InstrumentationDjangoMiddlewareSettings()
 _settings.message_tracer = MessageTracerSettings()
 _settings.process_host = ProcessHostSettings()
 _settings.rum = RumSettings()
@@ -634,11 +644,15 @@ def _parse_status_codes(value, target):
     return target
 
 
-def _parse_attributes(s):
+# Called from newrelic.config.py to parse
+# attributes and django middleware lists
+def _parse_attributes(s, middleware=False):
     valid = []
     for item in s.split():
         if "*" not in item[:-1] and len(item.encode("utf-8")) < 256:
             valid.append(item)
+        elif middleware:
+            _logger.warning("Improperly formatted middleware: %r", item)
         else:
             _logger.warning("Improperly formatted attribute: %r", item)
     return valid
@@ -904,6 +918,12 @@ def default_otlp_host(host):
     "NEW_RELIC_INSTRUMENTATION_KOMBU_CONSUMER_ENABLED", default=False
 )
 
+_settings.instrumentation.middleware.django.enabled = _environ_as_bool(
+    "NEW_RELIC_INSTRUMENTATION_DJANGO_MIDDLEWARE_ENABLED", default=True
+)
+_settings.instrumentation.middleware.django.exclude = []
+_settings.instrumentation.middleware.django.include = []
+
 _settings.event_harvest_config.harvest_limits.analytic_event_data = _environ_as_int(
     "NEW_RELIC_ANALYTICS_EVENTS_MAX_SAMPLES_STORED", DEFAULT_RESERVOIR_SIZE
 )

newrelic/hooks/framework_django.py

@@ -18,7 +18,7 @@
 import threading
 import warnings
 
-from newrelic.api.application import register_application
+from newrelic.api.application import application_settings, register_application
 from newrelic.api.background_task import BackgroundTaskWrapper
 from newrelic.api.error_trace import wrap_error_trace
 from newrelic.api.function_trace import FunctionTrace, FunctionTraceWrapper, wrap_function_trace
@@ -238,77 +238,12 @@ def wrapper(wrapped, instance, args, kwargs):
         return FunctionWrapper(wrapped, wrapper)
 
     for wrapped in middleware:
-        yield wrapper(wrapped)
-
-
-# Because this is not being used in any version of Django that is
-# within New Relic's support window, no tests will be added
-# for this.  However, value exists to keeping backwards compatible
-# functionality, so instead of removing this instrumentation, this
-# will be excluded from the coverage analysis.
-def wrap_view_middleware(middleware):  # pragma: no cover
-    # This is no longer being used. The changes to strip the
-    # wrapper from the view handler when passed into the function
-    # urlresolvers.reverse() solves most of the problems. To back
-    # that up, the object wrapper now proxies various special
-    # methods so that comparisons like '==' will work. The object
-    # wrapper can even be used as a standin for the wrapped object
-    # when used as a key in a dictionary and will correctly match
-    # the original wrapped object.
-
-    # Wrapper to be applied to view middleware. Records the time
-    # spent in the middleware as separate function node and also
-    # attempts to name the web transaction after the name of the
-    # middleware with success being determined by the priority.
-    # This wrapper is special in that it must strip the wrapper
-    # from the view handler when being passed to the view
-    # middleware to avoid issues where middleware wants to do
-    # comparisons between the passed middleware and some other
-    # value. It is believed that the view handler should never
-    # actually be called from the view middleware so not an
-    # issue that no longer wrapped at this point.
+        do_not_wrap = is_denied_middleware(callable_name(wrapped))
 
-    def wrapper(wrapped):
-        # The middleware if a class method would already be
-        # bound at this point, so is safe to determine the name
-        # when it is being wrapped rather than on each
-        # invocation.
-
-        name = callable_name(wrapped)
-
-        def wrapper(wrapped, instance, args, kwargs):
-            transaction = current_transaction()
-
-            def _wrapped(request, view_func, view_args, view_kwargs):
-                # This strips the view handler wrapper before call.
-
-                if hasattr(view_func, "_nr_last_object"):
-                    view_func = view_func._nr_last_object
-
-                return wrapped(request, view_func, view_args, view_kwargs)
-
-            if transaction is None:
-                return _wrapped(*args, **kwargs)
-
-            before = (transaction.name, transaction.group)
-
-            with FunctionTrace(name=name, source=wrapped):
-                try:
-                    return _wrapped(*args, **kwargs)
-
-                finally:
-                    # We want to name the transaction after this
-                    # middleware but only if the transaction wasn't
-                    # named from within the middleware itself explicitly.
-
-                    after = (transaction.name, transaction.group)
-                    if before == after:
-                        transaction.set_transaction_name(name, priority=2)
-
-        return FunctionWrapper(wrapped, wrapper)
-
-    for wrapped in middleware:
-        yield wrapper(wrapped)
+        if do_not_wrap:
+            yield wrapped
+        else:
+            yield wrapper(wrapped)
 
 
 def wrap_trailing_middleware(middleware):
@@ -323,7 +258,13 @@ def wrap_trailing_middleware(middleware):
     # invocation.
 
     for wrapped in middleware:
-        yield FunctionTraceWrapper(wrapped, name=callable_name(wrapped))
+        name = callable_name(wrapped)
+        do_not_wrap = is_denied_middleware(name)
+
+        if do_not_wrap:
+            yield wrapped
+        else:
+            yield FunctionTraceWrapper(wrapped, name=name)
 
 
 def insert_and_wrap_middleware(handler, *args, **kwargs):
@@ -1149,17 +1090,126 @@ def _wrapper(wrapped, instance, args, kwargs):
     return _wrapper(middleware)
 
 
+# For faster logic, could we cache the callable_names to
+# avoid this logic for multiple calls to the same middleware?
+# Also, is that even a scenario we need to worry about?
+def is_denied_middleware(callable_name):
+    settings = application_settings() or global_settings()
+
+    # Return True (skip wrapping) if:
+    # 1. middleware wrapping is disabled or
+    # 2. the callable name is in the exclude list
+    if not settings.instrumentation.middleware.django.enabled or (
+        callable_name in settings.instrumentation.middleware.django.exclude
+    ):
+        return True
+
+    # Return False (middleware will be wrapped) if:
+    # 1. If the callable name is in the include list.
+    # If we have made it to this point in the logic, that means
+    # that the callable name is not explicitly in the exclude
+    # list and, whether it is in the exclude list as a wildcard
+    # or not, the list of middleware to include explicitly takes
+    # precedence over the exclude list's wildcards.
+    # 2. enabled=True and len(exclude)==0
+    # This scenario is redundant logic, but we utilize it to
+    # speed up the logic for the common case where
+    # the user has not specified any include or exclude lists.
+    if (callable_name in settings.instrumentation.middleware.django.include) or (
+        settings.instrumentation.middleware.django.enabled
+        and len(settings.instrumentation.middleware.django.exclude) == 0
+    ):
+        return False
+
+    # =========================================================
+    # Wildcard logic for include and exclude lists:
+    # =========================================================
+    # Returns False if an entry in the include wildcard is more
+    # specific than exclude wildcard.  Otherwise, it will iterate
+    # through the entire include list and return True if no more
+    # specific include (than exclude) is found.
+    def include_logic(callable_name, exclude_middleware_name):
+        """
+        We iterate through the entire include and exclude lists to
+        ensure that the user has not included overlapping wildcards
+        that would potentially be less specific than the exclude in
+        one case and more specific than the exclude in another case.
+
+        e.g.:
+        exclude = middleware.*, middleware.parameters.bar*
+        include = middleware.parameters.*
+
+        Where `middleware.*` is less specific than `middleware.parameters.*`
+        but `middleware.parameters.bar*` is more specific than `middleware.parameters.*`
+
+        In this case, we want to make sure to exclude any middleware that
+        matches the `middleware.parameters.bar*` pattern, but include any
+        other middleware that matches the `middleware.parameters.*` pattern.
+        """
+        for include_middleware in settings.instrumentation.middleware.django.include:
+            if include_middleware.endswith("*"):
+                include_middleware_name = include_middleware.rstrip("*")
+                if callable_name.startswith(include_middleware_name):
+                    # Count number of dots in the include and exclude
+                    # middleware names to determine specificity.
+                    exclude_dots = exclude_middleware_name.count(".")
+                    include_dots = include_middleware_name.count(".")
+                    if include_dots > exclude_dots:
+                        # Include this middleware--include is more specific and takes precedence
+                        return False
+                    elif include_dots == exclude_dots:
+                        # Count number of characters after the last dot
+                        exclude_post_dot_char_count = len(exclude_middleware_name) - exclude_middleware_name.rfind(".")
+                        include_post_dot_char_count = len(include_middleware_name) - include_middleware_name.rfind(".")
+                        if include_post_dot_char_count > exclude_post_dot_char_count:
+                            # Include this middleware--include is more specific and takes precedence
+                            return False
+                        else:
+                            # Exclude wildcard has the same or greater specificity than the include wildcard
+                            # Expect to exclude this middleware--so far, exclude is more specific and takes precedence
+                            continue
+                    else:
+                        # Expect to exclude this middleware--so far, exclude is more specific and takes precedence
+                        pass
+
+        # if we have made it to this point, there is no include middleware that is
+        # more specific than the exclude middleware, so we can safely return True
+        return True
+
+    # Check if the callable name matches any of the excluded middleware patterns.
+    # If middleware name ends with '*', it is a wildcard
+    deny = False
+    for exclude_middleware in settings.instrumentation.middleware.django.exclude:
+        if exclude_middleware.endswith("*"):
+            name = exclude_middleware.rstrip("*")
+            if callable_name.startswith(name):
+                include = include_logic(callable_name, name)
+                if not include:
+                    return False
+                else:
+                    deny |= include
+
+    # If we have made it to this point, there are contents within
+    # the exclude list and those contents act as a no-op with
+    # respect to the specific middleware being evaluated
+    return deny
+
+
 def _nr_wrapper_convert_exception_to_response_(wrapped, instance, args, kwargs):
     def _bind_params(original_middleware, *args, **kwargs):
         return original_middleware
 
     original_middleware = _bind_params(*args, **kwargs)
     converted_middleware = wrapped(*args, **kwargs)
     name = callable_name(original_middleware)
+    do_not_wrap = is_denied_middleware(name)
 
-    if is_coroutine_function(converted_middleware) or is_asyncio_coroutine(converted_middleware):
-        return _nr_wrap_converted_middleware_async_(converted_middleware, name)
-    return _nr_wrap_converted_middleware_(converted_middleware, name)
+    if do_not_wrap:
+        return converted_middleware
+    else:
+        if is_coroutine_function(converted_middleware) or is_asyncio_coroutine(converted_middleware):
+            return _nr_wrap_converted_middleware_async_(converted_middleware, name)
+        return _nr_wrap_converted_middleware_(converted_middleware, name)
 
 
 def instrument_django_core_handlers_exception(module):

tests/framework_django/conftest.py

@@ -12,20 +12,22 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import pytest
 
-from testing_support.fixtures import collector_agent_registration_fixture, collector_available_fixture
+from newrelic.api.application import application_instance
+from newrelic.core.agent import agent_instance
 
-_default_settings = {
-    "package_reporting.enabled": False,  # Turn off package reporting for testing as it causes slow downs.
-    "transaction_tracer.explain_threshold": 0.0,
-    "transaction_tracer.transaction_threshold": 0.0,
-    "transaction_tracer.stack_trace_threshold": 0.0,
-    "debug.log_data_collector_payloads": True,
-    "debug.record_transaction_failure": True,
-    "debug.log_autorum_middleware": True,
-    "feature_flag": {"django.instrumentation.inclusion-tags.r1"},
-}
 
-collector_agent_registration = collector_agent_registration_fixture(
-    app_name="Python Agent Test (framework_django)", default_settings=_default_settings
-)
+# Even though `django_collector_agent_registration_fixture()` also
+# has the application deletion during the breakdown of the fixture,
+# and `django_collector_agent_registration_fixture()` is scoped to
+# "function", not all modules are using this.  Some are using
+# `collector_agent_registration_fixture()` scoped to "module".
+# Therefore, for those instances, we need to make sure that the
+# application is removed from the agent.
+@pytest.fixture(scope="module", autouse=True)
+def remove_application_from_agent():
+    yield
+    application = application_instance()
+    if application and application.name and (application.name in agent_instance()._applications):
+        del agent_instance()._applications[application.name]

tests/framework_django/test_application.py

@@ -16,6 +16,8 @@
 
 import django
 from testing_support.fixtures import (
+    collector_agent_registration_fixture,
+    collector_available_fixture,
     override_application_settings,
     override_generic_settings,
     override_ignore_status_codes,
@@ -29,6 +31,20 @@
 DJANGO_VERSION = tuple(map(int, django.get_version().split(".")[:2]))
 DJANGO_SETTINGS_MODULE = os.environ.get("DJANGO_SETTINGS_MODULE", None)
 
+_default_settings = {
+    "package_reporting.enabled": False,  # Turn off package reporting for testing as it causes slow downs.
+    "transaction_tracer.explain_threshold": 0.0,
+    "transaction_tracer.transaction_threshold": 0.0,
+    "transaction_tracer.stack_trace_threshold": 0.0,
+    "debug.log_data_collector_payloads": True,
+    "debug.record_transaction_failure": True,
+    "debug.log_autorum_middleware": True,
+}
+
+collector_agent_registration = collector_agent_registration_fixture(
+    app_name="Python Agent Test (framework_django)", default_settings=_default_settings, scope="module"
+)
+
 
 def target_application():
     from _target_application import _target_application