Revert "feat(proj-config): Add v3 of proj config endpoint (#34746)" (#34956)

This reverts commit d9e850f2723a9d4919b0038d2f6cb59321eef295.

src/sentry/api/endpoints/relay/project_configs.py

@@ -1,6 +1,5 @@
 import logging
 import random
-from typing import Optional
 
 from django.conf import settings
 from rest_framework.request import Request
@@ -12,7 +11,6 @@ from sentry.api.base import Endpoint
 from sentry.api.permissions import RelayPermission
 from sentry.models import Organization, OrganizationOption, Project, ProjectKey, ProjectKeyStatus
 from sentry.relay import config, projectconfig_cache
-from sentry.tasks.relay import schedule_update_config_cache
 from sentry.utils import metrics
 
 logger = logging.getLogger(__name__)
@@ -48,15 +46,7 @@ class RelayProjectConfigsEndpoint(Endpoint):
         version = request.GET.get("version") or "1"
         set_tag("relay_protocol_version", version)
 
-        no_cache = request.relay_request_data.get("no_cache") or False
-        set_tag("relay_no_cache", no_cache)
-
-        if version == "3" and not no_cache:
-            # Always compute the full config. It's invalid to send partial
-            # configs to processing relays, and these validate the requests they
-            # get with permissions and trim configs down accordingly.
-            return self._post_or_schedule_by_key(request)
-        elif version in ["2", "3"]:
+        if version in ["2", "3"]:
             return self._post_by_key(
                 request=request,
                 full_config_requested=full_config_requested,
@@ -69,42 +59,6 @@ class RelayProjectConfigsEndpoint(Endpoint):
         else:
             return Response("Unsupported version, we only support versions 1 to 3.", 400)
 
-    def _post_or_schedule_by_key(self, request: Request):
-        public_keys = set(request.relay_request_data.get("publicKeys") or ())
-
-        proj_configs = {}
-        pending = []
-        for key in public_keys:
-            computed = self._get_cached_or_schedule(key)
-            if not computed:
-                pending.append(key)
-            else:
-                proj_configs[key] = computed
-
-        res = {"configs": proj_configs, "pending": pending}
-
-        return Response(res, status=200)
-
-    def _get_cached_or_schedule(self, public_key) -> Optional[dict]:
-        """
-        Returns the config of a project if it's in the cache; else, schedules a
-        task to compute and write it into the cache.
-
-        Debouncing of the project happens after the task has been scheduled.
-        """
-        cached_config = projectconfig_cache.get(public_key)
-        if cached_config:
-            return cached_config
-
-        schedule_update_config_cache(
-            generate=True,
-            organization_id=None,
-            project_id=None,
-            public_key=public_key,
-            update_reason="project_config.post_v3",
-        )
-        return None
-
     def _post_by_key(self, request: Request, full_config_requested):
         public_keys = request.relay_request_data.get("publicKeys")
         public_keys = set(public_keys or ())

src/sentry/conf/server.py

@@ -1354,7 +1354,7 @@ SENTRY_QUOTAS = "sentry.quotas.Quota"
 SENTRY_QUOTA_OPTIONS = {}
 
 # Cache for Relay project configs
-SENTRY_RELAY_PROJECTCONFIG_CACHE = "sentry.relay.projectconfig_cache.redis.RedisProjectConfigCache"
+SENTRY_RELAY_PROJECTCONFIG_CACHE = "sentry.relay.projectconfig_cache.base.ProjectConfigCache"
 SENTRY_RELAY_PROJECTCONFIG_CACHE_OPTIONS = {}
 
 # Which cache to use for debouncing cache updates to the projectconfig cache

src/sentry/relay/projectconfig_debounce_cache/base.py

@@ -44,7 +44,4 @@ class ProjectConfigDebounceCache(Service):
         """
         Mark a task done such that `check_is_debounced` starts emitting False
         for the given parameters.
-
-        Returns 1 if the task was removed, 0 if it wasn't.
         """
-        return 1

src/sentry/relay/projectconfig_debounce_cache/redis.py

@@ -52,4 +52,4 @@ class RedisProjectConfigDebounceCache(ProjectConfigDebounceCache):
     def mark_task_done(self, public_key, project_id, organization_id):
         key = _get_redis_key(public_key, project_id, organization_id)
         client = self.__get_redis_client(key)
-        return client.delete(key)
+        client.delete(key)

src/sentry/tasks/relay.py

@@ -46,15 +46,13 @@ def update_config_cache(
     sentry_sdk.set_tag("update_reason", update_reason)
     sentry_sdk.set_tag("generate", generate)
 
-    # Not running this at the beginning of the task to add tags in case there's
-    # something wrong going on.
-    if not should_update_cache(
-        organization_id=organization_id, project_id=project_id, public_key=public_key
-    ):
-        # XXX(iker): this approach doesn't work with celery's ack_late enabled.
-        # If ack_late is enabled and the task fails after being marked as done,
-        # the second attempt will exit early and not compute the project config.
-        return
+    # Delete key before generating configs such that we never have an outdated
+    # but valid cache.
+    #
+    # If this was running at the end of the task, it would be more effective
+    # against bursts of updates, but introduces a different race where an
+    # outdated cache may be used.
+    projectconfig_debounce_cache.mark_task_done(public_key, project_id, organization_id)
 
     if organization_id:
         projects = list(Project.objects.filter(organization_id=organization_id))
@@ -67,13 +65,13 @@ def update_config_cache(
             keys = [ProjectKey.objects.get(public_key=public_key)]
         except ProjectKey.DoesNotExist:
             # In this particular case, where a project key got deleted and
-            # triggered an update, we know that key doesn't exist and we want to
-            # avoid creating more tasks for it.
+            # triggered an update, we at least know the public key that needs
+            # to be deleted from cache.
             #
             # In other similar cases, like an org being deleted, we potentially
             # cannot find any keys anymore, so we don't know which cache keys
             # to delete.
-            projectconfig_cache.set_many({public_key: {"disabled": True}})
+            projectconfig_cache.delete_many([public_key])
             return
 
     else:
@@ -99,21 +97,6 @@ def update_config_cache(
         projectconfig_cache.delete_many(cache_keys_to_delete)
 
 
-def should_update_cache(organization_id=None, project_id=None, public_key=None) -> bool:
-    # Delete key before generating configs such that we never have an outdated
-    # but valid cache. Also, only run task if it's still scheduled and another
-    # worker hasn't picked it up.
-    #
-    # If this was running at the end of the task, it would be more effective
-    # against bursts of updates, but introduces a different race where an
-    # outdated cache may be used.
-    return 0 < projectconfig_debounce_cache.mark_task_done(
-        organization_id=organization_id,
-        project_id=project_id,
-        public_key=public_key,
-    )
-
-
 def schedule_update_config_cache(
     generate, project_id=None, organization_id=None, public_key=None, update_reason=None
 ):
@@ -141,7 +124,7 @@ def schedule_update_config_cache(
             "One of organization_id, project_id, public_key has to be provided, not many."
         )
 
-    if projectconfig_debounce_cache.is_debounced(public_key, project_id, organization_id):
+    if projectconfig_debounce_cache.check_is_debounced(public_key, project_id, organization_id):
         metrics.incr(
             "relay.projectconfig_cache.skipped",
             tags={"reason": "debounce", "update_reason": update_reason},
@@ -163,10 +146,3 @@ def schedule_update_config_cache(
         public_key=public_key,
         update_reason=update_reason,
     )
-
-    # Checking if the project is debounced and debouncing it are two separate
-    # actions that aren't atomic. If the process marks a project as debounced
-    # and dies before scheduling it, the cache will be stale for the whole TTL.
-    # To avoid that, make sure we first schedule the task, and only then mark
-    # the project as debounced.
-    projectconfig_debounce_cache.debounce(public_key, project_id, organization_id)

tests/sentry/api/endpoints/test_relay_projectconfigs_v2.py

@@ -376,3 +376,15 @@ def test_exposes_features(call_endpoint, task_runner):
         for config in result["configs"].values():
             config = config["config"]
             assert config["sessionMetrics"] == {"version": 1, "drop": False}
+
+
+@pytest.mark.django_db
+def test_v3(call_endpoint):
+    # For now v2 and v3 are supposed to be identical, the payload is backwards compatible if
+    # there are no pending configs and we never generate pending configs currently.
+    # with task_runner():
+    result, status_code = call_endpoint(full_config=False, version=3)
+
+    assert status_code < 400
+    assert result.get("configs")
+    assert not result.get("pending")

tests/sentry/api/endpoints/test_relay_projectconfigs_v3.py

@@ -1,212 +0,0 @@
-from unittest.mock import patch
-from uuid import uuid4
-
-import pytest
-from django.urls import reverse
-from sentry_relay.auth import generate_key_pair
-
-from sentry.models.relay import Relay
-from sentry.relay.config import ProjectConfig
-from sentry.tasks.relay import update_config_cache
-from sentry.utils import json
-
-
-@pytest.fixture
-def key_pair():
-    return generate_key_pair()
-
-
-@pytest.fixture
-def public_key(key_pair):
-    return key_pair[1]
-
-
-@pytest.fixture
-def private_key(key_pair):
-    return key_pair[0]
-
-
-@pytest.fixture
-def relay_id():
-    return str(uuid4())
-
-
-@pytest.fixture
-def relay(relay_id, public_key):
-    return Relay.objects.create(relay_id=relay_id, public_key=str(public_key), is_internal=True)
-
-
-@pytest.fixture
-def call_endpoint(client, relay, private_key, default_projectkey):
-    def inner(full_config, public_keys=None):
-        path = reverse("sentry-api-0-relay-projectconfigs") + "?version=3"
-
-        if public_keys is None:
-            public_keys = [str(default_projectkey.public_key)]
-
-        if full_config is None:
-            raw_json, signature = private_key.pack({"publicKeys": public_keys, "no_cache": False})
-        else:
-            raw_json, signature = private_key.pack(
-                {"publicKeys": public_keys, "fullConfig": full_config, "no_cache": False}
-            )
-
-        resp = client.post(
-            path,
-            data=raw_json,
-            content_type="application/json",
-            HTTP_X_SENTRY_RELAY_ID=relay.relay_id,
-            HTTP_X_SENTRY_RELAY_SIGNATURE=signature,
-        )
-
-        return json.loads(resp.content), resp.status_code
-
-    return inner
-
-
-@pytest.fixture
-def never_update_cache(monkeypatch):
-    monkeypatch.setattr("sentry.tasks.relay.should_update_cache", lambda *args, **kwargs: False)
-
-
-@pytest.fixture
-def always_update_cache(monkeypatch):
-    monkeypatch.setattr("sentry.tasks.relay.should_update_cache", lambda *args, **kwargs: True)
-
-
-@pytest.fixture
-def projectconfig_cache_get_mock_config(monkeypatch):
-    monkeypatch.setattr(
-        "sentry.relay.projectconfig_cache.get", lambda *args, **kwargs: {"is_mock_config": True}
-    )
-
-
-@pytest.fixture
-def single_mock_proj_cached(monkeypatch):
-    def cache_get(*args, **kwargs):
-        if args[0] == "must_exist":
-            return {"is_mock_config": True}
-        return None
-
-    monkeypatch.setattr("sentry.relay.projectconfig_cache.get", cache_get)
-
-
-@pytest.fixture
-def projectconfig_debounced_cache(monkeypatch):
-    monkeypatch.setattr(
-        "sentry.relay.projectconfig_debounce_cache.is_debounced", lambda *args, **kargs: True
-    )
-
-
-@pytest.fixture
-def project_config_get_mock(monkeypatch):
-    monkeypatch.setattr(
-        "sentry.relay.config.get_project_config",
-        lambda *args, **kwargs: ProjectConfig("mock_project", is_mock_config=True),
-    )
-
-
-@pytest.mark.django_db
-def test_return_full_config_if_in_cache(
-    call_endpoint, default_projectkey, projectconfig_cache_get_mock_config
-):
-    result, status_code = call_endpoint(full_config=True)
-    assert status_code < 400
-    assert result == {
-        "configs": {default_projectkey.public_key: {"is_mock_config": True}},
-        "pending": [],
-    }
-
-
-@pytest.mark.django_db
-def test_return_partial_config_if_in_cache(
-    call_endpoint, default_projectkey, projectconfig_cache_get_mock_config
-):
-    # Partial configs aren't supported, but the endpoint must always return full
-    # configs.
-    result, status_code = call_endpoint(full_config=False)
-    assert status_code < 400
-    expected = {
-        "configs": {default_projectkey.public_key: {"is_mock_config": True}},
-        "pending": [],
-    }
-    assert result == expected
-
-
-@pytest.mark.django_db
-def test_proj_in_cache_and_another_pending(
-    call_endpoint, default_projectkey, single_mock_proj_cached
-):
-    result, status_code = call_endpoint(
-        full_config=True, public_keys=["must_exist", default_projectkey.public_key]
-    )
-    assert status_code < 400
-    assert result == {
-        "configs": {"must_exist": {"is_mock_config": True}},
-        "pending": [default_projectkey.public_key],
-    }
-
-
-@patch("sentry.tasks.relay.update_config_cache.delay")
-@pytest.mark.django_db
-def test_enqueue_task_if_config_not_cached_not_queued(
-    schedule_mock,
-    call_endpoint,
-    default_projectkey,
-):
-    result, status_code = call_endpoint(full_config=True)
-    assert status_code < 400
-    assert result == {"configs": {}, "pending": [default_projectkey.public_key]}
-    assert schedule_mock.call_count == 1
-
-
-@patch("sentry.tasks.relay.update_config_cache.delay")
-@pytest.mark.django_db
-def test_debounce_task_if_proj_config_not_cached_already_enqueued(
-    task_mock,
-    call_endpoint,
-    default_projectkey,
-    projectconfig_debounced_cache,
-):
-    result, status_code = call_endpoint(full_config=True)
-    assert status_code < 400
-    assert result == {"configs": {}, "pending": [default_projectkey.public_key]}
-    assert task_mock.call_count == 0
-
-
-@patch("sentry.relay.projectconfig_cache.set_many")
-@pytest.mark.django_db
-def test_task_doesnt_run_if_not_debounced(
-    cache_set_many_mock, default_projectkey, never_update_cache
-):
-    update_config_cache(
-        generate=True,
-        organization_id=None,
-        project_id=None,
-        public_key=default_projectkey.public_key,
-        update_reason="test",
-    )
-    assert cache_set_many_mock.call_count == 0
-
-
-@patch("sentry.relay.projectconfig_cache.set_many")
-@pytest.mark.django_db
-def test_task_writes_config_into_cache(
-    cache_set_many_mock,
-    default_projectkey,
-    always_update_cache,
-    project_config_get_mock,
-):
-    update_config_cache(
-        generate=True,
-        organization_id=None,
-        project_id=None,
-        public_key=default_projectkey.public_key,
-        update_reason="test",
-    )
-
-    assert cache_set_many_mock.call_count == 1
-    # Using a tuple because that's the format `.args` uses
-    assert cache_set_many_mock.call_args.args == (
-        {default_projectkey.public_key: {"is_mock_config": True}},
-    )

tests/sentry/tasks/test_relay.py

@@ -30,44 +30,29 @@ def redis_cache(monkeypatch):
         "sentry.relay.projectconfig_debounce_cache.redis.RedisProjectConfigDebounceCache",
     )
 
-    return cache
-
-
-@pytest.fixture
-def debounce_cache(monkeypatch):
     debounce_cache = RedisProjectConfigDebounceCache()
     monkeypatch.setattr(
-        "sentry.relay.projectconfig_debounce_cache.mark_task_done",
-        debounce_cache.mark_task_done,
+        "sentry.relay.projectconfig_debounce_cache.mark_task_done", debounce_cache.mark_task_done
     )
     monkeypatch.setattr(
         "sentry.relay.projectconfig_debounce_cache.check_is_debounced",
         debounce_cache.check_is_debounced,
     )
-    monkeypatch.setattr(
-        "sentry.relay.projectconfig_debounce_cache.debounce",
-        debounce_cache.debounce,
-    )
-    monkeypatch.setattr(
-        "sentry.relay.projectconfig_debounce_cache.is_debounced",
-        debounce_cache.is_debounced,
-    )
 
-    return debounce_cache
+    return cache
 
 
-@pytest.fixture
-def always_update_cache(monkeypatch):
-    monkeypatch.setattr("sentry.tasks.relay.should_update_cache", lambda *args, **kwargs: True)
+@pytest.mark.django_db
+def test_no_cache(monkeypatch, default_project):
+    def apply_async(*a, **kw):
+        assert False
+
+    monkeypatch.setattr("sentry.tasks.relay.update_config_cache.apply_async", apply_async)
+    schedule_update_config_cache(generate=True, project_id=default_project.id)
 
 
 @pytest.mark.django_db
-def test_debounce(
-    monkeypatch,
-    default_project,
-    default_organization,
-    debounce_cache,
-):
+def test_debounce(monkeypatch, default_project, default_organization, redis_cache):
     tasks = []
 
     def apply_async(args, kwargs):
@@ -76,11 +61,9 @@ def test_debounce(
 
     monkeypatch.setattr("sentry.tasks.relay.update_config_cache.apply_async", apply_async)
 
-    debounce_cache.mark_task_done(None, default_project.id, None)
     schedule_update_config_cache(generate=True, project_id=default_project.id)
     schedule_update_config_cache(generate=False, project_id=default_project.id)
 
-    debounce_cache.mark_task_done(None, None, default_organization.id)
     schedule_update_config_cache(generate=True, organization_id=default_organization.id)
     schedule_update_config_cache(generate=False, organization_id=default_organization.id)
 
@@ -112,7 +95,6 @@ def test_generate(
     task_runner,
     entire_organization,
     redis_cache,
-    always_update_cache,
 ):
     assert not redis_cache.get(default_projectkey.public_key)
 
@@ -148,7 +130,6 @@ def test_invalidate(
     task_runner,
     entire_organization,
     redis_cache,
-    always_update_cache,
 ):
 
     cfg = {"foo": "bar"}
@@ -220,7 +201,10 @@ def test_projectkeys(default_project, task_runner, redis_cache):
         pk.save()
 
     for key in deleted_pks:
-        assert redis_cache.get(key.public_key) == {"disabled": True}
+        # XXX: Ideally we would write `{"disabled": True}` into Redis, however
+        # it's fine if we don't and instead Relay starts hitting the endpoint
+        # which will write this for us.
+        assert not redis_cache.get(key.public_key)
 
     (pk_json,) = redis_cache.get(pk.public_key)["publicKeys"]
     assert pk_json["publicKey"] == pk.public_key
@@ -234,7 +218,7 @@ def test_projectkeys(default_project, task_runner, redis_cache):
     with task_runner():
         pk.delete()
 
-    assert redis_cache.get(pk.public_key) == {"disabled": True}
+    assert not redis_cache.get(pk.public_key)
 
     for key in ProjectKey.objects.filter(project_id=default_project.id):
         assert not redis_cache.get(key.public_key)