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.

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
NilanshBansal opened this issue May 1, 2024 · 2 comments · Fixed by #33309 · May be fixed by #33641
Open

[Task]: Implement Redis caching for tenant configuration #33083

NilanshBansal opened this issue May 1, 2024 · 2 comments · Fixed by #33309 · May be fixed by #33641
Assignees
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
Copy link
Contributor

NilanshBansal commented May 1, 2024

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

  • Implement caching for fetching tenant configuration
  • Invalidate cache if tenant configuration gets updated, update the database and rebuild cache.
@NilanshBansal NilanshBansal added the Task A simple Todo label May 1, 2024
@NilanshBansal NilanshBansal self-assigned this May 1, 2024
@NilanshBansal 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 github-actions bot added the Integrations Pod Issues related to a specific integration label May 1, 2024
@NilanshBansal 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
@NilanshBansal
Copy link
Contributor Author

Picking this up after the release blocker issue gets resolved
Ref: https://theappsmith.slack.com/archives/CTHN8GX5Y/p1714974437746959
cc: @rajatagrawal @rohan-arthur

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>
@NilanshBansal
Copy link
Contributor Author

Reopening as the task had to be reverted.
Original PR: #33309
Revert PR: #33512
Reason: https://theappsmith.slack.com/archives/C02K2MZERSL/p1715838779182389?thread_ts=1715829340.668969&cid=C02K2MZERSL

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
Projects
None yet
1 participant