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.
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
[Task]: Implement Redis caching for tenant configuration #33083
Open
Labels
Integrations Pod General
Issues related to the Integrations Pod that don't fit into other tags.
Integrations Pod
Issues related to a specific integration
Task
A simple Todo
Comments
NilanshBansal
added
the
Integrations Pod General
Issues related to the Integrations Pod that don't fit into other tags.
label
May 1, 2024
github-actions
bot
added
the
Integrations Pod
Issues related to a specific integration
label
May 1, 2024
NilanshBansal
changed the title
[Task]: Check feasibility for caching the whole tenant config and updating it on changes
[Task]: Implement Redis caching for tenant configuration
May 1, 2024
Picking this up after the release blocker issue gets resolved |
2 tasks
NilanshBansal
added a commit
that referenced
this issue
May 9, 2024
NilanshBansal
added a commit
that referenced
this issue
May 15, 2024
## Description The tenant is fetched multiple times across the appsmith codebase but is rarely updated (from the admin settings). Every time a fetch call to the database is costly both in terms of resources and time taken. The consolidated api also makes a call to fetch the tenant and return to the client. To improve the performance of fetching the tenant information, we are moving the tenant information to redis cache for quicker fetch. This will improve the performance of the consolidated api and also reduce the time taken by all the different functionalities within the backend codebase which depend on tenant to process further. **TL;DR** Adds tenant information `tenantService.getDefaultTenant()` to redis. Fixes #33083 ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9079438120> > Commit: 306e77f > Cypress dashboard url: <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9079438120&attempt=2" target="_blank">Click here!</a> <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No --------- Co-authored-by: Arpit Mohan <mohanarpit@users.noreply.github.com>
Reopening as the task had to be reverted. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Integrations Pod General
Issues related to the Integrations Pod that don't fit into other tags.
Integrations Pod
Issues related to a specific integration
Task
A simple Todo
Description
The tenants/current api is the slowest component of consolidated/view api. Reducing the latency of tenants/current api will reduce the latency of backend in view experience for all users.
Relevant Slack thread : https://theappsmith.slack.com/archives/C024GUDM0LT/p1713933688987279
In the diagram in the slack thread, the blue line just below the magenta line at the top is the contribution of tenants/current to consolidated api. The diagram shows that tenants/current is 90% contributor to consolidated view api.
Upon debugging the cause of latency from tenants/current, it is found that the api makes a DB call to fetch the tenant configuration every time.
Caching the tenant config ID is already present in the codebase. Extending that approach, we can cache the whole tenant config and update it when a user changes admin settings.
Not making the DB call will reduce the latency of tenants/current api and decrease the latency of consolidated view api as well.
Subtasks
The text was updated successfully, but these errors were encountered: