Merge branch 'main' into copilot/update-docker-config-ubuntu-24-04

Author: davidfischer

CHANGELOG.rst

@@ -6,6 +6,21 @@ CHANGELOG
 .. This is included by docs/developer/changelog.rst
 
 
+Version v5.31.0
+---------------
+
+This release added Redis and Celery monitoring to Sentry in production
+as well as Sentry profiling and tracing.
+There was also a change to fix some DB locking issues with the Flight denormalized fields
+which requires the Celery task queue to update these fields every few minutes
+(hence more monitoring of Celery).
+
+:Date: November 20, 2025
+
+ * @davidfischer: Add Redis and Celery to Sentry monitoring (#1101)
+ * @ericholscher: Fix Flight denormalized field locking issues (#1099)
+
+
 Version v5.30.0
 ---------------
 

adserver/admin.py

@@ -928,9 +928,14 @@ def refund_impressions(self, request, queryset):
         )
 
         count = 0
+        flights = set()
         for impression in queryset:
             if impression.refund():
                 count += 1
+                flights.add(impression.advertisement.flight)
+
+        for flight in flights:
+            flight.refresh_denormalized_totals()
 
         messages.add_message(
             request,

adserver/models.py

@@ -1492,6 +1492,28 @@ def ecpc(self):
             return value_delivered / clicks
         return 0
 
+    def refresh_denormalized_totals(self):
+        """
+        Refresh the denormalized total_views and total_clicks fields from AdImpression.
+
+        This should be called every few minutes instead of updating
+        these fields in real-time to avoid lock contention.
+        """
+        aggregation = AdImpression.objects.filter(advertisement__flight=self).aggregate(
+            total_views=models.Sum("views"),
+            total_clicks=models.Sum("clicks"),
+        )
+
+        self.total_views = aggregation["total_views"] or 0
+        self.total_clicks = aggregation["total_clicks"] or 0
+
+        if self.sold_clicks > 0 and self.total_clicks > self.sold_clicks:
+            self.total_clicks = self.sold_clicks
+        if self.sold_impressions > 0 and self.total_views > self.sold_impressions:
+            self.total_views = self.sold_impressions
+
+        self.save(update_fields=["total_views", "total_clicks"])
+
     def copy_niche_targeting_urls(self, other_flight):
         """Copy niche targeting URLs from another flight."""
         if "adserver.analyzer" in settings.INSTALLED_APPS:
@@ -1909,15 +1931,11 @@ def incr(self, impression_type, publisher, offer=None):
         for imp_type in impression_types:
             assert imp_type in IMPRESSION_TYPES
 
-            # Update the denormalized fields on the Flight
-            if imp_type == VIEWS:
-                Flight.objects.filter(pk=self.flight_id).update(
-                    total_views=models.F("total_views") + 1
-                )
-            elif imp_type == CLICKS:
-                Flight.objects.filter(pk=self.flight_id).update(
-                    total_clicks=models.F("total_clicks") + 1
-                )
+            # NOTE: We no longer update the denormalized total_views/total_clicks fields
+            # in real-time to avoid lock contention on the Flight table.
+            # These fields are now calculated on-demand from AdImpression or
+            # refreshed periodically by a background task.
+            # See: Flight.refresh_denormalized_totals()
 
         # Ensure that an impression object exists for today
         # and make sure to query the writable DB for this
@@ -2917,19 +2935,14 @@ def refund(self):
                 **{self.impression_type: models.F(self.impression_type) - 1}
             )
 
-            # Update the denormalized impressions on the Flight
-            # And the denormalized aggregate AdImpressions
+            # Update the denormalized aggregate AdImpressions
+            # Note: We no longer update Flight.total_views/total_clicks in real-time
+            # to avoid lock contention. These are refreshed periodically.
             if self.viewed:
-                Flight.objects.filter(pk=self.advertisement.flight_id).update(
-                    total_views=models.F("total_views") - 1
-                )
                 AdImpression.objects.filter(pk=impression.pk).update(
                     **{VIEWS: models.F(VIEWS) - 1}
                 )
             if self.clicked:
-                Flight.objects.filter(pk=self.advertisement.flight_id).update(
-                    total_clicks=models.F("total_clicks") - 1
-                )
                 AdImpression.objects.filter(pk=impression.pk).update(
                     **{CLICKS: models.F(CLICKS) - 1}
                 )

adserver/tasks.py

@@ -7,6 +7,7 @@
 from django.conf import settings
 from django.contrib.sites.shortcuts import get_current_site
 from django.core import mail
+from django.core.cache import cache
 from django.db.models import Count
 from django.db.models import F
 from django.db.models import FloatField
@@ -24,6 +25,7 @@
 from .constants import FLIGHT_STATE_CURRENT
 from .constants import FLIGHT_STATE_UPCOMING
 from .constants import PAID_CAMPAIGN
+from .constants import PUBLISHER_HOUSE_CAMPAIGN
 from .importers import psf
 from .models import AdImpression
 from .models import Advertisement
@@ -924,6 +926,41 @@ def calculate_ad_ctrs(days=7, min_views=1_000):
             ad.save()
 
 
+@app.task()
+def refresh_flight_denormalized_totals():
+    """
+    Refresh denormalized total_views and total_clicks fields for all live flights.
+
+    This task should be run periodically (e.g., every 5-10 minutes) to update
+    the denormalized fields without causing lock contention on the Flight table.
+    """
+    start_time = timezone.now()
+    log.info("Starting refresh of denormalized totals for live flights")
+
+    # Only refresh active flights to avoid unnecessary work
+    flights = Flight.objects.filter(live=True).exclude(
+        campaign__campaign_type=PUBLISHER_HOUSE_CAMPAIGN
+    )
+    total_flights = flights.count()
+
+    for flight in flights:
+        flight.refresh_denormalized_totals()
+
+    # Update cache with last successful run timestamp
+    cache.set(
+        "flight_totals_last_refresh",
+        timezone.now().isoformat(),
+        timeout=None,  # Never expire
+    )
+
+    duration = (timezone.now() - start_time).total_seconds()
+    log.info(
+        "Finished refreshing denormalized totals: %d flights, took %.2fs",
+        total_flights,
+        duration,
+    )
+
+
 @app.task()
 def notify_on_ad_image_change(advertisement_id):
     ad = Advertisement.objects.filter(id=advertisement_id).first()

adserver/tests/test_decision_engine.py

@@ -271,13 +271,13 @@ def test_custom_interval(self):
             self.assertTrue(self.backend.filter_flight(self.cpm_flight))
 
             self.cpm_flight.total_views = 10_000 - pace - 1
-            self.cpm_flight.save()
+            self.cpm_flight.save(update_fields=['total_views'])
             self.assertEqual(self.cpm_flight.views_needed_this_interval(), 1)
             self.assertTrue(self.backend.filter_flight(self.cpm_flight))
 
             # Don't show this flight anymore. It is above pace
             self.cpm_flight.total_views = 10_000 - pace
-            self.cpm_flight.save()
+            self.cpm_flight.save(update_fields=['total_views'])
             self.assertEqual(self.cpm_flight.views_needed_this_interval(), 0)
             self.assertFalse(self.backend.filter_flight(self.cpm_flight))
 
@@ -300,8 +300,8 @@ def test_flight_clicks(self):
         self.advertisement1.incr(CLICKS, self.publisher)
         self.advertisement1.incr(CLICKS, self.publisher)
 
-        # Refresh the data on the include_flight - gets the denormalized views
-        self.include_flight.refresh_from_db()
+        # Refresh the denormalized totals manually since they're no longer updated in real-time
+        self.include_flight.refresh_denormalized_totals()
 
         self.assertEqual(self.include_flight.clicks_remaining(), 998)
         self.assertEqual(self.include_flight.total_clicks, 2)
@@ -315,8 +315,8 @@ def test_flight_clicks(self):
         # Add 1 click for today
         self.advertisement1.incr(CLICKS, self.publisher)
 
-        # Refresh the data on the include_flight - gets the denormalized views
-        self.include_flight.refresh_from_db()
+        # Refresh the denormalized totals manually since they're no longer updated in real-time
+        self.include_flight.refresh_denormalized_totals()
 
         self.assertEqual(self.include_flight.clicks_remaining(), 997)
         self.assertEqual(self.include_flight.clicks_today(), 1)
@@ -466,8 +466,8 @@ def test_clicks_needed(self):
         for _ in range(clicks_to_simulate):
             self.advertisement1.incr(CLICKS, self.publisher)
 
-        # Refresh the data on the include_flight - gets the denormalized views
-        self.include_flight.refresh_from_db()
+        # Refresh the denormalized totals manually since they're no longer updated in real-time
+        self.include_flight.refresh_denormalized_totals()
 
         self.assertEqual(self.include_flight.clicks_needed_this_interval(), 23)
 
@@ -491,8 +491,8 @@ def test_views_needed(self):
         for _ in range(views_to_simulate):
             self.advertisement1.incr(VIEWS, self.publisher)
 
-        # Refresh the data on the include_flight - gets the denormalized views
-        self.cpm_flight.refresh_from_db()
+        # Refresh the denormalized totals manually since they're no longer updated in real-time
+        self.cpm_flight.refresh_denormalized_totals()
 
         self.assertEqual(self.cpm_flight.views_needed_this_interval(), 313)
 

adserver/tests/test_health_endpoints.py

@@ -0,0 +1,57 @@
+"""Tests for health check endpoints."""
+
+from datetime import timedelta
+
+from django.core.cache import cache
+from django.test import TestCase
+from django.urls import reverse
+from django.utils import timezone
+
+
+class FlightTotalsHealthTest(TestCase):
+    """Tests for the flight totals health check endpoint."""
+
+    def setUp(self):
+        """Clear cache before each test."""
+        cache.clear()
+
+    def test_health_check_no_cache(self):
+        """Test health check returns 503 when task has never run."""
+        response = self.client.get(reverse("health-flight-totals"))
+        self.assertEqual(response.status_code, 503)
+        data = response.json()
+        self.assertEqual(data["status"], "error")
+        self.assertIn("never run", data["message"])
+
+    def test_health_check_recent_run(self):
+        """Test health check returns 200 when task ran recently."""
+        # Set cache to indicate task ran 5 minutes ago
+        cache.set("flight_totals_last_refresh", timezone.now().isoformat())
+
+        response = self.client.get(reverse("health-flight-totals"))
+        self.assertEqual(response.status_code, 200)
+        data = response.json()
+        self.assertEqual(data["status"], "ok")
+        self.assertLessEqual(data["minutes_since_refresh"], 1)
+
+    def test_health_check_stale_run(self):
+        """Test health check returns 503 when task hasn't run in a while."""
+        # Set cache to indicate task ran 30 minutes ago (stale)
+        stale_time = timezone.now() - timedelta(minutes=30)
+        cache.set("flight_totals_last_refresh", stale_time.isoformat())
+
+        response = self.client.get(reverse("health-flight-totals"))
+        self.assertEqual(response.status_code, 503)
+        data = response.json()
+        self.assertEqual(data["status"], "error")
+        self.assertGreater(data["minutes_since_refresh"], 20)
+
+    def test_health_check_invalid_timestamp(self):
+        """Test health check returns 503 when cache contains invalid data."""
+        cache.set("flight_totals_last_refresh", "invalid-timestamp")
+
+        response = self.client.get(reverse("health-flight-totals"))
+        self.assertEqual(response.status_code, 503)
+        data = response.json()
+        self.assertEqual(data["status"], "error")
+        self.assertIn("Invalid timestamp", data["message"])

adserver/tests/test_models.py

@@ -522,6 +522,8 @@ def test_campaign_totals(self):
         self.ad1.incr(CLICKS, self.publisher)
         self.ad1.incr(CLICKS, self.publisher)
 
+        self.flight.refresh_denormalized_totals()
+
         self.assertAlmostEqual(self.campaign.total_value(), 4.0)
 
         cpm_flight = get(
@@ -551,6 +553,10 @@ def test_campaign_totals(self):
         ad2.incr(VIEWS, self.publisher)
         ad2.incr(VIEWS, self.publisher)
 
+        # Refresh denormalized totals for both flights
+        self.flight.refresh_denormalized_totals()
+        cpm_flight.refresh_denormalized_totals()
+
         self.assertAlmostEqual(self.campaign.total_value(), 4.3)
 
     def test_flight_state(self):
@@ -576,12 +582,12 @@ def test_flight_value_remaining(self):
         self.flight.save()
         self.assertAlmostEqual(self.flight.value_remaining(), 100 * 2)
 
-        # Each click is worth $2
+        # Each click is $2
         self.ad1.incr(CLICKS, self.publisher)
         self.ad1.incr(CLICKS, self.publisher)
         self.ad1.incr(CLICKS, self.publisher)
 
-        self.flight.refresh_from_db()
+        self.flight.refresh_denormalized_totals()
         self.assertAlmostEqual(self.flight.value_remaining(), 97 * 2)
 
         self.flight.cpm = 50.0
@@ -596,7 +602,7 @@ def test_flight_value_remaining(self):
         for _ in range(25):
             self.ad1.incr(VIEWS, self.publisher)
 
-        self.flight.refresh_from_db()
+        self.flight.refresh_denormalized_totals()
         self.assertAlmostEqual(self.flight.value_remaining(), 5.0 - (25 * 0.05))
 
     def test_projected_total_value(self):
@@ -607,7 +613,7 @@ def test_projected_total_value(self):
         self.ad1.incr(CLICKS, self.publisher)
         self.ad1.incr(CLICKS, self.publisher)
 
-        self.flight.refresh_from_db()
+        self.flight.refresh_denormalized_totals()
         self.assertAlmostEqual(self.flight.projected_total_value(), 1000 * 2)
 
         self.flight.cpm = 50.0
@@ -727,7 +733,7 @@ def test_refund(self):
         for click in (click1, click2, click3):
             self.assertIsNotNone(click)
 
-        self.flight.refresh_from_db()
+        self.flight.refresh_denormalized_totals()
 
         self.assertEqual(self.flight.total_clicks, 3)
 
@@ -760,7 +766,7 @@ def test_refund(self):
             self.assertTrue(offer.clicked)
 
         # Reload data from the DB
-        self.flight.refresh_from_db()
+        self.flight.refresh_denormalized_totals()
         impression.refresh_from_db()
 
         self.assertEqual(self.flight.total_clicks, 1)

adserver/urls.py

@@ -67,6 +67,7 @@
 from .views import dashboard
 from .views import do_not_track
 from .views import do_not_track_policy
+from .views import flight_totals_health
 
 
 urlpatterns = [
@@ -82,6 +83,12 @@
     # Do not Track
     path(r".well-known/dnt/", do_not_track, name="dnt-status"),
     path(r".well-known/dnt-policy.txt", do_not_track_policy, name="dnt-policy"),
+    # Health checks
+    path(
+        r"health/flight-totals/",
+        flight_totals_health,
+        name="health-flight-totals",
+    ),
     # Ad API
     path(r"api/v1/", include("adserver.api.urls")),
     # Staff interface