From 208736aaacc56e86fd05e85fa761c0c5422d1fc4 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 21 Sep 2021 20:35:29 -0400 Subject: [PATCH 01/16] feat(symbolicator): Add the ability to manipulate the automatic killswitch for the LPQ. --- .../processing/realtime_metrics/base.py | 44 +++- .../processing/realtime_metrics/redis.py | 80 +++++++ .../test_realtime_metrics_store.py | 214 ++++++++++++++++++ .../processing/realtime_metrics/test_redis.py | 147 ++++++++++++ 4 files changed, 484 insertions(+), 1 deletion(-) create mode 100644 tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py diff --git a/src/sentry/processing/realtime_metrics/base.py b/src/sentry/processing/realtime_metrics/base.py index 5e2b15f48d0f91..bcdcf306f6cfea 100644 --- a/src/sentry/processing/realtime_metrics/base.py +++ b/src/sentry/processing/realtime_metrics/base.py @@ -1,10 +1,19 @@ +from typing import Set + from sentry.utils.services import Service class RealtimeMetricsStore(Service): # type: ignore """A service for storing metrics about incoming requests within a given time window.""" - __all__ = ("increment_project_event_counter", "increment_project_duration_counter", "validate") + __all__ = ( + "increment_project_event_counter", + "increment_project_duration_counter", + "validate", + "get_lpq_projects", + "add_project_to_lpq", + "remove_projects_from_lpq", + ) def increment_project_event_counter(self, project_id: int, timestamp: int) -> None: """Increment the event counter for the given project_id. @@ -26,3 +35,36 @@ def increment_project_duration_counter( the time of the event in seconds since the UNIX epoch and "duration" the processing time in seconds. """ pass + + def get_lpq_projects(self) -> Set[int]: + """ + Fetches the list of projects that are currently using the low priority queue. + + Returns a list of project IDs. + """ + pass + + def add_project_to_lpq(self, project_id: int): + """ + Moves projects to the low priority queue. + + This forces all symbolication events triggered by the specified projects to be redirected to + the low priority queue, unless these projects have been manually excluded from the low + priority queue via the `store.symbolicate-event-lpq-never` kill switch. + + Raises ``LowPriorityQueueMembershipError`` if this fails to add the specified project to the + low priority queue. + """ + pass + + def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: + """ + Removes projects from the low priority queue. + + This restores all specified projects back to the regular queue, unless they have been + manually forced into the low priority queue via the store.symbolicate-event-lpq-always kill + switch. + + + Returns all projects that have been successfully removed from the low priority queue. + """ diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 03edfd52f8f0ef..a53b43ab905a67 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -1,10 +1,14 @@ import datetime +from typing import Optional, Set from sentry.exceptions import InvalidConfiguration from sentry.utils import redis from . import base +# redis key for entry storing current list of LPQ members +LPQ_MEMBERS_KEY = "store.symbolicate-event-lpq-selected" + class RedisRealtimeMetricsStore(base.RealtimeMetricsStore): """An implementation of RealtimeMetricsStore based on a Redis backend.""" @@ -80,3 +84,79 @@ def increment_project_duration_counter( pipeline.hincrby(key, duration, 1) pipeline.pexpire(key, self._histogram_ttl) pipeline.execute() + + # TODO: do these killswitch helpers belong here, or in a different class? LowPriorityQueueStore? + # This probably needs locking + def get_lpq_projects(self) -> Set[int]: + """ + Fetches the list of projects that are currently using the low priority queue. + + Returns a list of project IDs. + """ + # TODO: there's got to be a better way to do this instead of invoking _to_int twice + return { + _to_int(project_id) + for project_id in self.inner.smembers(LPQ_MEMBERS_KEY) + if _to_int(project_id) is not None + } + + def add_project_to_lpq(self, project_id: int): + """ + Moves projects to the low priority queue. + + This forces all symbolication events triggered by the specified projects to be redirected to + the low priority queue, unless these projects have been manually excluded from the low + priority queue via the store.symbolicate-event-lpq-never kill switch. + + Raises ``LowPriorityQueueMembershipError`` if this fails to add the specified project to the + low priority queue. + """ + + added = self.inner.sadd(LPQ_MEMBERS_KEY, project_id) + + # Looks like this might already be in the LPQ, or redis failed to add it. This is only a + # problem if redis failed to add it. + if added == 0 and not self.inner.sismember(LPQ_MEMBERS_KEY, project_id): + raise LowPriorityQueueMembershipError( + f"Failed to move project ID {project_id} to the low priority queue" + ) + + def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: + """ + Removes projects from the low priority queue. + + This restores all specified projects back to the regular queue, unless they have been + manually forced into the low priority queue via the store.symbolicate-event-lpq-always kill + switch. + + + Returns all projects that have been successfully removed from the low priority queue. + """ + if len(project_ids) == 0: + return set() + + removed = self.inner.srem(LPQ_MEMBERS_KEY, *project_ids) + + if removed == len(project_ids): + return project_ids + + # Looks like only a subset of the project IDs were removed from the LPQ list. + in_lpq = self.inner.smembers(LPQ_MEMBERS_KEY) + was_removed = project_ids.intersection(in_lpq) + + return was_removed + + +def _to_int(value: str or int) -> Optional[int]: + try: + return int(value) if value else None + except ValueError: + return None + + +class LowPriorityQueueMembershipError(Exception): + """ + Something went wrong while updating the list of projects designated for the low priority queue. + """ + + pass diff --git a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py new file mode 100644 index 00000000000000..d7490b704bbb43 --- /dev/null +++ b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py @@ -0,0 +1,214 @@ +import datetime +import time +from typing import TYPE_CHECKING + +import pytest + +from sentry.processing.realtime_metrics import base # type: ignore +from sentry.utils import redis + +if TYPE_CHECKING: + from typing import Callable, TypeVar + + # Declare fixture decorator to swallow the function, this isn't the actual type returned + # but the type is unusable as a function which is what matters. + F = TypeVar("F", bound=Callable[..., None]) + + def _fixture(func: F) -> F: + ... + + pytest.fixture = _fixture + + +@pytest.fixture +def redis_cluster() -> redis._RedisCluster: + return redis.redis_clusters.get("default") + + +@pytest.fixture +def store(redis_cluster: redis._RedisCluster) -> base.RealtimeMetricsStore: + return base.RealtimeMetricsStore( + redis_cluster, counter_bucket_size=10, counter_ttl=datetime.timedelta(milliseconds=400) + ) + + +def test_increment_project_event_counter( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + store.increment_project_event_counter(17, 1147) + counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") + assert counter == "1" + time.sleep(0.5) + counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") + assert counter is None + + +def test_increment_project_event_counter_twice( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + store.increment_project_event_counter(17, 1147) + time.sleep(0.2) + store.increment_project_event_counter(17, 1149) + counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") + assert counter == "2" + time.sleep(0.3) + # it should have expired by now + counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") + assert counter is None + + +def test_increment_project_event_counter_multiple( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + store.increment_project_event_counter(17, 1147) + store.increment_project_event_counter(17, 1152) + + assert redis_cluster.get("symbolicate_event_low_priority:17:1140") == "1" + assert redis_cluster.get("symbolicate_event_low_priority:17:1150") == "1" + + +def test_get_lpq_projects_unset(store: base.RealtimeMetricsStore) -> None: + in_lpq = store.get_lpq_projects() + assert in_lpq == set() + + +def test_get_lpq_projects_empty( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) + + in_lpq = store.get_lpq_projects() + assert in_lpq == set() + + +def test_get_lpq_projects_filled( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + in_lpq = store.get_lpq_projects() + assert in_lpq == set([1]) + + +def test_add_project_to_lpq_unset( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_lpq_empty( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_lpq_dupe( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_lpq_filled( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1", "11"} + + +def test_remove_projects_from_lpq_unset( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + removed = store.remove_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_empty( + store: base.RealtimeMetricsStore, 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_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_only_member( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + removed = store.remove_projects_from_lpq({1}) + assert removed == {1} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_nonmember( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"11"} + + +def test_remove_projects_from_lpq_subset( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1}) + assert removed == {1} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"11"} + + +def test_remove_projects_from_lpq_all_members( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1, 11}) + assert removed == {1, 11} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_no_members( + store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + removed = store.remove_projects_from_lpq({}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"1"} diff --git a/tests/sentry/processing/realtime_metrics/test_redis.py b/tests/sentry/processing/realtime_metrics/test_redis.py index fe510ddafe29ef..c1d3f98efe80bb 100644 --- a/tests/sentry/processing/realtime_metrics/test_redis.py +++ b/tests/sentry/processing/realtime_metrics/test_redis.py @@ -112,3 +112,150 @@ def test_increment_project_duration_counter_different_buckets( assert redis_cluster.hget("symbolicate_event_low_priority:histogram:10:17:1140", "20") == "1" assert redis_cluster.hget("symbolicate_event_low_priority:histogram:10:17:1150", "40") == "1" + + +def test_get_lpq_projects_unset(store: RedisRealtimeMetricsStore) -> None: + in_lpq = store.get_lpq_projects() + assert in_lpq == set() + + +def test_get_lpq_projects_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) + + in_lpq = store.get_lpq_projects() + assert in_lpq == set() + + +def test_get_lpq_projects_filled( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + in_lpq = store.get_lpq_projects() + assert in_lpq == set([1]) + + +def test_add_project_to_lpq_unset( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_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) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_lpq_dupe( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1"} + + +def test_add_project_to_lpq_filled( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + assert store.add_project_to_lpq(1) + in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert in_lpq == {"1", "11"} + + +def test_remove_projects_from_lpq_unset( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + removed = store.remove_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_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_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_only_member( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + removed = store.remove_projects_from_lpq({1}) + assert removed == {1} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_nonmember( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"11"} + + +def test_remove_projects_from_lpq_subset( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1}) + assert removed == {1} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"11"} + + +def test_remove_projects_from_lpq_all_members( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) + + removed = store.remove_projects_from_lpq({1, 11}) + assert removed == {1, 11} + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == set() + + +def test_remove_projects_from_lpq_no_members( + store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster +) -> None: + redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) + + removed = store.remove_projects_from_lpq({}) + assert removed == set() + + remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") + assert remaining == {"1"} From f9c51b5e83294e5e855fd00e3e5e61025f98c7d5 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Wed, 22 Sep 2021 01:35:03 +0000 Subject: [PATCH 02/16] style(lint): Auto commit lint changes --- .../processing/realtime_metrics/test_realtime_metrics_store.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py index d7490b704bbb43..44aec9dc0a5924 100644 --- a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py +++ b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py @@ -87,7 +87,7 @@ def test_get_lpq_projects_filled( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) in_lpq = store.get_lpq_projects() - assert in_lpq == set([1]) + assert in_lpq == {1} def test_add_project_to_lpq_unset( From 9c61ac61652571d1caec60de00940c61034e53bb Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 21 Sep 2021 21:57:10 -0400 Subject: [PATCH 03/16] forgot to update tests --- .../realtime_metrics/test_realtime_metrics_store.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py index 44aec9dc0a5924..fa5392dc891194 100644 --- a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py +++ b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py @@ -93,7 +93,7 @@ def test_get_lpq_projects_filled( def test_add_project_to_lpq_unset( store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster ) -> None: - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -104,7 +104,7 @@ def test_add_project_to_lpq_empty( redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -114,7 +114,7 @@ def test_add_project_to_lpq_dupe( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -124,7 +124,7 @@ def test_add_project_to_lpq_filled( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1", "11"} From ed1b4ac39706708a236adfe3d6dd0b3828f89fdc Mon Sep 17 00:00:00 2001 From: Betty Da Date: Tue, 21 Sep 2021 22:03:17 -0400 Subject: [PATCH 04/16] fix types --- src/sentry/processing/realtime_metrics/base.py | 3 ++- src/sentry/processing/realtime_metrics/redis.py | 15 +++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/base.py b/src/sentry/processing/realtime_metrics/base.py index bcdcf306f6cfea..a72a7458c4911f 100644 --- a/src/sentry/processing/realtime_metrics/base.py +++ b/src/sentry/processing/realtime_metrics/base.py @@ -44,7 +44,7 @@ def get_lpq_projects(self) -> Set[int]: """ pass - def add_project_to_lpq(self, project_id: int): + def add_project_to_lpq(self, project_id: int) -> None: """ Moves projects to the low priority queue. @@ -68,3 +68,4 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: Returns all projects that have been successfully removed from the low priority queue. """ + pass diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index a53b43ab905a67..ae3e0a01e47e92 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -93,14 +93,13 @@ def get_lpq_projects(self) -> Set[int]: Returns a list of project IDs. """ - # TODO: there's got to be a better way to do this instead of invoking _to_int twice - return { - _to_int(project_id) - for project_id in self.inner.smembers(LPQ_MEMBERS_KEY) - if _to_int(project_id) is not None - } - - def add_project_to_lpq(self, project_id: int): + return set( + filter( + None, {_to_int(project_id) for project_id in self.inner.smembers(LPQ_MEMBERS_KEY)} + ) + ) + + def add_project_to_lpq(self, project_id: int) -> None: """ Moves projects to the low priority queue. From 9887140c1c0aaa3673a14404676ed22bf09bf04d Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:41:48 -0400 Subject: [PATCH 05/16] TODO answered --- src/sentry/processing/realtime_metrics/redis.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index ae3e0a01e47e92..5c8ab72437ac72 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -85,8 +85,6 @@ def increment_project_duration_counter( pipeline.pexpire(key, self._histogram_ttl) pipeline.execute() - # TODO: do these killswitch helpers belong here, or in a different class? LowPriorityQueueStore? - # This probably needs locking def get_lpq_projects(self) -> Set[int]: """ Fetches the list of projects that are currently using the low priority queue. From 2cabcf9c5f1d4c0a7724fd01d1fceb154cbc4903 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:42:12 -0400 Subject: [PATCH 06/16] better name --- src/sentry/processing/realtime_metrics/redis.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 5c8ab72437ac72..8a1592a1fe40ed 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -93,7 +93,11 @@ def get_lpq_projects(self) -> Set[int]: """ return set( filter( - None, {_to_int(project_id) for project_id in self.inner.smembers(LPQ_MEMBERS_KEY)} + None, + { + _to_int(project_id_raw) + for project_id_raw in self.inner.smembers(LPQ_MEMBERS_KEY) + }, ) ) From 60091a4b0ef649dfa04d235477f60d6cea1fb852 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:43:02 -0400 Subject: [PATCH 07/16] fix up docs --- src/sentry/processing/realtime_metrics/base.py | 18 ++++++------------ .../processing/realtime_metrics/redis.py | 13 ++++++------- 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/base.py b/src/sentry/processing/realtime_metrics/base.py index a72a7458c4911f..cc4536ebed8f3c 100644 --- a/src/sentry/processing/realtime_metrics/base.py +++ b/src/sentry/processing/realtime_metrics/base.py @@ -46,14 +46,11 @@ def get_lpq_projects(self) -> Set[int]: def add_project_to_lpq(self, project_id: int) -> None: """ - Moves projects to the low priority queue. + Moves a project to the low priority queue. - This forces all symbolication events triggered by the specified projects to be redirected to - the low priority queue, unless these projects have been manually excluded from the low - priority queue via the `store.symbolicate-event-lpq-never` kill switch. - - Raises ``LowPriorityQueueMembershipError`` if this fails to add the specified project to the - low priority queue. + This forces all symbolication events triggered by the specified project to be redirected to + the low priority queue, unless the project is manually excluded from the low priority queue + via the `store.symbolicate-event-lpq-never` kill switch. """ pass @@ -62,10 +59,7 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: Removes projects from the low priority queue. This restores all specified projects back to the regular queue, unless they have been - manually forced into the low priority queue via the store.symbolicate-event-lpq-always kill - switch. - - - Returns all projects that have been successfully removed from the low priority queue. + manually forced into the low priority queue via the `store.symbolicate-event-lpq-always` + kill switch. """ pass diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 8a1592a1fe40ed..397eb542da8e81 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -103,11 +103,11 @@ def get_lpq_projects(self) -> Set[int]: def add_project_to_lpq(self, project_id: int) -> None: """ - Moves projects to the low priority queue. + Moves a project to the low priority queue. - This forces all symbolication events triggered by the specified projects to be redirected to - the low priority queue, unless these projects have been manually excluded from the low - priority queue via the store.symbolicate-event-lpq-never kill switch. + This forces all symbolication events triggered by the specified project to be redirected to + the low priority queue, unless the project is manually excluded from the low priority queue + via the `store.symbolicate-event-lpq-never` kill switch. Raises ``LowPriorityQueueMembershipError`` if this fails to add the specified project to the low priority queue. @@ -127,9 +127,8 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: Removes projects from the low priority queue. This restores all specified projects back to the regular queue, unless they have been - manually forced into the low priority queue via the store.symbolicate-event-lpq-always kill - switch. - + manually forced into the low priority queue via the `store.symbolicate-event-lpq-always` + kill switch. Returns all projects that have been successfully removed from the low priority queue. """ From 7926d03a8b9c8daa50d4553d585ed6693f74fc7f Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:43:31 -0400 Subject: [PATCH 08/16] fix types once again --- src/sentry/processing/realtime_metrics/redis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 397eb542da8e81..a786e521390a49 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -147,7 +147,7 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: return was_removed -def _to_int(value: str or int) -> Optional[int]: +def _to_int(value: str) -> Optional[int]: try: return int(value) if value else None except ValueError: From 181a66168e9f5ac1fe4169ea71a2cab3f5175dc8 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:43:54 -0400 Subject: [PATCH 09/16] partial updates aren't a thing so they don't need to be handled --- .../processing/realtime_metrics/redis.py | 42 +++++-------------- 1 file changed, 11 insertions(+), 31 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index a786e521390a49..ea42b0d0c200d3 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -109,20 +109,16 @@ def add_project_to_lpq(self, project_id: int) -> None: the low priority queue, unless the project is manually excluded from the low priority queue via the `store.symbolicate-event-lpq-never` kill switch. - Raises ``LowPriorityQueueMembershipError`` if this fails to add the specified project to the - low priority queue. + This may throw an exception if there is some sort of issue registering the project with the + queue. """ - added = self.inner.sadd(LPQ_MEMBERS_KEY, project_id) + # This returns 0 if project_id was already in the set, 1 if it was added, and throws an + # exception if there's a problem so it's fine if we just ignore the return value of this as + # the project is always added if this successfully completes. + self.inner.sadd(LPQ_MEMBERS_KEY, project_id) - # Looks like this might already be in the LPQ, or redis failed to add it. This is only a - # problem if redis failed to add it. - if added == 0 and not self.inner.sismember(LPQ_MEMBERS_KEY, project_id): - raise LowPriorityQueueMembershipError( - f"Failed to move project ID {project_id} to the low priority queue" - ) - - def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: + def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: """ Removes projects from the low priority queue. @@ -130,21 +126,13 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: manually forced into the low priority queue via the `store.symbolicate-event-lpq-always` kill switch. - Returns all projects that have been successfully removed from the low priority queue. + This may throw an exception if there is some sort of issue deregistering the projects from + the queue. """ if len(project_ids) == 0: - return set() - - removed = self.inner.srem(LPQ_MEMBERS_KEY, *project_ids) - - if removed == len(project_ids): - return project_ids + return - # Looks like only a subset of the project IDs were removed from the LPQ list. - in_lpq = self.inner.smembers(LPQ_MEMBERS_KEY) - was_removed = project_ids.intersection(in_lpq) - - return was_removed + self.inner.srem(LPQ_MEMBERS_KEY, *project_ids) def _to_int(value: str) -> Optional[int]: @@ -152,11 +140,3 @@ def _to_int(value: str) -> Optional[int]: return int(value) if value else None except ValueError: return None - - -class LowPriorityQueueMembershipError(Exception): - """ - Something went wrong while updating the list of projects designated for the low priority queue. - """ - - pass From f817e9b2dd06d05a9daa7912a5f91a912c0e80c3 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 15:53:25 -0400 Subject: [PATCH 10/16] types --- src/sentry/processing/realtime_metrics/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/processing/realtime_metrics/base.py b/src/sentry/processing/realtime_metrics/base.py index cc4536ebed8f3c..1d0c5404257eab 100644 --- a/src/sentry/processing/realtime_metrics/base.py +++ b/src/sentry/processing/realtime_metrics/base.py @@ -54,7 +54,7 @@ def add_project_to_lpq(self, project_id: int) -> None: """ pass - def remove_projects_from_lpq(self, project_ids: Set[int]) -> Set[int]: + def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: """ Removes projects from the low priority queue. From a88117910cf39e46a83e05bf63177fda2dcf1bd7 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 22 Sep 2021 19:17:33 -0400 Subject: [PATCH 11/16] inner was renamed to cluster --- src/sentry/processing/realtime_metrics/redis.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index ea42b0d0c200d3..248de15913489c 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -96,7 +96,7 @@ def get_lpq_projects(self) -> Set[int]: None, { _to_int(project_id_raw) - for project_id_raw in self.inner.smembers(LPQ_MEMBERS_KEY) + for project_id_raw in self.cluster.smembers(LPQ_MEMBERS_KEY) }, ) ) @@ -116,7 +116,7 @@ def add_project_to_lpq(self, project_id: int) -> None: # This returns 0 if project_id was already in the set, 1 if it was added, and throws an # exception if there's a problem so it's fine if we just ignore the return value of this as # the project is always added if this successfully completes. - self.inner.sadd(LPQ_MEMBERS_KEY, project_id) + self.cluster.sadd(LPQ_MEMBERS_KEY, project_id) def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: """ @@ -132,7 +132,7 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: if len(project_ids) == 0: return - self.inner.srem(LPQ_MEMBERS_KEY, *project_ids) + self.cluster.srem(LPQ_MEMBERS_KEY, *project_ids) def _to_int(value: str) -> Optional[int]: From 94c3be27b126c52833cc52d225fcddd4db832bc5 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Thu, 23 Sep 2021 19:12:24 -0400 Subject: [PATCH 12/16] fix tests --- .../test_realtime_metrics_store.py | 214 ------------------ .../processing/realtime_metrics/test_redis.py | 30 +-- 2 files changed, 11 insertions(+), 233 deletions(-) delete mode 100644 tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py diff --git a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py b/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py deleted file mode 100644 index fa5392dc891194..00000000000000 --- a/tests/sentry/processing/realtime_metrics/test_realtime_metrics_store.py +++ /dev/null @@ -1,214 +0,0 @@ -import datetime -import time -from typing import TYPE_CHECKING - -import pytest - -from sentry.processing.realtime_metrics import base # type: ignore -from sentry.utils import redis - -if TYPE_CHECKING: - from typing import Callable, TypeVar - - # Declare fixture decorator to swallow the function, this isn't the actual type returned - # but the type is unusable as a function which is what matters. - F = TypeVar("F", bound=Callable[..., None]) - - def _fixture(func: F) -> F: - ... - - pytest.fixture = _fixture - - -@pytest.fixture -def redis_cluster() -> redis._RedisCluster: - return redis.redis_clusters.get("default") - - -@pytest.fixture -def store(redis_cluster: redis._RedisCluster) -> base.RealtimeMetricsStore: - return base.RealtimeMetricsStore( - redis_cluster, counter_bucket_size=10, counter_ttl=datetime.timedelta(milliseconds=400) - ) - - -def test_increment_project_event_counter( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - store.increment_project_event_counter(17, 1147) - counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") - assert counter == "1" - time.sleep(0.5) - counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") - assert counter is None - - -def test_increment_project_event_counter_twice( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - store.increment_project_event_counter(17, 1147) - time.sleep(0.2) - store.increment_project_event_counter(17, 1149) - counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") - assert counter == "2" - time.sleep(0.3) - # it should have expired by now - counter = redis_cluster.get("symbolicate_event_low_priority:17:1140") - assert counter is None - - -def test_increment_project_event_counter_multiple( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - store.increment_project_event_counter(17, 1147) - store.increment_project_event_counter(17, 1152) - - assert redis_cluster.get("symbolicate_event_low_priority:17:1140") == "1" - assert redis_cluster.get("symbolicate_event_low_priority:17:1150") == "1" - - -def test_get_lpq_projects_unset(store: base.RealtimeMetricsStore) -> None: - in_lpq = store.get_lpq_projects() - assert in_lpq == set() - - -def test_get_lpq_projects_empty( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) - - in_lpq = store.get_lpq_projects() - assert in_lpq == set() - - -def test_get_lpq_projects_filled( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - in_lpq = store.get_lpq_projects() - assert in_lpq == {1} - - -def test_add_project_to_lpq_unset( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - store.add_project_to_lpq(1) - in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert in_lpq == {"1"} - - -def test_add_project_to_lpq_empty( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) - - store.add_project_to_lpq(1) - in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert in_lpq == {"1"} - - -def test_add_project_to_lpq_dupe( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - - store.add_project_to_lpq(1) - in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert in_lpq == {"1"} - - -def test_add_project_to_lpq_filled( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - - store.add_project_to_lpq(1) - in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert in_lpq == {"1", "11"} - - -def test_remove_projects_from_lpq_unset( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - removed = store.remove_projects_from_lpq({1}) - assert removed == set() - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == set() - - -def test_remove_projects_from_lpq_empty( - store: base.RealtimeMetricsStore, 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_projects_from_lpq({1}) - assert removed == set() - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == set() - - -def test_remove_projects_from_lpq_only_member( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - - removed = store.remove_projects_from_lpq({1}) - assert removed == {1} - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == set() - - -def test_remove_projects_from_lpq_nonmember( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - - removed = store.remove_projects_from_lpq({1}) - assert removed == set() - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == {"11"} - - -def test_remove_projects_from_lpq_subset( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - - removed = store.remove_projects_from_lpq({1}) - assert removed == {1} - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == {"11"} - - -def test_remove_projects_from_lpq_all_members( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - - removed = store.remove_projects_from_lpq({1, 11}) - assert removed == {1, 11} - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == set() - - -def test_remove_projects_from_lpq_no_members( - store: base.RealtimeMetricsStore, redis_cluster: redis._RedisCluster -) -> None: - redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - - removed = store.remove_projects_from_lpq({}) - assert removed == set() - - remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") - assert remaining == {"1"} diff --git a/tests/sentry/processing/realtime_metrics/test_redis.py b/tests/sentry/processing/realtime_metrics/test_redis.py index c1d3f98efe80bb..9640116795ddcb 100644 --- a/tests/sentry/processing/realtime_metrics/test_redis.py +++ b/tests/sentry/processing/realtime_metrics/test_redis.py @@ -140,7 +140,7 @@ def test_get_lpq_projects_filled( def test_add_project_to_lpq_unset( store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster ) -> None: - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -151,7 +151,7 @@ def test_add_project_to_lpq_empty( redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -161,7 +161,7 @@ def test_add_project_to_lpq_dupe( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1"} @@ -171,7 +171,7 @@ def test_add_project_to_lpq_filled( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - assert store.add_project_to_lpq(1) + store.add_project_to_lpq(1) in_lpq = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert in_lpq == {"1", "11"} @@ -179,8 +179,7 @@ def test_add_project_to_lpq_filled( def test_remove_projects_from_lpq_unset( store: RedisRealtimeMetricsStore, redis_cluster: redis._RedisCluster ) -> None: - removed = store.remove_projects_from_lpq({1}) - assert removed == set() + store.remove_projects_from_lpq({1}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == set() @@ -192,9 +191,7 @@ def test_remove_projects_from_lpq_empty( redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) redis_cluster.srem("store.symbolicate-event-lpq-selected", 1) - removed = store.remove_projects_from_lpq({1}) - assert removed == set() - + store.remove_projects_from_lpq({1}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == set() @@ -204,8 +201,7 @@ def test_remove_projects_from_lpq_only_member( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - removed = store.remove_projects_from_lpq({1}) - assert removed == {1} + store.remove_projects_from_lpq({1}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == set() @@ -216,8 +212,7 @@ def test_remove_projects_from_lpq_nonmember( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - removed = store.remove_projects_from_lpq({1}) - assert removed == set() + store.remove_projects_from_lpq({1}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == {"11"} @@ -229,8 +224,7 @@ def test_remove_projects_from_lpq_subset( redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - removed = store.remove_projects_from_lpq({1}) - assert removed == {1} + store.remove_projects_from_lpq({1}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == {"11"} @@ -242,8 +236,7 @@ def test_remove_projects_from_lpq_all_members( redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) redis_cluster.sadd("store.symbolicate-event-lpq-selected", 11) - removed = store.remove_projects_from_lpq({1, 11}) - assert removed == {1, 11} + store.remove_projects_from_lpq({1, 11}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == set() @@ -254,8 +247,7 @@ def test_remove_projects_from_lpq_no_members( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) - removed = store.remove_projects_from_lpq({}) - assert removed == set() + store.remove_projects_from_lpq({}) remaining = redis_cluster.smembers("store.symbolicate-event-lpq-selected") assert remaining == {"1"} From 3ffba2ea6b195bc16cb84ac847931b058ae50ce6 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 29 Sep 2021 16:09:15 -0400 Subject: [PATCH 13/16] remove parts of docstrings that aren't implemented by the function it describes --- src/sentry/processing/realtime_metrics/redis.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 248de15913489c..aad40853996cd3 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -103,11 +103,10 @@ def get_lpq_projects(self) -> Set[int]: def add_project_to_lpq(self, project_id: int) -> None: """ - Moves a project to the low priority queue. + Assigns a project to the low priority queue. - This forces all symbolication events triggered by the specified project to be redirected to - the low priority queue, unless the project is manually excluded from the low priority queue - via the `store.symbolicate-event-lpq-never` kill switch. + This registers an intent to redirect all symbolication events triggered by the specified + project to be redirected to the low priority queue. This may throw an exception if there is some sort of issue registering the project with the queue. @@ -122,9 +121,7 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: """ Removes projects from the low priority queue. - This restores all specified projects back to the regular queue, unless they have been - manually forced into the low priority queue via the `store.symbolicate-event-lpq-always` - kill switch. + This registers an intent to restore all specified projects back to the regular queue. This may throw an exception if there is some sort of issue deregistering the projects from the queue. From 307bb51d65508f9c0593891dbf0327ca0f33c307 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 29 Sep 2021 16:14:17 -0400 Subject: [PATCH 14/16] axe _to_int because it hides a problem we want visible --- .../processing/realtime_metrics/redis.py | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index aad40853996cd3..3d980648f143ad 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -1,5 +1,5 @@ import datetime -from typing import Optional, Set +from typing import Set from sentry.exceptions import InvalidConfiguration from sentry.utils import redis @@ -91,15 +91,7 @@ def get_lpq_projects(self) -> Set[int]: Returns a list of project IDs. """ - return set( - filter( - None, - { - _to_int(project_id_raw) - for project_id_raw in self.cluster.smembers(LPQ_MEMBERS_KEY) - }, - ) - ) + return {project_id_raw for project_id_raw in self.cluster.smembers(LPQ_MEMBERS_KEY)} def add_project_to_lpq(self, project_id: int) -> None: """ @@ -130,10 +122,3 @@ def remove_projects_from_lpq(self, project_ids: Set[int]) -> None: return self.cluster.srem(LPQ_MEMBERS_KEY, *project_ids) - - -def _to_int(value: str) -> Optional[int]: - try: - return int(value) if value else None - except ValueError: - return None From 8bf11bca983f98510de248369b00ec91a8efd250 Mon Sep 17 00:00:00 2001 From: Betty Da Date: Wed, 29 Sep 2021 17:47:53 -0400 Subject: [PATCH 15/16] missed a detail --- src/sentry/processing/realtime_metrics/redis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/processing/realtime_metrics/redis.py b/src/sentry/processing/realtime_metrics/redis.py index 3d980648f143ad..24a21c3e61e607 100644 --- a/src/sentry/processing/realtime_metrics/redis.py +++ b/src/sentry/processing/realtime_metrics/redis.py @@ -91,7 +91,7 @@ def get_lpq_projects(self) -> Set[int]: Returns a list of project IDs. """ - return {project_id_raw for project_id_raw in self.cluster.smembers(LPQ_MEMBERS_KEY)} + return {int(project_id) for project_id in self.cluster.smembers(LPQ_MEMBERS_KEY)} def add_project_to_lpq(self, project_id: int) -> None: """ From c131716549656f164d17ff350582fde8b576d333 Mon Sep 17 00:00:00 2001 From: "getsantry[bot]" <66042841+getsantry[bot]@users.noreply.github.com> Date: Wed, 29 Sep 2021 21:49:07 +0000 Subject: [PATCH 16/16] style(lint): Auto commit lint changes --- tests/sentry/processing/realtime_metrics/test_redis.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/processing/realtime_metrics/test_redis.py b/tests/sentry/processing/realtime_metrics/test_redis.py index 9640116795ddcb..20107b885abb9f 100644 --- a/tests/sentry/processing/realtime_metrics/test_redis.py +++ b/tests/sentry/processing/realtime_metrics/test_redis.py @@ -134,7 +134,7 @@ def test_get_lpq_projects_filled( ) -> None: redis_cluster.sadd("store.symbolicate-event-lpq-selected", 1) in_lpq = store.get_lpq_projects() - assert in_lpq == set([1]) + assert in_lpq == {1} def test_add_project_to_lpq_unset(