feat: Adds the atlas-list-performance-advisor base tool #528
Conversation
kylelai1
changed the title
| }, | ||
| }); | ||
| return { | ||
| suggestedIndexes: (response as SuggestedIndexesResponse).content.suggestedIndexes ?? [], |
There was a problem hiding this comment.
This is concerning - why are we recasting the response to a different shape? This seems to indicate the definitions we've generated from the open api spec are not correct.
There was a problem hiding this comment.
Yes, that's correct. I've manually tested this with the MCP server tool, and separately by making requests to the admin api (v2), and found that the response from the open API spec is not correct. It is missing the content field, which I've added when accessing the suggestedIndexes. This applies to the 4 different PA endpoints we're calling as a part of this tool.
There was a problem hiding this comment.
I wonder why the other responses from the API client don't have the same problem. @blva's team owns that part of the codebase, so perhaps something to look into?
There was a problem hiding this comment.
I don't see content in ApiPerformanceAdvisorResponseView (schema name PerformanceAdvisorResponse) shape in MMS.
Why are we adding this interface and why do we need this content field? I think you should be able to use the openapi specs as they are.
interface SuggestedIndexesResponse {
content: components["schemas"]["PerformanceAdvisorResponse"];
}
You don't need to import content: components["schemas"]["PerformanceAdvisorResponse"]; here, you should probably use PerformanceAdvisorResponse from openapi.d.ts, right?
There was a problem hiding this comment.
So far looks good, but I think there are a few things to improve here in terms of user experience and maintainability of the actual tool.
Great job @kylelai1, we know it's still complicated to implement new tools but you've been doing great so far.
| projectId: z.string().describe("Atlas project ID to list performance advisor recommendations"), | ||
| clusterName: z.string().describe("Atlas cluster name to list performance advisor recommendations"), | ||
| operations: z | ||
| .array(PerformanceAdvisorOperationType) | ||
| .default(PerformanceAdvisorOperationType.options) | ||
| .describe("Operations to list performance advisor recommendations"), | ||
| since: z.date().describe("Date to list slow query logs since").optional(), | ||
| namespaces: z | ||
| .array(z.string()) | ||
| .describe("Namespaces to list slow query logs. Only relevant for the slowQueryLogs operation.") | ||
| .optional(), | ||
| }; |
There was a problem hiding this comment.
I think this requires some second thoughts.
When the user is connected to an Atlas Cluster, we have already the projectId and clusterName, which should be enough. I think we should leverage this as a default. This would make projectId and clusterName optional.
You can retrieve the current connected cluster from the session:
this.session.connectedAtlasCluster
There was a problem hiding this comment.
If the user has an atlas cluster already connected, the LLM should be able to provide the projectId and clusterName already without any additional user input. If there hasn't been a specified cluster, then the LLM will prompt the user to provide the projectId and clusterName.
if we were to make the projectId and clusterName default, then we'd need to build it in to the tool to retrieve this from the connected cluster as you said.
Do we find that the LLM prompting the user for this is something we want to avoid, and that we want to instead have the tool handle as much of that as possible? Even if the projectId and clusterName are not optionals, the LLM will still provide these if there was already a previous connected cluster.
There was a problem hiding this comment.
I don't think there's an explicit expectation that the user should be connected to the cluster (via a mongodb connection) before they're able to ask for performance advice. Additionally, many other Atlas tools follow the same pattern of requiring a cluster name and project id, so I think leaving things as they are makes sense.
There was a problem hiding this comment.
Arguably this is the first tool that is actually relevant for a single cluster where the user is connected.
However, I am not going to die on this hill, but at least I would suggest to use the AtlasArgs types we have in args.ts, instead of Zod primitives, to align to what other Atlas tools do.
Edit:
The Atlas connect tool does not provide the projectId of the cluster, so my guess is that we trust the agent to provide it from another place after somehow listing projects and clusters.
I think this could be avoided and simplified making these args optional, so a prompt as easy as: give me the performance suggestions of the current cluster" would work even if rhe user never queried for other projects, action that might not have permissions to do.
There was a problem hiding this comment.
If there is already a connected cluster, then a simple prompt should be able to be understood by the LLM without prompting for the projectID and clusterName again. We can revisit this during QA when we manually test the tool with a connected cluster or without a connected cluster to see if it will prompt the user for the projectId and clusterName again.
Looks like AtlasArgs was not created when I started work for this ticket, so thanks for pointing that out! I'll make that change.
kylelai1
merged commit 173c587
into
atlas-list-performance-advisor-tool
Proposed changes
This PR adds the atlas-list-performance-advisor tool to the MCP server, which retrieves the following performance advisor recommendations from the admin API: index suggestions, drop index suggestions, schema suggestions, slow query logs.
This PR merges the changes into the atlas-list-performance-advisor-tool branch.
Testing
Checklist