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

feat: Cached tentant record for context #2025

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions packages/tenants/src/context/TenantAgentContextProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
isValidJweStructure,
JsonEncoder,
isJsonObject,
CacheModuleConfig,
} from '@credo-ts/core'

import { TenantAgent } from '../TenantAgent'
Expand Down Expand Up @@ -55,13 +56,26 @@ export class TenantAgentContextProvider implements AgentContextProvider {
}

public async getAgentContextForContextCorrelationId(contextCorrelationId: string) {
this.logger.debug('Inside getAgentContextForContextCorrelationId')
// It could be that the root agent context is requested, in that case we return the root agent context
if (contextCorrelationId === this.rootAgentContext.contextCorrelationId) {
return this.rootAgentContext
}

// TODO: maybe we can look at not having to retrieve the tenant record if there's already a context available.
const tenantRecord = await this.tenantRecordService.getTenantById(this.rootAgentContext, contextCorrelationId)
const cache = this.rootAgentContext.dependencyManager.resolve(CacheModuleConfig).cache

this.logger.debug('Getting tenantRecord from cache')
let tenantRecord: TenantRecord | null = await cache.get(
this.rootAgentContext,
`contextCorrelationId-${contextCorrelationId}`
)
if (!tenantRecord) {
// TODO: maybe we can look at not having to retrieve the tenant record if there's already a context available.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be removed

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

this.logger.debug('TenantRecord not found in cache')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all logs

tenantRecord = await this.tenantRecordService.getTenantById(this.rootAgentContext, contextCorrelationId)
await cache.set(this.rootAgentContext, `contextCorrelationId-${contextCorrelationId}`, tenantRecord)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your suggestion. In line with that, I modified the code.

this.logger.debug(`Cached tenantRecord '${contextCorrelationId}'`)
}
const shouldUpdate = !isStorageUpToDate(tenantRecord.storageVersion)

// If the tenant storage is not up to date, and autoUpdate is disabled we throw an error
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { AgentContext } from '@credo-ts/core'

import { Key } from '@credo-ts/core'
import { CacheModule, CacheModuleConfig, InMemoryLruCache, Key } from '@credo-ts/core'
import { container } from 'tsyringe'

import { EventEmitter } from '../../../../core/src/agent/EventEmitter'
import { getAgentConfig, getAgentContext, mockFunction } from '../../../../core/tests/helpers'
Expand Down Expand Up @@ -62,6 +63,11 @@ describe('TenantAgentContextProvider', () => {

const tenantAgentContext = jest.fn() as unknown as AgentContext

container.registerInstance(CacheModuleConfig, {
cache: new CacheModule({
cache: new InMemoryLruCache({ limit: 100 }),
}).config.cache as unknown as CacheModule,
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can simplify this

Does this work?

Suggested change
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 })
})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, This test case is working I have tested it on my local machine.

mockFunction(tenantRecordService.getTenantById).mockResolvedValue(tenantRecord)
mockFunction(tenantSessionCoordinator.getContextForSession).mockResolvedValue(tenantAgentContext)

Expand Down
Loading