feat: add permanent environment document cache #5187
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
github-actions
bot
added
api
Uffizzi Ephemeral Environment
|
api/app/settings/common.py
Outdated
| 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 | ||
| ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I've come up with the following options to consider:
- Accept
-1as aCACHE_ENVIRONMENT_DOCUMENT_SECONDSvalue that means "never expire the cache". This will simplify business logic and hopefully ease cognitive load. We have a lot of settings already! - 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) .
There was a problem hiding this comment.
Updated the logic in line with your second suggestion 👍
api/environments/models.py
Outdated
| 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 | ||
| } | ||
| ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm... I somewhat agree with this. Let me see if I can refactor this (and the environment variables as per the above conversation).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
Changes
This PR sets out to add a permanent cache of the environment document, similar to how our SaaS environment works where it writes the document to dynamodb. This updates the current logic for caching the environment document, but hooks it into the same logic that updates the document in dynamodb for our SaaS platform. It also makes the cache backend customisable to allow self hosted installs to use, e.g. redis to cache the document.
How did you test this code?
TODO