feat: Add Atlas Local Tools #632
Conversation
|
@nirinchev @kmruiz @himanshusinghs All PRs to the feature branch has been reviewed but we would like someone from DevTools to review before merging to main please :) |
| if (process.env.SKIP_ATLAS_LOCAL_TESTS === "true") { | ||
| vitestDefaultExcludes.push("**/atlas-local/**"); | ||
| } |
There was a problem hiding this comment.
[nit] Can we apply the same exclusion mechanism for the atlas tests. I know that those are magically excluded by virtue of not providing the API keys, but it's kind of annoying we have several different ways of doing things.
There was a problem hiding this comment.
Makes sense to be explicit, added
src/common/session.ts
Outdated
| setAtlasLocalClient(atlasLocalClient: Client): void { | ||
| this.atlasLocalClient = atlasLocalClient; | ||
| } |
There was a problem hiding this comment.
atlasLocalClient is already a public field - this method seems redundant.
There was a problem hiding this comment.
Removed the method
| // Lookup the deployment id and add it to the telemetry metadata | ||
| await this.lookupDeploymentId(client, deploymentName); |
There was a problem hiding this comment.
This feels a bit awkward and error-prone. Can we move this to resolveTelemetryMetadata similarly to what we do in the atlas tools?
There was a problem hiding this comment.
refactored to be more similar to atlas tools
|
|
||
| export abstract class AtlasLocalToolBase extends ToolBase { | ||
| public category: ToolCategory = "atlas-local"; | ||
| protected deploymentId?: string; |
There was a problem hiding this comment.
We shouldn't be storing state from previous invocations - it's very flimsy and a potential footgun.
There was a problem hiding this comment.
Removed. Only the create tool now stores the deploymentId so we can associate the tool call with the created deployment in the telemetry
| const deploymentOptions: CreateDeploymentOptions = { | ||
| name: deploymentName, | ||
| creationSource: { | ||
| type: "MCPServer" as CreationSourceType, |
There was a problem hiding this comment.
| type: "MCPServer" as CreationSourceType, | |
| type: CreationSourceType.MCPServer, |
There was a problem hiding this comment.
This was suggested here too. iirc this could not be done because of the way the napi creates a typescript binding from a rust enum.
There was a problem hiding this comment.
This suggestion is different from the original one though. Here I'm suggesting using the enum as an enum as opposed to a string.
There was a problem hiding this comment.
Ah right, yes in that case the issue is that the atlas-local dependency is optional so you can only import it as a type rather than directly
There was a problem hiding this comment.
I see, this is super awkward and we should do something to resolve this. If importing the package directly is not allowed, we should ensure we have a lint rule for that to avoid accidental imports. Additionally, we should probably convert these enums into union types so that we don't need to import the package to use them.
There was a problem hiding this comment.
Ah yes string union type that is the name. We looked into using them since that is the ideal type but napi-rs doesn't support them from what we found
src/server.ts
Outdated
| const { Client: AtlasLocalClient } = await import("@mongodb-js-preview/atlas-local"); | ||
|
|
||
| // Connect to Atlas Local client | ||
| // This will fail if docker is not running | ||
| const client = AtlasLocalClient.connect(); | ||
|
|
||
| // Set Atlas Local client | ||
| this.session.setAtlasLocalClient(client); |
There was a problem hiding this comment.
We should probably do this in TransportRunnerBase and cache the client rather than re-do it for every connection. Then we could pass down the client to the server at initialization time and register the tools all the same time.
| console.warn( | ||
| "Failed to initialize Atlas Local client, atlas-local tools will be disabled (error: ", | ||
| error, | ||
| ")" | ||
| ); |
There was a problem hiding this comment.
We should probably handle the two expected errors - failure to import due to unsupported platform and docker not being available and emit more meaningful logs.
There was a problem hiding this comment.
Split the errors and added more meaningful logging
| error: unknown, | ||
| args: ToolArgs<typeof this.argsShape> | ||
| ): Promise<CallToolResult> | CallToolResult { | ||
| // Error Handling for expected Atlas Local errors go here |
There was a problem hiding this comment.
We should also handle the case the docker daemon is not running. This can happen if I started the MCP server and at a later point, shut down docker.
4 participants
Proposed changes
Jira: MCP-40
Checklist