twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
15.7k stars 1.7k forks source link

Transform featureFlags into configFlags #4562

Closed FelixMalfait closed 4 days ago

FelixMalfait commented 5 months ago

Context

The line between feature flag and environment variables is blurry. Ex: If I set a config var like FREE_TRIAL_LENGTH, maybe I will want to overwrite it at the user-level? If I create a variable GOOGLE_API_KEY maybe I want people who self-host to be able to set its value directly from the admin panel, etc. Is IS_BILLING_ENABLED a feature flag or a config?

See how Flagsmith blends both concepts under a single object:

Screenshot 2024-03-18 at 17 05 42 Screenshot 2024-03-18 at 17 05 51

As of today featureFlags are used:

And environment variables are used:

Proposal

We need to extend FeatureFlag to become ConfigFlag, that means:

Steps:

Use-cases

capaj commented 5 months ago

it should cache the database values efficiently, we want to make sure we don't add a big performance overhead but put this in cache and clear the cache wisely**

How exactly do you want the caching to be implemented? Just in memory cache to avoid going into the DB every time? How long should it be cached for? 5 minutes for example?

FelixMalfait commented 5 months ago

@capaj Tbh I didn't really think about it in details while writing the issue. The issue with in-memory is that it would be hard to invalidate it. We have a cache service powered by redis, that could make invalidation easier, but it won't be as efficient as in-memory. So maybe we should just do in-memory and set 5 minutes yes (forgetting about cache invalidation then? what do you think? we'll need it when we do the admin panel but can maybe afford to do it just in-memory as long as we have 1 server only it will work / for people who self-host...) It's the kind of issue that is not straightforward and will probably requirement judgement to adapt and not follow exactly. I also edited to add a refactoring part on clientConfig, where I gave a direction but not 100% sure it will need a bit of exploration to make the best choices

capaj commented 5 months ago

I can make it async and configurable. Default to in memory, but you could just supply a redis URL in env var to use redis. Redis is perfect as caching layer.

When you say cache service-do you mean something like upstash? I was looking into deps earlier and there is no redis client in deps.

FelixMalfait commented 5 months ago

@capaj No I meant that we already have an abstraction layer called "Cache Storage . Service" you can find the file in the project. Env var is REDIS_HOST

capaj commented 5 months ago

can you link me to where it is defined? I just searched for getRedisHost usages and it's only used in one place in https://github.com/twentyhq/twenty/blob/735e75b3b1d172feff9f0c41ffd1cb4afe32375e/packages/twenty-server/src/integrations/message-queue/message-queue.module-factory.ts#L35

FelixMalfait commented 5 months ago

https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/engine/integrations/cache-storage/cache-storage.service.ts https://github.com/twentyhq/twenty/blob/main/packages/twenty-server/src/engine/integrations/cache-storage/cache-storage.module.ts https://docs.nestjs.com/techniques/caching

The file you're looking at is not up-to-date btw (not on main branch anymore)

FelixMalfait commented 5 months ago

What we could do eventually is put an additional layer for the config so that the first time we ask for a config flag it's retrieved from redis but all flags are stored in a var, so that if we access other feature flag/config values in the same request, it takes the in-memory value and not do other redis calls

saadfrhan commented 4 months ago

Hi @FelixMalfait, can you assign me this issue? I will try to solve it.

FelixMalfait commented 4 months ago

Thanks @saadfrhan ; @capaj are you working on it already?

capaj commented 4 months ago

No, I have not started on this. Assign this to anyone else. I was only considering it.

On Tue, 16 Apr 2024, 09:45 Félix Malfait, @.***> wrote:

Thanks @saadfrhan https://github.com/saadfrhan ; @capaj https://github.com/capaj are you working on it already?

— Reply to this email directly, view it on GitHub https://github.com/twentyhq/twenty/issues/4562#issuecomment-2058448716, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJ6WITSI6C3AEJSW3VXT4DY5TJILAVCNFSM6AAAAABE36XILOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYGQ2DQNZRGY . You are receiving this because you were mentioned.Message ID: @.***>

FelixMalfait commented 4 months ago

Hey @saadfrhan this is not an easy one. Ping me on Discord if you need guidance! Thanks

saadfrhan commented 4 months ago

Yes. I will ping you if I stuck somewhere. Thanks!

saadfrhan commented 4 months ago

I apologize. I thought I could solve it.