Hotfix: disable PageCounter, fix dashboard 502 / client IP / zip tooltip / filters banner (#11921)

Hotfix based on master, per ENG-12193 plus the download-dashboard production issues.

  ### Disable PageCounter (ENG-12193)
  `update_counter` returns early — no session load, no `select_for_update` row lock, no writes; `DownloadCountReporter` unplugged from the daily reporters. Reads keep serving the stored (frozen) numbers; `DailyDownloadCountReport` index/API unchanged. No migrations.

  ### Dashboard 502 on wide ranges
  The changelist feeds the dashboard an ordered queryset and Django folds ordering columns into the GROUP BY of `values().annotate()` — every breakdown returned ~one row per event (measured: 5k events → 5k rows from one breakdown; fixed → 3). Also: one aggregate pass instead of twelve scans, single ChangeList per
  view instead of two, no full-table COUNT(*), Outcome sort via order_field expression.

  ### IP column recorded the load balancer
  New `get_client_ip()` resolves the client from `X-Forwarded-For` right-to-left past infrastructure hops; deployment-specific LB addresses go in the new `OSF_TRUSTED_PROXY_CIDRS` env (devops: set the LB public address, e.g. `35.190.55.96/32`). Old rows can't be backfilled.

  ### ENG-12136 / ENG-12137
  Zip-filter doughnut tooltip now hit-tests the arc under the cursor (was resolving to the invisible zero slice and showing nothing). Applied-filters banner is derived from the ChangeList's filter specs — same labels as the panel ('Yes', 'Single file'), and future filters appear automatically.

  ### Testing
  286 passed / 8 skipped across dashboard, telemetry, analytics, osfstorage and addons suites on this combined branch. Metrics suites verified identical to a pristine master baseline (pre-existing failures there are unrelated). `makemigrations --check` and `manage.py check` clean. No schema changes anywhere.

Author: sh-andriy

addons/base/views.py

@@ -58,6 +58,7 @@
 from osf.utils import permissions
 from osf.utils.download_telemetry import (
     classify_download_channel,
+    get_client_ip,
     never_breaks_downloads,
     record_download,
 )
@@ -221,7 +222,9 @@ def _record_file_download(target, file_node, query_params, auth, version=None):
         version_identifier=getattr(version, 'identifier', None),
         storage_provider=getattr(file_node, 'provider', '') or '',
         user_guid=getattr(getattr(auth, 'user', None), '_id', None),
-        ip=request.remote_addr,
+        # remote_addr behind the load balancer is the balancer, not the client --
+        # the client is recovered from X-Forwarded-For
+        ip=get_client_ip(request.remote_addr, request.headers.get('X-Forwarded-For', '')),
         user_agent=request.headers.get('User-Agent', ''),
         source_area=source_area,
         download_channel=classify_download_channel(source_area, is_api_token=is_api_token),

addons/osfstorage/tests/test_models.py

@@ -184,6 +184,7 @@ def test_download_count_file_defaults(self):
         assert child.get_download_count() == 0
 
     def test_download_count_file(self):
+        # PageCounter is disabled (ENG-12193), so legacy counts stay at zero
         s = SessionStore()
         s.create()
         child = self.node_settings.get_root().append_file('Test')
@@ -192,10 +193,10 @@ def test_download_count_file(self):
         utils.update_analytics(self.project, child, 1, s.session_key)
         utils.update_analytics(self.project, child, 2, s.session_key)
 
-        assert child.get_download_count() == 3
-        assert child.get_download_count(0) == 1
-        assert child.get_download_count(1) == 1
-        assert child.get_download_count(2) == 1
+        assert child.get_download_count() == 0
+        assert child.get_download_count(0) == 0
+        assert child.get_download_count(1) == 0
+        assert child.get_download_count(2) == 0
 
     def test_create_version_locks_file_row(self):
 

addons/osfstorage/tests/test_utils.py

@@ -27,6 +27,7 @@ def setUp(self):
         self.record.save()
 
     def test_serialize_revision(self):
+        # PageCounter is disabled (ENG-12193), so legacy counts stay at zero
         s = SessionStore()
         s.create()
         utils.update_analytics(self.project, self.record, 0, s.session_key)
@@ -39,7 +40,7 @@ def test_serialize_revision(self):
                 'url': self.user.url,
             },
             'date': self.versions[0].created.isoformat(),
-            'downloads': 2,
+            'downloads': 0,
             'md5': None,
             'sha256': None,
         }
@@ -50,9 +51,9 @@ def test_serialize_revision(self):
             0,
         )
         assert expected == observed
-        assert self.record.get_download_count() == 3
-        assert self.record.get_download_count(version=2) == 1
-        assert self.record.get_download_count(version=0) == 2
+        assert self.record.get_download_count() == 0
+        assert self.record.get_download_count(version=2) == 0
+        assert self.record.get_download_count(version=0) == 0
 
     def test_anon_revisions(self):
         s = SessionStore()

admin/templates/download_events/download_events.html

@@ -652,13 +652,18 @@ <h4 style="margin:0 0 12px;color:#aaa;text-align:center;">
 
 // A zero slice ("File" or "Zip") has no arc, so don't pop a "File 0" / "Zip 0"
 // tooltip for it — that reads as a bug when a filter zeroes one side out.
+// The tooltip must also hit-test the arc actually under the cursor
+// (nearest + intersect): the "index" mode inherited from commonOptions resolved
+// hovers to the invisible zero slice, so filtering to zips only showed no
+// tooltip at all instead of the Zip count/GB.
 const doughnutOptions = {
     ...commonOptions,
     cutout: "60%",
     plugins: {
         ...commonOptions.plugins,
         tooltip: {
-            ...commonOptions.plugins.tooltip,
+            mode: "nearest",
+            intersect: true,
             filter: (item) => item.parsed !== 0
         }
     }

osf/admin.py

@@ -415,6 +415,22 @@ def user(self, obj):
 # threshold is what separates a real failure from a cancel.
 DOWNLOAD_FAILURE_MIN_STATUS = 400
 
+# Sort key for the computed Outcome column: Completed, Cancelled, Failed, then single
+# files (which have no outcome). Attached via ``admin_order_field``, so the CASE only
+# runs when someone actually sorts by Outcome -- annotating it onto every queryset made
+# all the dashboard aggregates and counts pay for it on every page load.
+OUTCOME_SORT_ORDER = Case(
+    When(zip_completed=True, then=Value(0)),  # Completed
+    When(
+        zip_completed=False,
+        status_code__gte=DOWNLOAD_FAILURE_MIN_STATUS,
+        then=Value(2),  # Failed
+    ),
+    When(zip_completed=False, then=Value(1)),  # Cancelled
+    default=Value(3),  # single files have no outcome ('—')
+    output_field=IntegerField(),
+)
+
 
 class DownloadOutcomeFilter(SimpleListFilter):
     """Filter zips by how they ended: completed, cancelled mid-stream, or failed."""
@@ -552,27 +568,10 @@ class DownloadEventsView(admin.ModelAdmin):
         'user_agent'
     )
     search_help_text = 'Search by username, full name, user or node guid, ip, path, storage provider, user or storage region, source area, user agent.'
-
-    def get_queryset(self, request):
-        """Annotate an outcome rank so the computed Outcome column is sortable.
-
-        The rank mirrors :meth:`outcome` exactly. It's just an ordering key — it doesn't
-        change what rows are returned, so the table and the dashboard aggregates are
-        unaffected.
-        """
-        return super().get_queryset(request).annotate(
-            _outcome_rank=Case(
-                When(zip_completed=True, then=Value(0)),  # Completed
-                When(
-                    zip_completed=False,
-                    status_code__gte=DOWNLOAD_FAILURE_MIN_STATUS,
-                    then=Value(2),  # Failed
-                ),
-                When(zip_completed=False, then=Value(1)),  # Cancelled
-                default=Value(3),  # single files have no outcome ('—')
-                output_field=IntegerField(),
-            )
-        )
+    # Without this the admin runs a COUNT(*) over the ENTIRE table on every page load,
+    # just to print "n total" next to the filtered count. At production volume that's a
+    # full-table scan per view.
+    show_full_result_count = False
 
     @admin.display(description='User', ordering='user__username')
     def user_display(self, obj):
@@ -593,7 +592,7 @@ def user_agent_display(self, obj):
             return '—'
         return obj.user_agent if len(obj.user_agent) <= 80 else obj.user_agent[:79] + '…'
 
-    @admin.display(description='Outcome', ordering='_outcome_rank')
+    @admin.display(description='Outcome', ordering=OUTCOME_SORT_ORDER)
     def outcome(self, obj):
         """Human-readable end state. Single files have no outcome — they're recorded at the
         redirect before any bytes move, so they never report completion."""
@@ -630,33 +629,38 @@ def changelist_view(self, request, extra_context=None):
 
         if extra_context is None:
             extra_context = {}
-        changelist = self.get_changelist_instance(request)
-        extra_context['download_events_dashboard'] = self.get_dashboard_data(changelist.get_queryset(request))
-        extra_context['download_events_active_filters'] = self._active_filters(request)
-        return super().changelist_view(request, extra_context=extra_context)
-
-    # query-string param -> human label, for the "applied filters" banner above the charts
-    FILTER_LABELS = (
-        ('q', 'Search'),
-        ('project_guid', 'Project'),
-        ('download_user', 'User'),
-        ('download_type', 'Download type'),
-        ('outcome', 'Outcome'),
-        ('zip_completed__exact', 'Zip completed'),
-        ('storage_provider', 'Storage provider'),
-    )
-
-    def _active_filters(self, request):
+        response = super().changelist_view(request, extra_context=extra_context)
+
+        # Feed the charts from the ChangeList the admin just built rather than
+        # constructing a second one -- every ChangeList runs the whole
+        # filter/count/results pipeline in __init__, so two of them doubled all of
+        # that work per page view. On non-changelist responses (e.g. the redirect
+        # the admin issues for a bad filter value) there is no `cl` and the charts
+        # are simply skipped, where the old code 500ed.
+        context_data = getattr(response, 'context_data', None) or {}
+        changelist = context_data.get('cl')
+        if changelist is not None:
+            response.context_data['download_events_dashboard'] = self.get_dashboard_data(changelist.queryset)
+            response.context_data['download_events_active_filters'] = self._active_filters(request, changelist)
+        return response
+
+    def _active_filters(self, request, changelist):
         """The filters/search currently in effect, as ``[{'label', 'value'}]``, so the
         dashboard can show at a glance what its numbers are scoped to.
 
-        Purely presentational — it reads the same query string the changelist already
-        filtered on; it never changes what's queried.
+        Read off the ChangeList's own filter specs, so the banner shows exactly what
+        the filter panel shows -- same titles, same choice labels ('Yes', not '1';
+        'Single file', not 'file') -- and a filter added to ``list_filter`` shows up
+        here on its own. A hand-kept param map drifted: it missed the ``__exact``
+        params Django uses for choice fields and never learned about new filters.
+
+        Purely presentational; it never changes what's queried.
         """
         params = request.GET
         active = []
 
-        # the date range arrives in date+time halves; recombine them into one readable line
+        # The date range renders as a form, not choices, so its spec has nothing to
+        # report -- recombine the date+time halves from the query string instead.
         date_from = ' '.join(
             part for part in (params.get('created__range__gte_0'), params.get('created__range__gte_1')) if part
         )
@@ -666,10 +670,31 @@ def _active_filters(self, request):
         if date_from or date_to:
             active.append({'label': 'Date (UTC)', 'value': f"{date_from or '…'} → {date_to or 'now'}"})
 
-        for param, label in self.FILTER_LABELS:
-            value = params.get(param)
-            if value:
-                active.append({'label': label, 'value': value})
+        # the search box isn't a filter spec either
+        if params.get('q'):
+            active.append({'label': 'Search', 'value': params['q']})
+
+        for spec in changelist.filter_specs:
+            title = str(spec.title)
+            label = title[:1].upper() + title[1:]
+            if isinstance(spec, InputFilter):
+                # free-text filters override choices() for their form, so read the
+                # typed value directly
+                if spec.value():
+                    active.append({'label': label, 'value': spec.value()})
+                continue
+            try:
+                choices = list(spec.choices(changelist))
+            except Exception:
+                # a spec that can't enumerate choices (form-based filters) simply
+                # doesn't contribute a chip; never let the banner break the page
+                continue
+            active.extend(
+                {'label': label, 'value': str(choice.get('display', ''))}
+                # the first choice is always "All" -- selected means the filter is off
+                for choice in choices[1:]
+                if choice.get('selected') and choice.get('display')
+            )
         return active
 
     def _in_dashboard_group(self, request):
@@ -713,9 +738,6 @@ def has_delete_permission(self, request, obj=None):
     # genuinely terabyte-scale, so small numbers aren't cluttered with "(0.0 TB)".
     GB_PER_TB = 1024
 
-    def _sum_bytes(self, queryset):
-        return queryset.aggregate(total_bytes=Sum('size_bytes'))['total_bytes'] or 0
-
     def _to_gb(self, total_bytes):
         return round((total_bytes or 0) / (1024**3), 2)
 
@@ -736,15 +758,39 @@ def _percent(self, part, whole):
         return round(part * 100 / whole, 2)
 
     def get_dashboard_data(self, queryset):
-        file_queryset = queryset.filter(download_type=DownloadEvent.FILE)
-        zip_queryset = queryset.exclude(download_type=DownloadEvent.FILE)
-        total_file_downloads = file_queryset.count()
-        total_zip_downloads = zip_queryset.count()
-        total_bytes = self._sum_bytes(queryset)
-
-        total_downloads = queryset.count()
-        total_file_gb = self._to_gb(self._sum_bytes(file_queryset))
-        total_zip_gb = self._to_gb(self._sum_bytes(zip_queryset))
+        # The changelist hands this queryset over ordered by `created`, and Django folds
+        # ordering columns into the GROUP BY of values().annotate(). Since `created` is
+        # near-unique, that exploded every breakdown below into one row per EVENT rather
+        # than one per group -- harmless on an hour of data, a timeout on a day of it.
+        # Aggregates have no use for an ordering; drop it before anything else runs.
+        queryset = queryset.order_by()
+
+        is_file = Q(download_type=DownloadEvent.FILE)
+        # Every headline number in one pass over the range -- these were twelve separate
+        # scans of the same rows before.
+        totals = queryset.aggregate(
+            total_downloads=Count('id'),
+            total_bytes=Sum('size_bytes'),
+            file_downloads=Count('id', filter=is_file),
+            file_bytes=Sum('size_bytes', filter=is_file),
+            zip_downloads=Count('id', filter=~is_file),
+            zip_bytes=Sum('size_bytes', filter=~is_file),
+            # Zip outcomes. Single files are recorded before any bytes move, so they
+            # have no outcome and are left out of these entirely.
+            completed_zips=Count('id', filter=~is_file & Q(zip_completed=True)),
+            incomplete_zips=Count('id', filter=~is_file & Q(zip_completed=False)),
+            failed_zips=Count('id', filter=~is_file & Q(
+                zip_completed=False, status_code__gte=DOWNLOAD_FAILURE_MIN_STATUS)),
+            # distinct non-null users; Count skips NULLs, so anonymous rows don't count
+            unique_users=Count('user_id', distinct=True),
+        )
+        total_downloads = totals['total_downloads']
+        total_file_downloads = totals['file_downloads']
+        total_zip_downloads = totals['zip_downloads']
+        failed_zips = totals['failed_zips']
+        total_file_gb = self._to_gb(totals['file_bytes'])
+        total_zip_gb = self._to_gb(totals['zip_bytes'])
+
         time_series = self._build_time_series(queryset)
         # each breakdown returns the ranked *known* regions plus, separately, the
         # "Unknown" bucket — so a large Unknown doesn't crowd real regions off the chart
@@ -753,20 +799,14 @@ def get_dashboard_data(self, queryset):
         # downloads and GB grouped by where the bytes came from (osfstorage vs addons)
         storage_providers, storage_providers_unknown = self._build_region_breakdown(queryset, 'storage_provider')
 
-        # Zip outcomes. Single files are recorded before any bytes move, so they have no
-        # outcome and are left out of this breakdown entirely.
-        completed_zips = zip_queryset.filter(zip_completed=True).count()
-        failed_zips = zip_queryset.filter(
-            zip_completed=False, status_code__gte=DOWNLOAD_FAILURE_MIN_STATUS).count()
-        incomplete_zips = zip_queryset.filter(zip_completed=False).count()
         zip_outcomes = {
-            'completed': completed_zips,
+            'completed': totals['completed_zips'],
             # everything that didn't complete and wasn't a server failure is a user cancel
-            'cancelled': incomplete_zips - failed_zips,
+            'cancelled': totals['incomplete_zips'] - failed_zips,
             'failed': failed_zips,
         }
 
-        total_gb = self._to_gb(total_bytes)
+        total_gb = self._to_gb(totals['total_bytes'])
         split = {
             'file': {
                 'count': total_file_downloads,
@@ -789,7 +829,7 @@ def get_dashboard_data(self, queryset):
                 'total_downloads': total_downloads,
                 'total_gb': total_gb,
                 'total_gb_tb_suffix': self._tb_suffix(total_gb),
-                'unique_users': queryset.exclude(user_id__isnull=True).values('user_id').distinct().count(),
+                'unique_users': totals['unique_users'],
                 'failed_zips': failed_zips,
             },
             'split': split,
@@ -970,8 +1010,8 @@ def _build_channel_breakdown(self, queryset):
             total_bytes=Sum('size_bytes'),
         )
         labels = dict(DownloadEvent.DOWNLOAD_CHANNELS)
-        # Fold by channel in Python: a pre-existing annotation on the queryset (e.g. the
-        # sort's _outcome_rank) can leak into the GROUP BY and split a channel across rows,
+        # Fold by channel in Python: anything that sneaks extra columns into the GROUP BY
+        # (an inherited ordering, a stray annotation) would split a channel across rows,
         # so sum them back together — same reason _build_region_breakdown folds.
         folded = defaultdict(lambda: {'downloads': 0, 'bytes': 0})
         for row in rows:

osf/metrics/reporters/__init__.py

@@ -2,7 +2,8 @@
 
 # from .active_users import ActiveUserReporter
 from .storage_addon_usage import StorageAddonUsageReporter
-from .download_count import DownloadCountReporter
+# reads PageCounter, which is disabled (ENG-12193)
+# from .download_count import DownloadCountReporter
 from .institution_summary import InstitutionSummaryReporter
 from .institutional_users import InstitutionalUsersReporter
 from .institution_summary_monthly import InstitutionalSummaryMonthlyReporter
@@ -21,7 +22,7 @@
 
 class AllDailyReporters(enum.Enum):
     # ACTIVE_USER = ActiveUserReporter
-    DOWNLOAD_COUNT = DownloadCountReporter
+    # DOWNLOAD_COUNT = DownloadCountReporter
     INSTITUTION_SUMMARY = InstitutionSummaryReporter
     NEW_USER_DOMAIN = NewUserDomainReporter
     NODE_COUNT = NodeCountReporter

osf/models/analytics.py

@@ -99,6 +99,11 @@ def clean_page(page):
 
     @classmethod
     def update_counter(cls, resource, file, version, action, node_info, session_key):
+        # ENG-12193: disabled. The metrics service owns these counts now, and the
+        # select_for_update below piles up on hot rows under heavy download traffic.
+        # Reads still work, numbers are frozen. Full removal is a follow-up.
+        return
+
         if version is not None:
             page = f'{action}:{resource._id}:{file._id}:{version}'
         else: