From d0a94bd289a4938c1756388f899fd822d82a506b Mon Sep 17 00:00:00 2001 From: "p.okapiec" Date: Tue, 22 Oct 2024 15:39:00 +0200 Subject: [PATCH 1/6] avoid making extensive queries by overriding queryset cache --- elasticapm/contrib/django/client.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/elasticapm/contrib/django/client.py b/elasticapm/contrib/django/client.py index a22098689..670929ecd 100644 --- a/elasticapm/contrib/django/client.py +++ b/elasticapm/contrib/django/client.py @@ -34,6 +34,7 @@ import django from django.conf import settings as django_settings from django.db import DatabaseError +from django.db.models.query import QuerySet from django.http import HttpRequest try: @@ -195,6 +196,14 @@ def capture(self, event_type, request=None, **kwargs): return result + @staticmethod + def _disable_querysets_evaluation(var): + """Overriding queryset result cache to avoid making any queries""" + if isinstance(var, QuerySet): + var._result_cache = [] + return var + + def _get_stack_info_for_trace( self, frames, @@ -205,6 +214,13 @@ def _get_stack_info_for_trace( ): """If the stacktrace originates within the elasticapm module, it will skip frames until some other module comes up.""" + + # Overriding processor func for django to avoid making queries to db through QuerySet objects + if locals_processor_func is not None: + locals_processor_func = lambda v: self._disable_querysets_evaluation(locals_processor_func(v)) + else: + locals_processor_func = lambda v: self._disable_querysets_evaluation(v) + return list( iterate_with_template_sources( frames, From 66c899179109a965f8a97a543d79573c8f377b37 Mon Sep 17 00:00:00 2001 From: "p.okapiec" Date: Mon, 25 Nov 2024 08:42:38 +0100 Subject: [PATCH 2/6] move variable transformation to transform encoding util --- elasticapm/contrib/django/client.py | 16 ---------------- elasticapm/utils/encoding.py | 4 ++++ tests/contrib/django/django_tests.py | 15 +++++++++++++++ tests/contrib/django/testapp/urls.py | 1 + tests/contrib/django/testapp/views.py | 14 ++++++++++++++ 5 files changed, 34 insertions(+), 16 deletions(-) diff --git a/elasticapm/contrib/django/client.py b/elasticapm/contrib/django/client.py index 670929ecd..a22098689 100644 --- a/elasticapm/contrib/django/client.py +++ b/elasticapm/contrib/django/client.py @@ -34,7 +34,6 @@ import django from django.conf import settings as django_settings from django.db import DatabaseError -from django.db.models.query import QuerySet from django.http import HttpRequest try: @@ -196,14 +195,6 @@ def capture(self, event_type, request=None, **kwargs): return result - @staticmethod - def _disable_querysets_evaluation(var): - """Overriding queryset result cache to avoid making any queries""" - if isinstance(var, QuerySet): - var._result_cache = [] - return var - - def _get_stack_info_for_trace( self, frames, @@ -214,13 +205,6 @@ def _get_stack_info_for_trace( ): """If the stacktrace originates within the elasticapm module, it will skip frames until some other module comes up.""" - - # Overriding processor func for django to avoid making queries to db through QuerySet objects - if locals_processor_func is not None: - locals_processor_func = lambda v: self._disable_querysets_evaluation(locals_processor_func(v)) - else: - locals_processor_func = lambda v: self._disable_querysets_evaluation(v) - return list( iterate_with_template_sources( frames, diff --git a/elasticapm/utils/encoding.py b/elasticapm/utils/encoding.py index 4455f2685..bd8d67f7b 100644 --- a/elasticapm/utils/encoding.py +++ b/elasticapm/utils/encoding.py @@ -36,6 +36,7 @@ import uuid from decimal import Decimal +from django.db.models import QuerySet from elasticapm.conf.constants import KEYWORD_MAX_LENGTH, LABEL_RE, LABEL_TYPES, LONG_FIELD_MAX_LENGTH PROTECTED_TYPES = (int, type(None), float, Decimal, datetime.datetime, datetime.date, datetime.time) @@ -144,6 +145,9 @@ class value_type(list): ret = float(value) elif isinstance(value, int): ret = int(value) + elif isinstance(value, QuerySet): + value._result_cache = [] + ret = repr(value) elif value is not None: try: ret = transform(repr(value)) diff --git a/tests/contrib/django/django_tests.py b/tests/contrib/django/django_tests.py index 535729bcf..4e0a3746f 100644 --- a/tests/contrib/django/django_tests.py +++ b/tests/contrib/django/django_tests.py @@ -1318,6 +1318,21 @@ def test_capture_post_errors_dict(client, django_elasticapm_client): assert error["context"]["request"]["body"] == "[REDACTED]" +@pytest.mark.parametrize( + "django_sending_elasticapm_client", + [{"capture_body": "errors"}, {"capture_body": "transactions"}, {"capture_body": "all"}, {"capture_body": "off"}], + indirect=True, +) +def test_capture_django_orm_timeout_error(client, django_sending_elasticapm_client): + with pytest.raises(DatabaseError): + client.get(reverse("elasticapm-django-orm-exc")) + + errors = django_sending_elasticapm_client.httpserver.payloads + if django_sending_elasticapm_client.config.capture_body in (constants.ERROR, "all"): + stacktrace = errors[0][1]["error"]["exception"]["stacktrace"] + assert "'qs': '[]'" in str(stacktrace) + + def test_capture_body_config_is_dynamic_for_errors(client, django_elasticapm_client): django_elasticapm_client.config.update(version="1", capture_body="all") with pytest.raises(MyException): diff --git a/tests/contrib/django/testapp/urls.py b/tests/contrib/django/testapp/urls.py index 857215280..92302e313 100644 --- a/tests/contrib/django/testapp/urls.py +++ b/tests/contrib/django/testapp/urls.py @@ -62,6 +62,7 @@ def handler500(request): re_path(r"^trigger-500-ioerror$", views.raise_ioerror, name="elasticapm-raise-ioerror"), re_path(r"^trigger-500-decorated$", views.decorated_raise_exc, name="elasticapm-raise-exc-decor"), re_path(r"^trigger-500-django$", views.django_exc, name="elasticapm-django-exc"), + re_path(r"^trigger-500-django-orm-exc$", views.django_queryset_error, name="elasticapm-django-orm-exc"), re_path(r"^trigger-500-template$", views.template_exc, name="elasticapm-template-exc"), re_path(r"^trigger-500-log-request$", views.logging_request_exc, name="elasticapm-log-request-exc"), re_path(r"^streaming$", views.streaming_view, name="elasticapm-streaming-view"), diff --git a/tests/contrib/django/testapp/views.py b/tests/contrib/django/testapp/views.py index 5a11b0961..91aa5c197 100644 --- a/tests/contrib/django/testapp/views.py +++ b/tests/contrib/django/testapp/views.py @@ -37,6 +37,8 @@ from django.http import HttpResponse, StreamingHttpResponse from django.shortcuts import get_object_or_404, render from django.views import View +from django.db.models import QuerySet +from django.db import DatabaseError import elasticapm @@ -70,6 +72,18 @@ def django_exc(request): return get_object_or_404(MyException, pk=1) +def django_queryset_error(request): + """Simulation of django ORM timeout""" + class CustomQuerySet(QuerySet): + def all(self): + raise DatabaseError() + + def __repr__(self) -> str: + return str(self._result_cache) + + qs = CustomQuerySet() + list(qs.all()) + def raise_exc(request): raise MyException(request.GET.get("message", "view exception")) From d4df8e90e87c1909a97a41eb941713aa6f3a1caa Mon Sep 17 00:00:00 2001 From: "p.okapiec" Date: Fri, 27 Dec 2024 07:30:54 +0100 Subject: [PATCH 3/6] handle django not installed, clearer failed query code --- elasticapm/utils/encoding.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/elasticapm/utils/encoding.py b/elasticapm/utils/encoding.py index bd8d67f7b..c8014d75c 100644 --- a/elasticapm/utils/encoding.py +++ b/elasticapm/utils/encoding.py @@ -36,7 +36,11 @@ import uuid from decimal import Decimal -from django.db.models import QuerySet +try: + from django.db.models import QuerySet as DjangoQuerySet +except ImportError: + DjangoQuerySet = None + from elasticapm.conf.constants import KEYWORD_MAX_LENGTH, LABEL_RE, LABEL_TYPES, LONG_FIELD_MAX_LENGTH PROTECTED_TYPES = (int, type(None), float, Decimal, datetime.datetime, datetime.date, datetime.time) @@ -145,9 +149,10 @@ class value_type(list): ret = float(value) elif isinstance(value, int): ret = int(value) - elif isinstance(value, QuerySet): - value._result_cache = [] - ret = repr(value) + elif DjangoQuerySet is not None and isinstance(value, DjangoQuerySet) and getattr(value, "_result_cache", True) is None: + # if we have a Django QuerySet a None result cache it may mean that the underlying query failed + # so represent it as an empty list instead of retrying the query again + ret = "<%s %r>" % (value.__class__.__name__, []) elif value is not None: try: ret = transform(repr(value)) From dc2282af9a55b028d6fac105d474dcbb0a0ec882 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 7 Jul 2025 16:11:23 +0200 Subject: [PATCH 4/6] Cleanup pr --- elasticapm/utils/encoding.py | 8 ++++++-- tests/contrib/django/django_tests.py | 14 ++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/elasticapm/utils/encoding.py b/elasticapm/utils/encoding.py index c8014d75c..b5ae72a85 100644 --- a/elasticapm/utils/encoding.py +++ b/elasticapm/utils/encoding.py @@ -149,10 +149,14 @@ class value_type(list): ret = float(value) elif isinstance(value, int): ret = int(value) - elif DjangoQuerySet is not None and isinstance(value, DjangoQuerySet) and getattr(value, "_result_cache", True) is None: + elif ( + DjangoQuerySet is not None + and isinstance(value, DjangoQuerySet) + and getattr(value, "_result_cache", True) is None + ): # if we have a Django QuerySet a None result cache it may mean that the underlying query failed # so represent it as an empty list instead of retrying the query again - ret = "<%s %r>" % (value.__class__.__name__, []) + ret = "<%s `unevaluated`>" % (value.__class__.__name__) elif value is not None: try: ret = transform(repr(value)) diff --git a/tests/contrib/django/django_tests.py b/tests/contrib/django/django_tests.py index 4e0a3746f..72c791280 100644 --- a/tests/contrib/django/django_tests.py +++ b/tests/contrib/django/django_tests.py @@ -1319,18 +1319,20 @@ def test_capture_post_errors_dict(client, django_elasticapm_client): @pytest.mark.parametrize( - "django_sending_elasticapm_client", + "django_elasticapm_client", [{"capture_body": "errors"}, {"capture_body": "transactions"}, {"capture_body": "all"}, {"capture_body": "off"}], indirect=True, ) -def test_capture_django_orm_timeout_error(client, django_sending_elasticapm_client): +def test_capture_django_orm_timeout_error(client, django_elasticapm_client): with pytest.raises(DatabaseError): client.get(reverse("elasticapm-django-orm-exc")) - errors = django_sending_elasticapm_client.httpserver.payloads - if django_sending_elasticapm_client.config.capture_body in (constants.ERROR, "all"): - stacktrace = errors[0][1]["error"]["exception"]["stacktrace"] - assert "'qs': '[]'" in str(stacktrace) + errors = django_elasticapm_client.events[ERROR] + if django_elasticapm_client.config.capture_body in (constants.ERROR, "all"): + stacktrace = errors[0]["exception"]["stacktrace"] + frames = [frame for frame in stacktrace if frame["function"] == "django_queryset_error"] + qs_var = frames[0]["vars"]["qs"] + assert qs_var == "" def test_capture_body_config_is_dynamic_for_errors(client, django_elasticapm_client): From 10a18190143895cad6cb3d965bbfa4cb1bfc0da2 Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 7 Jul 2025 16:14:25 +0200 Subject: [PATCH 5/6] Run pre-commit --- tests/contrib/django/testapp/views.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/contrib/django/testapp/views.py b/tests/contrib/django/testapp/views.py index 91aa5c197..906a8c2df 100644 --- a/tests/contrib/django/testapp/views.py +++ b/tests/contrib/django/testapp/views.py @@ -34,11 +34,11 @@ import time from django.contrib.auth.models import User +from django.db import DatabaseError +from django.db.models import QuerySet from django.http import HttpResponse, StreamingHttpResponse from django.shortcuts import get_object_or_404, render from django.views import View -from django.db.models import QuerySet -from django.db import DatabaseError import elasticapm @@ -74,16 +74,18 @@ def django_exc(request): def django_queryset_error(request): """Simulation of django ORM timeout""" + class CustomQuerySet(QuerySet): def all(self): raise DatabaseError() - + def __repr__(self) -> str: return str(self._result_cache) qs = CustomQuerySet() list(qs.all()) + def raise_exc(request): raise MyException(request.GET.get("message", "view exception")) From e3669513c3aa4b66e51010c2b35c9cd03cc76f8c Mon Sep 17 00:00:00 2001 From: Riccardo Magliocchetti Date: Mon, 7 Jul 2025 16:33:43 +0200 Subject: [PATCH 6/6] Update elasticapm/utils/encoding.py --- elasticapm/utils/encoding.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/elasticapm/utils/encoding.py b/elasticapm/utils/encoding.py index b5ae72a85..fedaa8d63 100644 --- a/elasticapm/utils/encoding.py +++ b/elasticapm/utils/encoding.py @@ -155,7 +155,7 @@ class value_type(list): and getattr(value, "_result_cache", True) is None ): # if we have a Django QuerySet a None result cache it may mean that the underlying query failed - # so represent it as an empty list instead of retrying the query again + # so represent it as unevaluated instead of retrying the query again ret = "<%s `unevaluated`>" % (value.__class__.__name__) elif value is not None: try: