feat: RemoteConfig don't sync if there are no changes

posthog/api/test/test_decide.py

@@ -123,7 +123,8 @@ def _post_decide(
         if self.use_remote_config:
             # We test a lot with settings changes so the idea is to refresh the remote config
             remote_config = RemoteConfig.objects.get(team=self.team)
-            remote_config.sync()
+            # Force as sync as lots of the tests are clearing redis purposefully which messes with things
+            remote_config.sync(force=True)
 
         if groups is None:
             groups = {}

posthog/models/remote_config.py

@@ -1,7 +1,6 @@
 import json
 import os
 from typing import Any, Optional
-from collections.abc import Callable
 from django.conf import settings
 from django.db import models
 from django.http import HttpRequest
@@ -69,8 +68,8 @@ def indent_js(js_content: str, indent: int = 4) -> str:
     return joined
 
 
-def cache_key_for_team_token(team_token: str, suffix: str) -> str:
-    return f"remote_config/{team_token}/{suffix}"
+def cache_key_for_team_token(team_token: str) -> str:
+    return f"remote_config/{team_token}/config"
 
 
 def sanitize_config_for_public_cdn(config: dict, request: Optional[HttpRequest] = None) -> dict:
@@ -291,10 +290,8 @@ def _build_site_apps_js(self):
         return site_apps_js + site_functions_js
 
     @classmethod
-    def _get_via_cache(
-        cls, token: str, suffix: str, fn: Callable[["RemoteConfig"], dict | str], timeout: int = CACHE_TIMEOUT
-    ) -> Any:
-        key = cache_key_for_team_token(token, suffix)
+    def _get_config_via_cache(cls, token: str) -> dict:
+        key = cache_key_for_team_token(token)
 
         data = cache.get(key)
         if data == "404":
@@ -309,25 +306,25 @@ def _get_via_cache(
         try:
             remote_config = cls.objects.select_related("team").get(team__api_token=token)
         except cls.DoesNotExist:
-            cache.set(key, "404", timeout=timeout)
+            cache.set(key, "404", timeout=CACHE_TIMEOUT)
             REMOTE_CONFIG_CACHE_COUNTER.labels(result="miss_but_missing").inc()
             raise
 
-        data = fn(remote_config)
-        cache.set(key, data, timeout=timeout)
+        data = remote_config.build_config()
+        cache.set(key, data, timeout=CACHE_TIMEOUT)
 
         return data
 
     @classmethod
     def get_config_via_token(cls, token: str, request: Optional[HttpRequest] = None) -> dict:
-        config = cls._get_via_cache(token, "config", lambda remote_config: remote_config.build_config())
+        config = cls._get_config_via_cache(token)
         config = sanitize_config_for_public_cdn(config, request=request)
 
         return config
 
     @classmethod
     def get_config_js_via_token(cls, token: str, request: Optional[HttpRequest] = None) -> str:
-        config = cls._get_via_cache(token, "config", lambda remote_config: remote_config.build_config())
+        config = cls._get_config_via_cache(token)
         # Get the site apps JS so we can render it in the JS
         site_apps_js = config.pop("siteAppsJS", None)
         # We don't want to include the minimal site apps content as we have the JS now
@@ -352,7 +349,7 @@ def get_array_js_via_token(cls, token: str, request: Optional[HttpRequest] = Non
 
         return f"""{get_array_js_content()}\n\n{js_content}"""
 
-    def sync(self):
+    def sync(self, force: bool = False):
         """
         When called we sync to any configured CDNs as well as redis for the /decide endpoint
         """
@@ -361,16 +358,21 @@ def sync(self):
 
         try:
             config = self.build_config()
-            self.config = config
-
-            cache.set(cache_key_for_team_token(self.team.api_token, "config"), config, timeout=CACHE_TIMEOUT)
 
-            self._purge_cdn()
+            if not force and config == self.config:
+                CELERY_TASK_REMOTE_CONFIG_SYNC.labels(result="no_changes").inc()
+                logger.info(f"RemoteConfig for team {self.team_id} is unchanged")
+                return
 
-            # TODO: Invalidate caches - in particular this will be the Cloudflare CDN cache
+            self.config = config
             self.synced_at = timezone.now()
             self.save()
 
+            # Update the redis cache key for the config
+            cache.set(cache_key_for_team_token(self.team.api_token), config, timeout=CACHE_TIMEOUT)
+            # Invalidate Cloudflare CDN cache
+            self._purge_cdn()
+
             CELERY_TASK_REMOTE_CONFIG_SYNC.labels(result="success").inc()
         except Exception as e:
             capture_exception(e)

posthog/models/test/test_hog_function.py

@@ -275,8 +275,8 @@ def test_hog_functions_reload_on_team_saved(self):
             {"key": "$pageview", "operator": "regex", "value": "test"},
         ]
         # 1 update team, 1 load hog functions, 1 update hog functions
-        # 8 unrelated due to RemoteConfig refresh
-        with self.assertNumQueries(3 + 8):
+        # 7 unrelated due to RemoteConfig refresh
+        with self.assertNumQueries(3 + 7):
             self.team.save()
         hog_function_1.refresh_from_db()
         hog_function_2.refresh_from_db()

posthog/models/test/test_remote_config.py

@@ -280,7 +280,7 @@ def setUp(self):
         super().setUp()
         self.remote_config.refresh_from_db()
         # Clear the cache so we are properly testing each flow
-        assert cache.delete(cache_key_for_team_token(self.team.api_token, "config"))
+        assert cache.delete(cache_key_for_team_token(self.team.api_token))
 
     def _assert_matches_config(self, data):
         assert data == snapshot(
@@ -348,10 +348,15 @@ def test_syncs_if_changes(self):
         self.remote_config.sync()
         assert synced_at < self.remote_config.synced_at  # type: ignore
 
+    def test_does_not_syncs_if_no_changes(self):
+        synced_at = self.remote_config.synced_at
+        self.remote_config.sync()
+        assert synced_at == self.remote_config.synced_at
+
     def test_persists_data_to_redis_on_sync(self):
         self.remote_config.config["surveys"] = True
         self.remote_config.sync()
-        assert cache.get(cache_key_for_team_token(self.team.api_token, "config"))
+        assert cache.get(cache_key_for_team_token(self.team.api_token))
 
     def test_gets_via_redis_cache(self):
         with self.assertNumQueries(CONFIG_REFRESH_QUERY_COUNT):
@@ -453,6 +458,8 @@ def test_purges_cdn_cache_on_sync(self, mock_post):
             REMOTE_CONFIG_CDN_PURGE_TOKEN="MY_TOKEN",
             REMOTE_CONFIG_CDN_PURGE_DOMAINS=["cdn.posthog.com", "https://cdn2.posthog.com"],
         ):
+            # Force a change to the config
+            self.remote_config.config["token"] = "NOT"
             self.remote_config.sync()
             mock_post.assert_called_once_with(
                 "https://api.cloudflare.com/client/v4/zones/MY_ZONE_ID/purge_cache",

posthog/tasks/test/test_remote_config.py

@@ -12,20 +12,42 @@ def setUp(self) -> None:
             organization=self.organization,
             name="Test project",
         )
-        self.other_team = team
+        self.other_team_1 = team
+
+        project, team = Project.objects.create_with_team(
+            initiating_user=self.user,
+            organization=self.organization,
+            name="Test project 2",
+        )
+        self.other_team_2 = team
 
     def test_sync_task_syncs_all_remote_configs(self) -> None:
         # Delete one teams config
-        RemoteConfig.objects.get(team=self.other_team).delete()
+        remote_config_deleted = RemoteConfig.objects.get(team=self.team)
+        remote_config_deleted_synced_at = remote_config_deleted.synced_at
+        remote_config_deleted.delete()
+
         configs = RemoteConfig.objects.all()
-        assert len(configs) == 1
+        assert len(configs) == 2
 
-        last_synced_at = RemoteConfig.objects.get(team=self.team).synced_at
+        # Modify the other team's config (indicate something didn't get synced properly)
+        remote_config_1 = RemoteConfig.objects.get(team=self.other_team_1)
+        remote_config_1.config["token"] = "MODIFIED"
+        remote_config_1.save()
+        remote_config_1_synced_at = remote_config_1.synced_at
+
+        # No modifications to this one
+        remote_config_2 = RemoteConfig.objects.get(team=self.other_team_2)
+        remote_config_2_synced_at = remote_config_2.synced_at
 
         sync_all_remote_configs()
 
         configs = RemoteConfig.objects.all()
-        assert len(configs) == 2
+        assert len(configs) == 3
 
-        assert RemoteConfig.objects.get(team=self.other_team).synced_at > last_synced_at  # type: ignore
-        assert RemoteConfig.objects.get(team=self.team).synced_at > last_synced_at  # type: ignore
+        # This one is deleted so should be synced
+        assert RemoteConfig.objects.get(team=self.team).synced_at > remote_config_deleted_synced_at  # type: ignore
+        # This one is modified so should be synced
+        assert RemoteConfig.objects.get(team=self.other_team_1).synced_at > remote_config_1_synced_at  # type: ignore
+        # This one is unchanged so should not be synced
+        assert RemoteConfig.objects.get(team=self.other_team_2).synced_at == remote_config_2_synced_at