-
Notifications
You must be signed in to change notification settings - Fork 9
POC for supporting Spotter 3 #85
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
Changes from all commits
79951b4
45df8b7
bd3c113
a8b9d61
4f7a1bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,24 @@ import { McpAgent } from "agents/mcp"; | |
| import { instrumentDO, type ResolveConfigFn } from '@microlabs/otel-cf-workers'; | ||
| import type { BaseMCPServer, Context } from "./servers/mcp-server-base"; | ||
| import type { Props } from "./utils"; | ||
| import { StreamingConversationState } from "./servers/mcp-server"; | ||
|
|
||
| export function instrumentedMCPServer<T extends BaseMCPServer>(MCPServer: new (ctx: Context) => T, config: ResolveConfigFn) { | ||
| export function instrumentedMCPServer<T extends BaseMCPServer>(MCPServer: new ( | ||
| ctx: Context, | ||
| getConversationState: ( | ||
| conversationId: string, | ||
| ) => Promise<StreamingConversationState | undefined>, | ||
| updateConversationStateAndResetTtlTimeout: ( | ||
| conversationId: string, | ||
| newState: StreamingConversationState, | ||
| ) => Promise<void>, | ||
| ) => T, config: ResolveConfigFn) { | ||
| const Agent = class extends McpAgent<Env, any, Props> { | ||
| server = new MCPServer(this as Context); | ||
| server = new MCPServer( | ||
| this as Context, | ||
| this.getConversationState.bind(this), | ||
| this.updateConversationStateAndResetTtlTimeout.bind(this), | ||
| ); | ||
|
|
||
| // Argument of type 'typeof ThoughtSpotMCPWrapper' is not assignable to parameter of type 'DOClass'. | ||
| // Cannot assign a 'protected' constructor type to a 'public' constructor type. | ||
|
|
@@ -17,6 +31,7 @@ export function instrumentedMCPServer<T extends BaseMCPServer>(MCPServer: new (c | |
|
|
||
| async init() { | ||
| await this.server.init(); | ||
| this.ctx.storage | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| public static serve(path: string) { | ||
|
|
@@ -36,6 +51,34 @@ export function instrumentedMCPServer<T extends BaseMCPServer>(MCPServer: new (c | |
| } | ||
| return server; | ||
| } | ||
|
|
||
| private async getConversationState(conversationId: string) { | ||
| return await this.ctx.storage?.get<StreamingConversationState>(conversationId); | ||
| } | ||
|
|
||
| private async updateConversationStateAndResetTtlTimeout( | ||
| conversationId: string, | ||
| newState: StreamingConversationState, | ||
| ) { | ||
| const oldState = await this.getConversationState(conversationId); | ||
| if (oldState?.ttlTimeoutId) { | ||
| await this.cancelSchedule(oldState.ttlTimeoutId); | ||
| } | ||
|
|
||
| const schedule = await this.schedule(30, 'clearConversationState' as any, { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we do delete and update here? Instead of just appending new messages ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I followed, there is no delete and update here? Are you referring to the scheduled timer?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, is this the TTL ? We delete the conversation after 30 sec ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I used 30s in the POC to validate it was working but in reality it will be longer of course |
||
| conversationId, | ||
| }); | ||
|
|
||
| await this.ctx.storage?.put(conversationId, { | ||
| ...newState, | ||
| ttlTimeoutId: schedule.id, | ||
| }); | ||
| } | ||
|
|
||
| private async clearConversationState(payload: { conversationId: string }) { | ||
| console.log('>>> clearing conversation state', payload.conversationId); | ||
| await this.ctx.storage?.delete(payload.conversationId); | ||
| } | ||
| } | ||
|
|
||
| return instrumentDO(Agent, config) as unknown as typeof Agent; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The number of parameters in this function signature is getting quite large. Consider using an object to group related parameters for better readability and maintainability. This is especially important as more features are added to the MCP server.
For example, instead of passing
getConversationStateandupdateConversationStateAndResetTtlTimeoutas separate parameters, you could create aConversationStateHandlersobject that encapsulates these functions.