ref(typing): Typing for the low priority symbolication queue (#29017)

Also removes a method that is effectively an alias for
an existing one without providing much benefit.

Co-authored-by: Betty Da <bda@sentry.io>

mypy.ini

@@ -54,6 +54,7 @@ files = src/sentry/api/bases/external_actor.py,
         src/sentry/snuba/query_subscription_consumer.py,
         src/sentry/spans/**/*.py,
         src/sentry/tasks/app_store_connect.py,
+        src/sentry/tasks/low_priority_symbolication.py,
         src/sentry/tasks/update_user_reports.py,
         src/sentry/unmerge.py,
         src/sentry/utils/appleconnect/,

src/sentry/processing/realtime_metrics/__init__.py

@@ -1,13 +1,28 @@
+from typing import TYPE_CHECKING
+
 from django.conf import settings
 
 from sentry.utils.services import LazyServiceWrapper
 
 from .base import RealtimeMetricsStore
 
-realtime_metrics_store = LazyServiceWrapper(
+realtime_metrics_store: RealtimeMetricsStore = LazyServiceWrapper(
     RealtimeMetricsStore,
     settings.SENTRY_REALTIME_METRICS_BACKEND,
     settings.SENTRY_REALTIME_METRICS_OPTIONS,
 )
 
 realtime_metrics_store.expose(locals())
+
+if TYPE_CHECKING:
+    # This is all too dynamic for mypy, so manually set the same attributes from
+    # RealtimeMetricsStore.__all__:
+    validate = realtime_metrics_store.validate
+    increment_project_event_counter = realtime_metrics_store.increment_project_event_counter
+    increment_project_duration_counter = realtime_metrics_store.increment_project_duration_counter
+    projects = realtime_metrics_store.projects
+    get_counts_for_project = realtime_metrics_store.get_counts_for_project
+    get_durations_for_project = realtime_metrics_store.get_durations_for_project
+    get_lpq_projects = realtime_metrics_store.get_lpq_projects
+    add_project_to_lpq = realtime_metrics_store.add_project_to_lpq
+    remove_projects_from_lpq = realtime_metrics_store.remove_projects_from_lpq

src/sentry/processing/realtime_metrics/base.py

@@ -37,9 +37,12 @@ class RealtimeMetricsStore(Service):  # type: ignore
     """A service for storing metrics about incoming requests within a given time window."""
 
     __all__ = (
+        "validate",
         "increment_project_event_counter",
         "increment_project_duration_counter",
-        "validate",
+        "projects",
+        "get_counts_for_project",
+        "get_durations_for_project",
         "get_lpq_projects",
         "add_project_to_lpq",
         "remove_projects_from_lpq",
@@ -106,17 +109,6 @@ class RealtimeMetricsStore(Service):  # type: ignore
         """
         raise NotImplementedError
 
-    def remove_project_from_lpq(self, project_id: int) -> bool:
-        """
-        Removes a project from the low priority queue.
-
-        This registers an intent to restore all specified projects back to the regular queue.
-
-        Returns True if the project was assigned to the queue prior to its removal. Returns False if
-        it wasn't assigned to the queue to begin with.
-        """
-        raise NotImplementedError
-
     def remove_projects_from_lpq(self, project_ids: Set[int]) -> int:
         """
         Removes projects from the low priority queue.

src/sentry/processing/realtime_metrics/redis.py

@@ -208,20 +208,6 @@ class RedisRealtimeMetricsStore(base.RealtimeMetricsStore):
         # expected to be in the set.
         return int(self.cluster.sadd(LPQ_MEMBERS_KEY, project_id)) > 0
 
-    def remove_project_from_lpq(self, project_id: int) -> bool:
-        """
-        Removes a project from the low priority queue.
-
-        This restores the specified project back to the regular queue, unless it has been
-        manually forced into the low priority queue via the `store.symbolicate-event-lpq-always`
-        kill switch.
-
-        This may throw an exception if there is some sort of issue deregistering the projects from
-        the queue.
-        """
-
-        return self.remove_projects_from_lpq({project_id}) > 0
-
     def remove_projects_from_lpq(self, project_ids: Set[int]) -> int:
         """
         Removes projects from the low priority queue.

src/sentry/tasks/low_priority_symbolication.py

@@ -9,20 +9,21 @@ This has three major tasks, executed in the following general order:
 """
 
 import logging
+from typing import Iterable
 
-from sentry.processing.realtime_metrics import realtime_metrics_store
+from sentry.processing import realtime_metrics
 from sentry.processing.realtime_metrics.base import BucketedCount, DurationHistogram
 from sentry.tasks.base import instrumented_task
 
 logger = logging.getLogger(__name__)
 
 
-@instrumented_task(
+@instrumented_task(  # type: ignore
     name="sentry.tasks.low_priority_symbolication.scan_for_suspect_projects",
     queue="symbolications.compute_low_priority_projects",
     ignore_result=True,
     soft_time_limit=10,
-)  # type: ignore
+)
 def scan_for_suspect_projects() -> None:
     """Scans and updates the list of projects assigned to the low priority queue."""
     _scan_for_suspect_projects()
@@ -31,37 +32,31 @@ def scan_for_suspect_projects() -> None:
 def _scan_for_suspect_projects() -> None:
     suspect_projects = set()
 
-    for project_id in realtime_metrics_store.projects():
+    for project_id in realtime_metrics.projects():
         suspect_projects.add(project_id)
         update_lpq_eligibility(project_id).apply_async()
 
     # Prune projects we definitely know shouldn't be in the queue any more.
     # `update_lpq_eligibility` should handle removing suspect projects from the list if it turns
     # out they need to be evicted.
-    current_lpq_projects = realtime_metrics_store.get_lpq_projects() or set()
+    current_lpq_projects = realtime_metrics.get_lpq_projects() or set()
     deleted_projects = current_lpq_projects.difference(suspect_projects)
     if len(deleted_projects) == 0:
         return
 
-    removed = realtime_metrics_store.remove_projects_from_lpq(deleted_projects)
+    realtime_metrics.remove_projects_from_lpq(deleted_projects)
 
-    if len(removed) > 0:
-        for project_id in removed:
-            logger.warning("Moved project out of symbolicator's low priority queue: %s", project_id)
-
-    not_removed = deleted_projects.difference(removed)
-    if len(not_removed) > 0:
-        logger.warning(
-            "Failed to move project(s) out of symbolicator's low priority queue: %s", not_removed
-        )
+    for project_id in deleted_projects:
+        # TODO: add metrics!
+        logger.warning("Moved project out of symbolicator's low priority queue: %s", project_id)
 
 
-@instrumented_task(
+@instrumented_task(  # type: ignore
     name="sentry.tasks.low_priority_symbolication.update_lpq_eligibility",
     queue="symbolications.compute_low_priority_projects",
     ignore_result=True,
     soft_time_limit=10,
-)  # type: ignore
+)
 def update_lpq_eligibility(project_id: int) -> None:
     """
     Given a project ID, determines whether the project belongs in the low priority queue and
@@ -71,20 +66,22 @@ def update_lpq_eligibility(project_id: int) -> None:
 
 
 def _update_lpq_eligibility(project_id: int) -> None:
-    counts = realtime_metrics_store.get_counts_for_project(project_id)
-    durations = realtime_metrics_store.get_durations_for_project(project_id)
+    counts = realtime_metrics.get_counts_for_project(project_id)
+    durations = realtime_metrics.get_durations_for_project(project_id)
 
     is_eligible = calculation_magic(counts, durations)
 
     if is_eligible:
-        was_added = realtime_metrics_store.add_project_to_lpq(project_id)
+        was_added = realtime_metrics.add_project_to_lpq(project_id)
         if was_added:
             logger.warning("Moved project to symbolicator's low priority queue: %s", project_id)
     elif not is_eligible:
-        was_removed = realtime_metrics_store.remove_project_from_lpq(project_id)
+        was_removed = realtime_metrics.remove_projects_from_lpq({project_id})
         if was_removed:
             logger.warning("Moved project out of symbolicator's low priority queue: %s", project_id)
 
 
-def calculation_magic(invocations: BucketedCount, durations: DurationHistogram) -> bool:
+def calculation_magic(
+    invocations: Iterable[BucketedCount], durations: Iterable[DurationHistogram]
+) -> bool:
     return False

tests/sentry/processing/realtime_metrics/test_redis.py

@@ -4,13 +4,13 @@ from typing import TYPE_CHECKING, Any, Dict
 
 import pytest
 
-from sentry.processing import realtime_metrics  # type: ignore
-from sentry.processing.realtime_metrics.base import (  # type: ignore
+from sentry.processing import realtime_metrics
+from sentry.processing.realtime_metrics.base import (
     BucketedCount,
     BucketedDurations,
     DurationHistogram,
 )
-from sentry.processing.realtime_metrics.redis import RedisRealtimeMetricsStore  # type: ignore
+from sentry.processing.realtime_metrics.redis import RedisRealtimeMetricsStore
 from sentry.utils import redis
 
 if TYPE_CHECKING:
@@ -289,64 +289,13 @@ def test_remove_projects_from_lpq_no_members(
 ) -> None:
     redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1)
 
-    removed = store.remove_projects_from_lpq({})
+    removed = store.remove_projects_from_lpq(set())
     assert removed == 0
 
     remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected")
     assert remaining == {"1"}
 
 
-#
-# remove_project_from_lpq()
-# This literally invokes remove_projects_from_lpq so bare bones tests should be enough
-
-
-def test_remove_project_from_lpq_unset(
-    store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster
-) -> None:
-    removed = store.remove_project_from_lpq(1)
-    assert not removed
-
-    remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected")
-    assert remaining == set()
-
-
-def test_remove_project_from_lpq_empty(
-    store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster
-) -> None:
-    redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1)
-    redis_cluster.srem("store.symbolicate-event-lpq-selected", 1)
-
-    removed = store.remove_project_from_lpq(1)
-    assert not removed
-    remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected")
-    assert remaining == set()
-
-
-def test_remove_project_from_lpq_only_member(
-    store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster
-) -> None:
-    redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1)
-
-    removed = store.remove_project_from_lpq(1)
-    assert removed
-
-    remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected")
-    assert remaining == set()
-
-
-def test_remove_project_from_lpq_nonmember(
-    store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster
-) -> None:
-    redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11)
-
-    removed = store.remove_project_from_lpq(1)
-    assert not removed
-
-    remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected")
-    assert remaining == {"11"}
-
-
 #
 # projects()
 #