Merge pull request #451 from openedx/diana/add-error-tagging

feat: Add root span tagging for exceptions.

CHANGELOG.rst

@@ -11,6 +11,18 @@ Change Log
 
 .. There should always be an "Unreleased" section for changes pending release.
 
+[5.16.0] - 2024-09-27
+---------------------
+Added
+~~~~~
+* Added a new method to backends for ``tag_root_span_with_error`` and added Datadog implementation of the functionality.
+* Uses the new method to tag the root span when processing exceptions.
+
+Changed
+~~~~~~~
+* Renamed ``CachedCustomMonitoringMiddleware`` to ``MonitoringSupportMiddleware`` and deprecated the old name. It will be removed in a future release.
+
+
 [5.15.0] - 2024-07-29
 ---------------------
 Added

edx_django_utils/__init__.py

@@ -2,7 +2,7 @@
 EdX utilities for Django Application development..
 """
 
-__version__ = "5.15.0"
+__version__ = "5.16.0"
 
 default_app_config = (
     "edx_django_utils.apps.EdxDjangoUtilsConfig"

edx_django_utils/monitoring/__init__.py

@@ -15,7 +15,8 @@
     CookieMonitoringMiddleware,
     DeploymentMonitoringMiddleware,
     FrontendMonitoringMiddleware,
-    MonitoringMemoryMiddleware
+    MonitoringMemoryMiddleware,
+    MonitoringSupportMiddleware
 )
 from .internal.transactions import get_current_transaction, ignore_transaction, set_monitoring_transaction_name
 from .internal.utils import (

edx_django_utils/monitoring/internal/backends.py

@@ -62,6 +62,14 @@ def create_span(self, name):
         or create a new root span if not currently in a span.
         """
 
+    @abstractmethod
+    def tag_root_span_with_error(self, exception):
+        """
+        Tags the root span with the given exception. This is primarily useful for
+        Datadog as New Relic handles this behavior correctly. Unclear if this is also needs to
+        be implemented for OTEL.
+        """
+
 
 class NewRelicBackend(TelemetryBackend):
     """
@@ -96,6 +104,10 @@ def create_span(self, name):
             nr_transaction = newrelic.agent.current_transaction()
             return newrelic.agent.FunctionTrace(nr_transaction, name)
 
+    def tag_root_span_with_error(self, exception):
+        # Does not need to be implemented for NewRelic, because it is handled automatically.
+        pass
+
 
 class OpenTelemetryBackend(TelemetryBackend):
     """
@@ -121,6 +133,10 @@ def create_span(self, name):
         # Currently, this is not implemented.
         pass
 
+    def tag_root_span_with_error(self, exception):
+        # Currently, this is not implemented for OTel
+        pass
+
 
 class DatadogBackend(TelemetryBackend):
     """
@@ -145,6 +161,10 @@ def record_exception(self):
     def create_span(self, name):
         return self.dd_tracer.trace(name)
 
+    def tag_root_span_with_error(self, exception):
+        root_span = self.dd_tracer.current_root_span()
+        root_span.set_exc_info(type(exception), exception, exception.__traceback__)
+
 
 # We're using an lru_cache instead of assigning the result to a variable on
 # module load. With the default settings (pointing to a TelemetryBackend

edx_django_utils/monitoring/internal/middleware.py

@@ -70,9 +70,12 @@ def record_python_version():
         _set_custom_attribute('python_version', platform.python_version())
 
 
-class CachedCustomMonitoringMiddleware(MiddlewareMixin):
+class MonitoringSupportMiddleware(MiddlewareMixin):
     """
-    Middleware batch reports cached custom attributes at the end of a request.
+    Middleware to support monitoring.
+
+    1. Middleware batch reports cached custom attributes at the end of a request.
+    2. Middleware adds error span tags to the root span.
 
     Make sure to add below the request cache in MIDDLEWARE.
 
@@ -130,6 +133,13 @@ def _batch_report(cls):
         for key, value in attributes_cache.data.items():
             _set_custom_attribute(key, value)
 
+    def _tag_root_span_with_error(self, exception):
+        """
+        Tags the root span with the exception information for all configured backends.
+        """
+        for backend in configured_backends():
+            backend.tag_root_span_with_error(exception)
+
     # Whether or not there was an exception, report any custom NR attributes that
     # may have been collected.
 
@@ -145,6 +155,16 @@ def process_exception(self, request, exception):    # pylint: disable=W0613
         Django middleware handler to process an exception
         """
         self._batch_report()
+        self._tag_root_span_with_error(exception)
+
+
+class CachedCustomMonitoringMiddleware(MonitoringSupportMiddleware):
+    """
+    DEPRECATED: Use MonitoringSupportMiddleware instead.
+
+    This is the old name for the MonitoringSupportMiddleware. We are keeping it
+    around for backwards compatibility until it can be fully removed.
+    """
 
 
 def _set_custom_attribute(key, value):

edx_django_utils/monitoring/tests/test_custom_monitoring.py

@@ -6,10 +6,10 @@
 from unittest.mock import Mock, call, patch
 
 import ddt
-from django.test import TestCase
+from django.test import TestCase, override_settings
 
 from edx_django_utils.cache import RequestCache
-from edx_django_utils.monitoring import CachedCustomMonitoringMiddleware, accumulate, get_current_transaction, increment
+from edx_django_utils.monitoring import MonitoringSupportMiddleware, accumulate, get_current_transaction, increment
 
 from ..middleware import CachedCustomMonitoringMiddleware as DeprecatedCachedCustomMonitoringMiddleware
 from ..middleware import MonitoringCustomMetricsMiddleware as DeprecatedMonitoringCustomMetricsMiddleware
@@ -31,8 +31,8 @@ def setUp(self):
 
     @patch('newrelic.agent')
     @ddt.data(
-        (CachedCustomMonitoringMiddleware, False, 'process_response'),
-        (CachedCustomMonitoringMiddleware, False, 'process_exception'),
+        (MonitoringSupportMiddleware, False, 'process_response'),
+        (MonitoringSupportMiddleware, False, 'process_exception'),
         (DeprecatedCachedCustomMonitoringMiddleware, True, 'process_response'),
         (DeprecatedMonitoringCustomMetricsMiddleware, True, 'process_response'),
     )
@@ -76,7 +76,7 @@ def test_accumulate_and_increment(
 
     @patch('newrelic.agent')
     @ddt.data(
-        (CachedCustomMonitoringMiddleware, False),
+        (MonitoringSupportMiddleware, False),
         (DeprecatedCachedCustomMonitoringMiddleware, True),
         (DeprecatedMonitoringCustomMetricsMiddleware, True),
     )
@@ -113,6 +113,21 @@ def test_accumulate_with_illegal_value(
         # Assert call args to newrelic.agent.add_custom_parameter().
         mock_newrelic_agent.add_custom_parameter.assert_has_calls(nr_agent_calls_expected, any_order=True)
 
+    @patch('ddtrace.Tracer.current_root_span')
+    def test_error_tagging(self, mock_get_root_span):
+        # Ensure that we continue to support tagging exceptions in MonitoringSupportMiddleware.
+        # This is only implemented for DatadogBackend at the moment.
+        fake_exception = Exception()
+        mock_root_span = Mock()
+        mock_get_root_span.return_value = mock_root_span
+        with override_settings(OPENEDX_TELEMETRY=['edx_django_utils.monitoring.DatadogBackend']):
+            MonitoringSupportMiddleware(self.mock_response).process_exception(
+                'fake request', fake_exception
+            )
+            mock_root_span.set_exc_info.assert_called_with(
+                type(fake_exception), fake_exception, fake_exception.__traceback__
+            )
+
     @patch('newrelic.agent')
     def test_get_current_transaction(self, mock_newrelic_agent):
         mock_newrelic_agent.current_transaction().name = 'fake-transaction'