From 7e35c1904d06f830fddbcbc996e3f0ca7475a42b Mon Sep 17 00:00:00 2001 From: Mb Date: Fri, 16 Aug 2024 06:01:39 +0200 Subject: [PATCH] Refactor SQL query time calculation to use Django aggregation (#720) * Use db aggregation of python sum for total time * Fix typo --------- Co-authored-by: Albert Wang --- project/tests/test_models.py | 5 ++--- silk/models.py | 24 ++++++++++++++---------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/project/tests/test_models.py b/project/tests/test_models.py index 0d0bc9e9..03dd90fe 100644 --- a/project/tests/test_models.py +++ b/project/tests/test_models.py @@ -69,7 +69,6 @@ def test_time_spent_on_sql_queries_if_has_no_related_SQLQueries(self): self.assertEqual(self.obj.time_spent_on_sql_queries, 0) - # FIXME probably a bug def test_time_spent_on_sql_queries_if_has_related_SQLQueries_with_no_time_taken(self): query = SQLQueryFactory() @@ -77,8 +76,8 @@ def test_time_spent_on_sql_queries_if_has_related_SQLQueries_with_no_time_taken( self.assertEqual(query.time_taken, None) - with self.assertRaises(TypeError): - self.obj.time_spent_on_sql_queries + # No exception should be raised, and the result should be 0.0 + self.assertEqual(self.obj.time_spent_on_sql_queries, 0.0) def test_time_spent_on_sql_queries_if_has_related_SQLQueries_and_time_taken(self): diff --git a/silk/models.py b/silk/models.py index c4d245f1..e86a31d6 100644 --- a/silk/models.py +++ b/silk/models.py @@ -17,6 +17,7 @@ IntegerField, ManyToManyField, OneToOneField, + Sum, TextField, ) from django.utils import timezone @@ -124,16 +125,13 @@ def profile_table(self): @property def time_spent_on_sql_queries(self): + """" + Calculate the total time spent in milliseconds on SQL queries using Django aggregates. """ - TODO: Perhaps there is a nicer way to do this with Django aggregates? - My initial thought was to perform: - SQLQuery.objects.filter.aggregate(Sum(F('end_time')) - Sum(F('start_time'))) - However this feature isnt available yet, however there has been talk - for use of F objects within aggregates for four years - here: https://code.djangoproject.com/ticket/14030. It looks - like this will go in soon at which point this should be changed. - """ - return sum(x.time_taken for x in SQLQuery.objects.filter(request=self)) + result = SQLQuery.objects.filter(request=self).aggregate( + total_time=Sum('time_taken', output_field=FloatField()) + ) + return result['total_time'] or 0.0 @property def headers(self): @@ -381,4 +379,10 @@ def is_context_profile(self): @property def time_spent_on_sql_queries(self): - return sum(x.time_taken for x in self.queries.all()) + """ + Calculate the total time spent in milliseconds on SQL queries using Django aggregates. + """ + result = self.queries.aggregate( + total_time=Sum('time_taken', output_field=FloatField()) + ) + return result['total_time'] or 0.0