feat: Cached tentant record for context #2025
Conversation
Signed-off-by: Sagar Khole <[email protected]>
|
Signed-off-by: Sagar Khole <[email protected]>
sagarkhole4
changed the title
| // TODO: maybe we can look at not having to retrieve the tenant record if there's already a context available. | ||
| this.logger.debug('TenantRecord not found in cache') | ||
| tenantRecord = await this.tenantRecordService.getTenantById(this.rootAgentContext, contextCorrelationId) | ||
| await cache.set(this.rootAgentContext, `contextCorrelationId-${contextCorrelationId}`, tenantRecord) |
There was a problem hiding this comment.
I think we should only put the tenant record in the cache after we have run the checks for agent storage updates etc.. (so after getContextForSession, and before return the agent context)
There was a problem hiding this comment.
I appreciate your suggestion. In line with that, I modified the code.
| `contextCorrelationId-${contextCorrelationId}` | ||
| ) | ||
| if (!tenantRecord) { | ||
| // TODO: maybe we can look at not having to retrieve the tenant record if there's already a context available. |
| ) | ||
| if (!tenantRecord) { | ||
| // TODO: maybe we can look at not having to retrieve the tenant record if there's already a context available. | ||
| this.logger.debug('TenantRecord not found in cache') |
There was a problem hiding this comment.
It would be helpful to add the ids etc.. to the log messages. So TenantRecord with id ${contextCorrelationId} not found in cache (same for the other log messages)
| container.registerInstance(CacheModuleConfig, { | ||
| cache: new CacheModule({ | ||
| cache: new InMemoryLruCache({ limit: 100 }), | ||
| }).config.cache as unknown as CacheModule, | ||
| }) |
There was a problem hiding this comment.
I think you can simplify this
Does this work?
| container.registerInstance(CacheModuleConfig, { | |
| cache: new CacheModule({ | |
| cache: new InMemoryLruCache({ limit: 100 }), | |
| }).config.cache as unknown as CacheModule, | |
| }) | |
| container.registerInstance(CacheModuleConfig, new CacheModuleConfig({ | |
| cache: new InMemoryLruCache({ limit: 100 }) | |
| }) |
There was a problem hiding this comment.
Yes, This test case is working I have tested it on my local machine.
Signed-off-by: Sagar Khole <[email protected]>
Signed-off-by: Sagar Khole <[email protected]>
Head branch was pushed to by a user without write access
…e before each test Signed-off-by: Sagar Khole <[email protected]>
|
It seems the lockfile is out of date, and you have to update it locally. ( i think running pnpm install should be enough) |
Signed-off-by: Sagar Khole <[email protected]>
|
executed pnpm install and committed the pnpm-lock file. |
|
Tests are now failing as removing a tenant does not remove it from the cache. I think we should remove the tenant record from the cache if a tenant is deleted. I think for every update to the tenant record, we should update the entry in the cache. And finally, we store the wallet key for the tenant in the wallet record, and so putting it in the cache is not safe I think. Any thoughts on that? |
|
I think we should maybe only use caching if using Askar profile as multitenant scheme, as in that case the wallet key is not used. But otherwise I'm not sure it's safe to store the wallet key in the cache |
I agree that we should ensure the tenant record is removed from the cache when a tenant is deleted. To address this, we can delete the cached record just before the deletion or update operation is performed. This way, the cache stays in sync with the actual database state. Regarding the storage of sensitive data like wallet keys, you're right—storing this kind of information directly in the cache could pose a security risk. To mitigate this, we could consider encrypting the data before caching it, ensuring that any sensitive information remains secure. As fetching data from the database can be resource-intensive, especially in a production environment, caching is a valuable mechanism to enhance performance. By keeping the cache updated efficiently, we can reduce database load and improve response times. |
The getAgentContextForContextCorrelationId function caches the tenant record to avoid the expensive operation of retrieving it from the database each time.