Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add permanent environment document cache #5187

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: add permanent environment document cache #5187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f18d0d7
Add permanent environment document cache
matthewelwell Mar 5, 2025
b9ef7a2
Add ENVIRONMENT_DOCUMENT_CACHE_MODE setting
matthewelwell Mar 7, 2025
6ab3b22
Fix logic and typing
matthewelwell Mar 13, 2025
f134ebe
Write basic integration test
matthewelwell Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions api/app/settings/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,8 +679,24 @@
"django.core.cache.backends.locmem.LocMemCache",
)

CACHE_ENVIRONMENT_DOCUMENT_SECONDS = env.int("CACHE_ENVIRONMENT_DOCUMENT_SECONDS", 0)
ENVIRONMENT_DOCUMENT_CACHE_LOCATION = "environment-documents"
ENVIRONMENT_DOCUMENT_CACHE_BACKEND = env(
"ENVIRONMENT_DOCUMENT_CACHE_BACKEND", "django.core.cache.backends.db.DatabaseCache"
)
EXPIRE_ENVIRONMENT_DOCUMENT_CACHE = env.bool("EXPIRE_ENVIRONMENT_DOCUMENT_CACHE", True)

if not EXPIRE_ENVIRONMENT_DOCUMENT_CACHE:
if "CACHE_ENVIRONMENT_DOCUMENT_SECONDS" in os.environ:
warnings.warn(
"Ignoring CACHE_ENVIRONMENT_DOCUMENT_SECONDS variable "
"since EXPIRE_ENVIRONMENT_DOCUMENT_CACHE is False"
)
CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT = None
else:
CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT = env.int(
"CACHE_ENVIRONMENT_DOCUMENT_SECONDS", 0
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between EXPIRE_ENVIRONMENT_DOCUMENT_CACHE=False and CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT=0? Also, why change a good name (_SECONDS suffix) to a worse one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between EXPIRE_ENVIRONMENT_DOCUMENT_CACHE=False and CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT=0?

Well, technically, lots! See here - they're distinct opposites. If we were to assume that 0 meant cache indefinitely then we'd be completely changing the default behaviour (which is to not cache at all). Admittedly, we have logic elsewhere in the app that only inspects the cache if the value is greater than 0 (see here), so we could modify that to our will, but I'd rather keep it such that the settings make sense on their own.

Also, why change a good name (_SECONDS suffix) to a worse one?

Yeah, this is a good question. My logic here was that None is now a valid value for the variable which, to me, meant that _TIMEOUT made more sense - I don't feel particularly strongly about this though. Note that I did not change the environment variable name, only the setting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come up with the following options to consider:

  1. Accept -1 as a CACHE_ENVIRONMENT_DOCUMENT_SECONDS value that means "never expire the cache". This will simplify business logic and hopefully ease cognitive load. We have a lot of settings already!
  2. Add an CACHE_ENVIRONMENT_DOCUMENT_MODE: Literal["PERSISTENT", "EXPIRING"] = env.str(default="EXPIRING"). This introduces a new setting but it's more expressive and easy to understand.

In any case, I expect the new/modified settings to be reflected in the docs.

My logic here was that None is now a valid value for the variable

I think it can be just left at 0 (or -1 if we go with option 1) .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the logic in line with your second suggestion 👍



USER_THROTTLE_CACHE_NAME = "user-throttle"
USER_THROTTLE_CACHE_BACKEND = env.str(
Expand Down Expand Up @@ -736,9 +752,9 @@
"TIMEOUT": 12 * 60 * 60, # 12 hours
},
ENVIRONMENT_DOCUMENT_CACHE_LOCATION: {
"BACKEND": "django.core.cache.backends.db.DatabaseCache",
"BACKEND": ENVIRONMENT_DOCUMENT_CACHE_BACKEND,
"LOCATION": ENVIRONMENT_DOCUMENT_CACHE_LOCATION,
"timeout": CACHE_ENVIRONMENT_DOCUMENT_SECONDS,
"TIMEOUT": CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT,
},
GET_FLAGS_ENDPOINT_CACHE_NAME: {
"BACKEND": GET_FLAGS_ENDPOINT_CACHE_BACKEND,
Expand Down
35 changes: 26 additions & 9 deletions api/environments/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from rest_framework.request import Request
from softdelete.models import SoftDeleteObject # type: ignore[import-untyped]

from app.utils import create_hash
from app.utils import create_hash, is_saas
from audit.constants import (
ENVIRONMENT_CREATED_MESSAGE,
ENVIRONMENT_UPDATED_MESSAGE,
Expand All @@ -45,7 +45,10 @@
from metadata.models import Metadata
from projects.models import Project
from segments.models import Segment
from util.mappers import map_environment_to_sdk_document
from util.mappers import (
map_environment_to_environment_document,
map_environment_to_sdk_document,
)
from webhooks.models import AbstractBaseExportableWebhookModel

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -245,7 +248,7 @@ def get_from_cache(cls, api_key): # type: ignore[no-untyped-def]
logger.info("Environment with api_key %s does not exist" % api_key)

@classmethod
def write_environments_to_dynamodb(
def write_environment_documents(
cls,
environment_id: int = None, # type: ignore[assignment]
project_id: int = None, # type: ignore[assignment]
Expand Down Expand Up @@ -288,10 +291,21 @@ def write_environments_to_dynamodb(
if not all([project, project.enable_dynamo_db, environment_wrapper.is_enabled]): # type: ignore[union-attr]
return

environment_wrapper.write_environments(environments)

if project.edge_v2_environments_migrated and environment_v2_wrapper.is_enabled: # type: ignore[union-attr]
environment_v2_wrapper.write_environments(environments)
if is_saas():
environment_wrapper.write_environments(environments)

if (
project.edge_v2_environments_migrated
and environment_v2_wrapper.is_enabled
): # type: ignore[union-attr]
environment_v2_wrapper.write_environments(environments)
elif not settings.EXPIRE_ENVIRONMENT_DOCUMENT_CACHE:
environment_document_cache.set_many(
{
e.api_key: map_environment_to_environment_document(e)
for e in environments
}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe the is_saas() check belongs here. I'd rather allow our users to employ their Dynamo in the future, e.g., by writing a Dynamo cache backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I somewhat agree with this. Let me see if I can refactor this (and the environment variables as per the above conversation).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this a bit more, I think my concern here is that this implies a much larger refactor.

I guess the ideal solution would be to refit our SaaS platform to essentially have something like:

ENVIRONMENT_DOCUMENT_CACHE_MODE: PERSISTENT
ENVIRONMENT_DOCUMENT_CACHE_BACKEND: cache.backends.custom.dynamodb

where cache.backends.custom.dynamodb is our own implementation of the django cache backend which handles the interactions with dynamodb.

While the prospect of this certainly excites me, I don't think it should necessarily be in scope for this piece of work.

An option to solve your direct comment here would be to remove the call to is_saas() and instead use some other setting, perhaps derived from whether e.g. ENVIRONMENTS_TABLE_NAME_DYNAMO is set.

Thoughts @khvn26?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually found a half decent halfway solution as there was already a conditional check in the code that was only relevant to the dynamodb path, so I've just used that as the check instead of is_saas for now. I still think the custom cache backend would be the best option, and may well simplify a lot of the dynamo integration code.


def get_feature_state(
self,
Expand Down Expand Up @@ -364,8 +378,11 @@ def get_environment_document(
cls,
api_key: str,
) -> dict[str, typing.Any]:
if settings.CACHE_ENVIRONMENT_DOCUMENT_SECONDS > 0:
return cls._get_environment_document_from_cache(api_key)
if (
settings.CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT > 0
or not settings.EXPIRE_ENVIRONMENT_DOCUMENT_CACHE
):
cls._get_environment_document_from_cache(api_key)
return cls._get_environment_document_from_db(api_key)

def get_create_log_message(self, history_instance) -> typing.Optional[str]: # type: ignore[no-untyped-def]
Expand Down
4 changes: 2 additions & 2 deletions api/environments/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,15 @@

@register_task_handler(priority=TaskPriority.HIGH) # type: ignore[misc]
def rebuild_environment_document(environment_id: int) -> None:
Environment.write_environments_to_dynamodb(environment_id=environment_id)
Environment.write_environment_documents(environment_id=environment_id)


@register_task_handler(priority=TaskPriority.HIGHEST) # type: ignore[misc]
def process_environment_update(audit_log_id: int): # type: ignore[no-untyped-def]
audit_log = AuditLog.objects.get(id=audit_log_id)

# Send environment document to dynamodb
Environment.write_environments_to_dynamodb(
Environment.write_environment_documents(
environment_id=audit_log.environment_id, project_id=audit_log.project_id
)

Expand Down
2 changes: 1 addition & 1 deletion api/integrations/common/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def write_environment_to_dynamodb(self): # type: ignore[no-untyped-def]
self.__class__.__name__,
)
return
Environment.write_environments_to_dynamodb(environment_id=self.environment_id)
Environment.write_environment_documents(environment_id=self.environment_id)

@hook(AFTER_UPDATE)
def clear_environment_cache(self): # type: ignore[no-untyped-def]
Expand Down
2 changes: 1 addition & 1 deletion api/integrations/webhook/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ class WebhookConfiguration(
@hook(AFTER_SAVE)
@hook(AFTER_DELETE)
def write_environment_to_dynamodb(self): # type: ignore[no-untyped-def]
Environment.write_environments_to_dynamodb(environment_id=self.environment_id)
Environment.write_environment_documents(environment_id=self.environment_id)
2 changes: 1 addition & 1 deletion api/projects/tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
def write_environments_to_dynamodb(project_id: int) -> None:
from environments.models import Environment

Environment.write_environments_to_dynamodb(project_id=project_id)
Environment.write_environment_documents(project_id=project_id)


@register_task_handler() # type: ignore[misc]
Expand Down
16 changes: 8 additions & 8 deletions api/tests/unit/environments/test_unit_environments_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ def test_write_environments_to_dynamodb_with_environment( # type: ignore[no-unt
mock_dynamo_env_wrapper.reset_mock()

# When
Environment.write_environments_to_dynamodb(
Environment.write_environment_documents(
environment_id=dynamo_enabled_project_environment_one.id
)

Expand All @@ -465,7 +465,7 @@ def test_write_environments_to_dynamodb_project( # type: ignore[no-untyped-def]
mock_dynamo_env_wrapper.reset_mock()

# When
Environment.write_environments_to_dynamodb(project_id=dynamo_enabled_project.id)
Environment.write_environment_documents(project_id=dynamo_enabled_project.id)

# Then
args, kwargs = mock_dynamo_env_wrapper.write_environments.call_args
Expand All @@ -485,7 +485,7 @@ def test_write_environments_to_dynamodb_with_environment_and_project( # type: i
mock_dynamo_env_wrapper.reset_mock()

# When
Environment.write_environments_to_dynamodb(
Environment.write_environment_documents(
environment_id=dynamo_enabled_project_environment_one.id
)

Expand All @@ -512,7 +512,7 @@ def test_write_environments_to_dynamodb__project_environments_v2_migrated__call_
mock_dynamo_env_v2_wrapper.is_enabled = True

# When
Environment.write_environments_to_dynamodb(project_id=dynamo_enabled_project.id)
Environment.write_environment_documents(project_id=dynamo_enabled_project.id)

# Then
args, kwargs = mock_dynamo_env_v2_wrapper.write_environments.call_args
Expand All @@ -536,7 +536,7 @@ def test_write_environments_to_dynamodb__project_environments_v2_migrated__wrapp
dynamo_enabled_project.save()

# When
Environment.write_environments_to_dynamodb(project_id=dynamo_enabled_project.id)
Environment.write_environment_documents(project_id=dynamo_enabled_project.id)

# Then
mock_dynamo_env_v2_wrapper.write_environments.assert_not_called()
Expand All @@ -563,7 +563,7 @@ def test_write_environments_to_dynamodb__project_environments_v2_not_migrated__w
mock_dynamo_env_v2_wrapper.is_enabled = True

# When
Environment.write_environments_to_dynamodb(project_id=dynamo_enabled_project.id)
Environment.write_environment_documents(project_id=dynamo_enabled_project.id)

# Then
mock_dynamo_env_v2_wrapper.write_environments.assert_not_called()
Expand Down Expand Up @@ -663,7 +663,7 @@ def test_environment_get_environment_document_with_caching_when_document_in_cach
environment, django_assert_num_queries, settings, mocker
):
# Given
settings.CACHE_ENVIRONMENT_DOCUMENT_SECONDS = 60
settings.CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT = 60

mocked_environment_document_cache = mocker.patch(
"environments.models.environment_document_cache"
Expand All @@ -685,7 +685,7 @@ def test_environment_get_environment_document_with_caching_when_document_not_in_
environment, django_assert_num_queries, settings, mocker
):
# Given
settings.CACHE_ENVIRONMENT_DOCUMENT_SECONDS = 60
settings.CACHE_ENVIRONMENT_DOCUMENT_TIMEOUT = 60

mocked_environment_document_cache = mocker.patch(
"environments.models.environment_document_cache"
Expand Down
4 changes: 2 additions & 2 deletions api/tests/unit/environments/test_unit_environments_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def test_process_environment_update_with_environment_audit_log(environment, mock
process_environment_update(audit_log_id=audit_log.id)

# Then
mock_environment_model_class.write_environments_to_dynamodb.assert_called_once_with(
mock_environment_model_class.write_environment_documents.assert_called_once_with(
environment_id=environment.id, project_id=environment.project.id
)
mock_send_environment_update_message_for_environment.assert_called_once_with(
Expand Down Expand Up @@ -76,7 +76,7 @@ def test_process_environment_update_with_project_audit_log(environment, mocker):
process_environment_update(audit_log_id=audit_log.id)

# Then
mock_environment_model_class.write_environments_to_dynamodb.assert_called_once_with(
mock_environment_model_class.write_environment_documents.assert_called_once_with(
environment_id=None, project_id=environment.project.id
)
mock_send_environment_update_message_for_environment.assert_not_called()
Expand Down
4 changes: 2 additions & 2 deletions api/tests/unit/integrations/amplitude/test_unit_amplitude_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ def test_amplitude_configuration_save_writes_environment_to_dynamodb( # type: i
amplitude_config.save()

# Then
mock_environment_model_class.write_environments_to_dynamodb.assert_called_once_with(
mock_environment_model_class.write_environment_documents.assert_called_once_with(
environment_id=environment.id
)

Expand All @@ -43,7 +43,7 @@ def test_amplitude_configuration_delete_writes_environment_to_dynamodb( # type:
amplitude_config.delete()

# Then
mock_environment_model_class.write_environments_to_dynamodb.assert_called_once_with(
mock_environment_model_class.write_environment_documents.assert_called_once_with(
environment_id=environment.id
)

Expand Down
Loading