Improve Advisor recommendation summaries - #3520
Improve Advisor recommendation summaries#3520Ravi Gupta (ravigupta99) wants to merge 20 commits into
Conversation
…ecommendation-patch
…ecommendation-patch
…ecommendation-patch # Conflicts: # servers/Azure.Mcp.Server/docs/azmcp-commands.md # servers/Azure.Mcp.Server/docs/e2eTestPrompts.md # tools/Azure.Mcp.Tools.Advisor/src/Commands/AdvisorJsonContext.cs # tools/Azure.Mcp.Tools.Advisor/src/Commands/Recommendation/RecommendationListCommand.cs # tools/Azure.Mcp.Tools.Advisor/src/Services/AdvisorService.cs # tools/Azure.Mcp.Tools.Advisor/src/Services/Models/RecommendationDescription.cs # tools/Azure.Mcp.Tools.Advisor/src/Services/Models/RecommendationProperties.cs # tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Recommendation/RecommendationListCommandTests.cs # tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceConversionTests.cs
…r-recommendation-summary # Conflicts: # servers/Azure.Mcp.Server/docs/azmcp-commands.md # tools/Azure.Mcp.Tools.Advisor/src/Commands/Recommendation/RecommendationListCommand.cs # tools/Azure.Mcp.Tools.Advisor/src/Options/Recommendation/RecommendationListOptions.cs # tools/Azure.Mcp.Tools.Advisor/src/Options/Recommendation/RecommendationSummaryOptions.cs # tools/Azure.Mcp.Tools.Advisor/src/Services/AdvisorService.cs # tools/Azure.Mcp.Tools.Advisor/src/Validation/RecommendationFilterValidator.cs # tools/Azure.Mcp.Tools.Advisor/src/Validation/ServiceRetirementFilterValidator.cs # tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceFilterBuilderTests.cs
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
…ecommendation-summary # Conflicts: # servers/Azure.Mcp.Server/README.md # servers/Azure.Mcp.Server/docs/azmcp-commands.md # servers/Azure.Mcp.Server/docs/e2eTestPrompts.md # servers/Azure.Mcp.Server/src/Resources/consolidated-tools.json # tools/Azure.Mcp.Tools.Advisor/src/AdvisorSetup.cs # tools/Azure.Mcp.Tools.Advisor/src/Commands/Recommendation/RecommendationListCommand.cs
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are comprehensive, internally consistent across command/service/validation/docs, and the updated/added unit tests substantively cover the new query-building and summary-shaping behavior.
Pull request overview
This PR upgrades the advisor_recommendation_summary tool to produce canonical, metadata-backed aggregation results across additional grouping dimensions (including lifecycle status, metadata subcategory, and service-retirement date), while tightening validation and aligning summary/list filtering semantics via shared query-building utilities.
Changes:
- Added a dedicated
RecommendationSummaryServicethat builds current-engine-safe ARG queries and returns{ key, label, count }buckets with stable recommendation type IDs and English metadata labels. - Centralized and hardened KQL predicate construction (including current-engine filtering) via
RecommendationQueryBuilder, and expanded/normalized validation for filters and--top. - Updated command surface + docs/prompts/changelog and expanded tests to cover the new grouping/filtering behavior.
File summaries
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/RecommendationSummaryServiceTests.cs | Adds coverage for summary query composition, parsing/sorting, truncation handling, and retirement filter validation. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/RecommendationQueryBuilderTests.cs | Adds coverage for shared instance predicate construction and KQL sanitization semantics. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceSummarizeQueryTests.cs | Removes legacy summarize-query tests replaced by the new summary service. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceFilterBuilderTests.cs | Removes legacy filter-builder tests replaced by RecommendationQueryBuilder. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Recommendation/RecommendationSummaryCommandTests.cs | Updates command tests for new groupings, validations, normalized filters, payload shape, and top/Unknown behavior. |
| tools/Azure.Mcp.Tools.Advisor/src/Validation/ServiceRetirementFilterValidator.cs | Extends shared retirement validation to support summary grouping and stricter empty/format checks. |
| tools/Azure.Mcp.Tools.Advisor/src/Validation/RecommendationFilterValidator.cs | Extracts shared validation/normalization helpers for summary + list. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/RecommendationSummaryService.cs | Implements metadata-backed summary querying, grouping, and payload parsing. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/RecommendationQueryBuilder.cs | Introduces shared, injection-resistant KQL predicate construction and current-engine restrictions. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/IRecommendationSummaryService.cs | Adds a dedicated summary service contract. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/IAdvisorService.cs | Removes the summary API from the main advisor service contract. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/AdvisorService.cs | Switches list filtering to the shared predicate builder and removes the legacy summary implementation. |
| tools/Azure.Mcp.Tools.Advisor/src/Options/Recommendation/RecommendationSummaryOptions.cs | Expands summary options with new filters and clarifies semantics/limits. |
| tools/Azure.Mcp.Tools.Advisor/src/Options/Recommendation/RecommendationListOptions.cs | Clarifies list --top semantics now that summary has its own cap. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationGroup.cs | Expands group model to include label alongside key and count. |
| tools/Azure.Mcp.Tools.Advisor/src/Commands/Recommendation/RecommendationSummaryCommand.cs | Updates command DI + validation + normalization + error handling and top/Unknown slicing behavior. |
| tools/Azure.Mcp.Tools.Advisor/src/AdvisorSetup.cs | Registers the new summary service and updates group description text. |
| tools/Azure.Mcp.Tools.Advisor/cspell.yaml | Adds leftouter to support KQL test strings. |
| servers/Azure.Mcp.Server/src/Resources/consolidated-tools.json | Refreshes the public tool description to reflect new summary capabilities and constraints. |
| servers/Azure.Mcp.Server/README.md | Adds example prompts and updates the Advisor service listing text. |
| servers/Azure.Mcp.Server/docs/e2eTestPrompts.md | Expands realistic evaluator prompts for the updated summary tool. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Updates CLI docs for the expanded summary command surface and semantics. |
| servers/Azure.Mcp.Server/changelog-entries/guptaravi-advisor-recommendation-summary.yml | Adds a schema-shaped changelog entry for the feature + fixes. |
Review details
- Files reviewed: 24/24 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "Only current-engine recommendations whose stable name is a 64-character hash and whose serviceGroupId is empty are included. " + | ||
| "Filters include category, impact, recommendation type ID, impacted resource type, resource name or ARM ID, problem-text search, subcategory, and explicit retirement-date comparisons. " + | ||
| "Use --search with this summary tool for topical aggregate questions such as counts or impact breakdowns for recommendations mentioning encryption or right-size; do not call recommendation list and count its capped results. " + | ||
| "For natural-language windows such as 'retiring soon' or 'in the next two months', compute an end date and pass le:<yyyy-MM-dd>; unqualified 'soon' means 90 calendar days and does not add a lower bound. " + |
There was a problem hiding this comment.
le:<end> is not a bounded “next N” window; it also includes overdue retirements. Please add a lower bound or stop advertising next-N windows.
| var projection = groupBy switch | ||
| { | ||
| GroupByRecommendationType => | ||
| " | extend key = iff(isempty(recommendationTypeId), 'Unknown', recommendationTypeId)" + |
There was a problem hiding this comment.
This changes the existing groups[].key contract from problem text to a type GUID. Please preserve compatibility or classify this as a breaking change with the required changelog entry, label, and migration note.
| } | ||
|
|
||
| var additionalFilter = BuildAdditionalFilter(filters, metadataByTypeId?.Keys); | ||
| var additionalFilter = RecommendationQueryBuilder.BuildInstancePredicates( |
There was a problem hiding this comment.
This also changes recommendation list by excluding non-64-character and service-group records. Please keep list semantics unchanged or explicitly scope, document, and test this second tool change.
| | advisor_recommendation_summary | Break down my Advisor recommendations by impact | none | | ||
| | advisor_recommendation_summary | Show the distribution of my Advisor recommendations by business impact | none | | ||
| | advisor_recommendation_summary | Show the top 10 most common Advisor recommendation types | none | | ||
| | advisor_recommendation_summary | Which Advisor recommendation type occurs most often? | none | |
There was a problem hiding this comment.
We should probably say, "Which Advisor recommendation type occurs most often for my resources/subscriptions/workloads". Else it gives incorrect assumption that this is based on global data.
What does this PR do?
Improves
advisor_recommendation_summaryso aggregate Advisor questions return canonical, complete, and correctly grouped recommendation counts.This PR:
{ key, label, count }, using stable recommendation type IDs with English metadata labels;properties.impactedFieldfor resource-type filtering/grouping, with resource-ID extraction only as a grouping fallback;Newrecommendations by default while status grouping includes all lifecycle states;--topas 1-100 while preserving the full total and a trailingUnknownbucket;strlen(name) == 64andisempty(properties.serviceGroupId);Dependency
PRs #3303 and #3307 are merged. This branch has been synchronized with the resulting
main.GitHub issue number?
Enhancement to the existing Advisor recommendation summary tool; no new onboarding issue was created.
Validation
dotnet test tools\Azure.Mcp.Tools.Advisor\tests\Azure.Mcp.Tools.Advisor.Tests\Azure.Mcp.Tools.Advisor.Tests.csprojdotnet build servers\Azure.Mcp.Server\src\Azure.Mcp.Server.csprojdotnet format Microsoft.Mcp.slnx --verify-no-changes --include "tools\Azure.Mcp.Tools.Advisor\**\*.cs".\eng\common\spelling\Invoke-Cspell.ps1.\eng\common\scripts\check-spelling-in-changed-files.ps1 ... -ExitWithError.\eng\scripts\Process-PackageReadMe.ps1 -Command validate -InputReadMePath .\servers\Azure.Mcp.Server\README.md.\eng\scripts\Update-AzCommandsMetadata.ps1.\eng\scripts\Compile-Changelog.ps1 -ChangelogPath servers\Azure.Mcp.Server\CHANGELOG.md -DryRun.\eng\scripts\Build-Local.ps1 -VerifyNpxTool selection
ToolDescriptionEvaluator passed all 22
advisor_recommendation_summaryprompts:0.419316-0.618690.The final Vally suite against the exact locally built
azmcp.exepassed 30/30 summary/list/metadata routing prompts. The previously unstable relative-retirement summary prompt also passed 5/5 repeated trials after the list/summary boundary was clarified.Live Azure validation
The exact branch binary was exercised against live Advisor data for all seven groupings and every public filter/validation path.
serviceGroupIdexclusions: 0 (forward-looking filter);Advisor does not currently have deterministic recorded-test resource infrastructure for generated recommendations or global metadata. This PR follows the established Advisor exception with comprehensive command/query tests, exact local-binary Azure calls, and independent ARG comparisons.
Native AOT
.\eng\scripts\Build-Local.ps1 -IncludeNativecompleted its wrapper and packaging flow but the inner Azure MCP native publish reported existing Cosmos dependency diagnostics:IL2026andIL3050inMicrosoft.Azure.Documents.JsonSerializable.SetValueCollection<T>;MSB3077from the Native AOT compiler.The Advisor projects build with no warnings. The native failure is pre-existing and unrelated to this change.
Pre-merge Checklist
azmcp-commands.mdand regenerated metadatae2eTestPrompts.mdmainafter dependency Add Advisor recommendation update tool #3303 mergedInvoking Livetests
Copilot submitted PRs are not trustworthy by default. Users with
writeaccess to the repo need to validate the contents of this PR before leaving a comment with the text/azp run mcp - pullrequest - live. This will trigger the necessary livetest workflows to complete required validation.