chore: Reactive resource support for debugging connectivity MCP-80 #413
Conversation
It can listen to events on the session and their arguments, and reduces the current state with the argument to provide a new state.
Also add unit tests to show how to test resources.
We might be telling the user that they successfully connected to a database and we haven't tested it, failing afterwards. By running the hello command, we verify that the user does indeed connected.
it seems that some agents won't detect changes in the resources until we tell them that the list also changed
kmruiz
changed the title
The server might be closed because the connection made the server die or because it's a unit test. In that case, do not fail, just log the error and continue. This is not a critical feature, in case that we couldn't sync once, we will try again at some point or just let it be.
| applyProxyToOIDC: true, | ||
| }); | ||
|
|
||
| await this.serviceProvider?.runCommand?.("admin", { hello: 1 }); |
There was a problem hiding this comment.
NodeDriverServiceProvider.connect, despite it's name, does not in fact connect. This behaviour is likely inherited from the driver.
By forcing a ping (by using hello) we force a connection and validate that it's in fact correct.
There was a problem hiding this comment.
we have a ping on atlas cluster connect tool where we added a ping, should we drop it?
There was a problem hiding this comment.
I see that it's used for answering the client with the connection status. I think we can keep it for now and then when we remove this logic to the new ConnectionManager (Tech Design) we can refactor and use the same code for both connections.
How do you feel about this strategy?
|
|
||
| type ResourceConfiguration = { name: string; uri: string; config: ResourceMetadata }; | ||
|
|
||
| export function ReactiveResource<Value, RelevantEvents extends readonly (keyof SessionEvents)[]>( |
There was a problem hiding this comment.
[q] any reason to use the function returning class pattern instead of using a class straight away?
export class ReactiveResource<...>
There was a problem hiding this comment.
My understanding is that resource metadata is immutable and by using this pattern I feel it's more convenient to specify the configuration once and still be type-safe. I think using normal classes where everything is specified in properties makes more sense when you want to change the state. With this pattern, it's not possible, as the metadata is hidden in the closure.
| this.server.mcpServer.sendResourceListChanged(); | ||
| } catch (error: unknown) { | ||
| logger.warning( | ||
| LogId.serverClosed, |
There was a problem hiding this comment.
This most likely needs a new log id?
| reduce(eventName: undefined, event: undefined): UserConfig { | ||
| void eventName; | ||
| void event; |
There was a problem hiding this comment.
This could possibly be avoided by having reduce without any params?
| eventName: "connect" | "disconnect" | "close" | "connection-error", | ||
| event: string | undefined | ||
| ): ConnectionStateDebuggingInformation { | ||
| void event; |
There was a problem hiding this comment.
Is this necessary anymore, now that you're using event in connection-error case?
Proposed changes
Resources can listen to events on the session and their arguments, and reduces the current state with the argument to provide a new state.
This way this is type safe and declarative, mimicking how tools work.
Checklist