feat: Atlas get performance advisor integration tests #589
Conversation
…rformance-advisor-base-tool
kylelai1
changed the title
kylelai1
changed the title
There was a problem hiding this comment.
Great job so far!
I know some suggestions apply to old code that you inherited, so sorry in advance this slipped over our other reviews. If you need help with any of these specific changes feel free to ping us, some of them might be a bit more advanced than usual.
| }; | ||
| } | ||
|
|
||
| if (statusCode === 402) { |
There was a problem hiding this comment.
Are we getting 402 when a customer tries to access PA on a free/shared tier cluster?
There was a problem hiding this comment.
No, we're getting a 402 for the integration tests when they're run with an org that has no payment set up, since an M10 is created for the new integration test. PA on a free/shared tier cluster will be a 200.
There was a problem hiding this comment.
So are users likely to ever get the 402 response?
There was a problem hiding this comment.
I think one thing to add here is probably elicitation is needed for all these tools since they can add cost to the user, here's some context bcbf889
There was a problem hiding this comment.
users are not likely to get the 402 response. This is purely for the integration tests.
| : "No schema suggestions found.", | ||
| `## Suggested Indexes\n${ | ||
| hasSuggestedIndexes | ||
| ? `Note: The "Weight" field is measured in bytes, and represents the estimated number of bytes saved in disk reads per executed read query that would be saved by implementing an index suggestion. Please convert this to MB or GB for easier readability.\n${JSON.stringify(suggestedIndexesResult.suggestedIndexes)}` |
There was a problem hiding this comment.
This is included from product feedback to make it apparent to the LLM that the "weight" field is in bytes, and gives the definition to relay to the LLM
| try { | ||
| const [suggestedIndexesResult, dropIndexSuggestionsResult, slowQueryLogsResult, schemaSuggestionsResult] = | ||
| await Promise.all([ | ||
| await Promise.allSettled([ |
There was a problem hiding this comment.
Changed this to Promise.allSettled. Made sure it was type safe below as well when using the allSettled.
|
I've just added some changes after demo-ing and testing with the product team, so I've re-requested review for the additional changes/enhancements as well. |
| : "No schema suggestions found.", | ||
| `## Suggested Indexes\n${ | ||
| hasSuggestedIndexes | ||
| ? `Note: The "Weight" field is measured in bytes, and represents the estimated number of bytes saved in disk reads per executed read query that would be saved by implementing an index suggestion. Please convert this to MB or GB for easier readability.\n${JSON.stringify(suggestedIndexesResult.value?.suggestedIndexes)}` |
There was a problem hiding this comment.
Probably this calculation is something that we can do ourselves? It's trivial and we avoid the situation where different LLMs do the calculation differently or wrong.
There was a problem hiding this comment.
We can do the calculation ourselves, but that would mean we'd need to determine whether we want to convert to KB or GB, which I think would depend on the number itself. If the "weight" changes, and is sometimes KB and sometimes GB, then we need to add the units to the "weight" field, which is currently typed as a number. If we want, we can just leave it as bytes, and not have any conversion being done.
kylelai1
merged commit 352831e
into
atlas-list-performance-advisor-tool
Proposed changes
Checklist