Add metadata-backed filtering and enrichment for List Advisor recommendations tool - #3307
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
…oushik137/advisor-list-metadata-join
|
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.
Pull request overview
Adds metadata-backed filtering and type-level enrichment to the Azure Advisor recommendation list tool, enabling filtering by recommendation type GUID and service-retirement metadata (tracking IDs + retirement date) while paging through Advisor metadata to avoid Resource Graph truncation.
Changes:
- Added new CLI filters (
--recommendation-type-id,--sub-category, multi-value--tracking-ids,--retirement-date) plus shared validation for service-retirement filter rules. - Implemented metadata resolution + paging (1000/page with continuation handling) and joined metadata into recommendation results (with Security-category bypass rules).
- Updated response models, tests, docs (
azmcp-commands.md,e2eTestPrompts.md), and added a changelog entry.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceMetadataJoinTests.cs | New tests covering metadata paging, lookup, and join behavior. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceFilterBuilderTests.cs | Extends KQL filter-building tests for new filters/type-ID intersection. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorServiceConversionTests.cs | Updates conversion/serialization expectations for expanded recommendation shape. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Services/AdvisorMetadataServiceTests.cs | Updates metadata query tests for multi-tracking-id behavior and new projection/order. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Recommendation/RecommendationListCommandTests.cs | Adds/updates command tests for new filters and validation errors. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Metadata/RecommendationMetadataListCommandTests.cs | Updates metadata-list command tests for tracking ID list handling. |
| tools/Azure.Mcp.Tools.Advisor/tests/Azure.Mcp.Tools.Advisor.Tests/Metadata/MetadataGetCommandTests.cs | Renames a test to reflect metadata language normalization wording. |
| tools/Azure.Mcp.Tools.Advisor/src/Validation/ServiceRetirementFilterValidator.cs | New shared validator for retirement-date parsing and subcategory compatibility rules. |
| tools/Azure.Mcp.Tools.Advisor/src/Validation/RecommendationFilterValidator.cs | New validator for recommendation list filters (allowed values, GUID format, etc.). |
| tools/Azure.Mcp.Tools.Advisor/src/Services/Models/RecommendationProperties.cs | Expands ARG wire model to include additional recommendation fields. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/Models/RecommendationDescription.cs | Adds Solution to wire short description model. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/Models/RecommendationData.cs | Expands wire model shape (e.g., resource group) and clarifies docs. |
| tools/Azure.Mcp.Tools.Advisor/src/Services/AdvisorService.cs | Implements metadata match resolution + paging, enrichment join, and extended filtering logic. |
| tools/Azure.Mcp.Tools.Advisor/src/Options/Recommendation/RecommendationListOptions.cs | Adds new CLI options and updated help text for metadata-backed filters. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationShortDescription.cs | New public response model for short description. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationResourceMetadata.cs | New public response model for impacted resource metadata. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationProperties.cs | New public response model capturing richer recommendation properties. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationMetadataFilters.cs | Updates metadata filters to support multi-value tracking IDs. |
| tools/Azure.Mcp.Tools.Advisor/src/Models/RecommendationFilters.cs | Adds new filtering fields (type ID, subcategory, tracking IDs, retirement date). |
| tools/Azure.Mcp.Tools.Advisor/src/Models/Recommendation.cs | Changes recommendation response to an ARM-style {id,name,type,properties} shape. |
| tools/Azure.Mcp.Tools.Advisor/src/Commands/Recommendation/RecommendationListCommand.cs | Wires new options/validation and forwards parsed filters into the service. |
| tools/Azure.Mcp.Tools.Advisor/src/Commands/Metadata/RecommendationMetadataListCommand.cs | Reuses shared retirement filter parsing/validation. |
| tools/Azure.Mcp.Tools.Advisor/src/Commands/AdvisorJsonContext.cs | Registers new response/wire-model serialization types for source-gen (AOT). |
| servers/Azure.Mcp.Server/docs/e2eTestPrompts.md | Adds/updates prompts covering the new filter scenarios. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Updates CLI docs to include new options and service-retirement guidance. |
| servers/Azure.Mcp.Server/changelog-entries/advisor-recommendation-metadata-filters.yml | Adds a changelog entry describing the new filters + paging behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| Category = metadata.Category, | ||
| Impact = metadata.Impact, | ||
| ShortDescription = new RecommendationShortDescription( |
There was a problem hiding this comment.
This builds the RecommendationShortDescription with metadata.DisplayName in both fields, so when DisplayName is set (the normal case) problem == solution and the instance's actual solution text is discarded. Two things worth reconsidering:
- Overwriting
solutionwith the type-level display name loses per-instance solution guidance. Consider per-field fallback (e.g.recommendation.Properties.ShortDescription?.Problem ?? metadata.DisplayNamefor problem, and keep the instance solution) or surfaceDisplayNamein its own field. - If
DisplayNameand the instance description are both null, this still allocates a record that serializes toshortDescription: {}underDefaultIgnoreCondition = WhenWritingNull.ConvertToRecommendationMetadataModelonly enforcesrecommendationTypeId(notDisplayName), so the "metadata always has displayName" assumption isn't guaranteed in code — cheap to guard by returningnullwhen both are empty.
| filters?.RetirementDate is not null || | ||
| !string.IsNullOrWhiteSpace(filters?.RetirementDateOperator); | ||
|
|
||
| internal static bool HasMetadataFilters(RecommendationFilters? filters) => |
There was a problem hiding this comment.
Because enrichment overwrites the instance category/impact with metadata values, a lone --impact or --category (non-Security) is treated as a metadata filter here. That pre-resolves the whole metadata catalog and then emits recommendationTypeId in~ ('id1' … 'idN') on the instance query. For broad filters that type-ID set can be large (paged at 1000), producing a very large IN clause and an extra catalog round-trip. It's a defensible consistency trade-off (filter on the same field you display), but worth a comment here and possibly a guardrail on how many type IDs get pushed into the query.
| { | ||
| var additionalFilter = BuildAdditionalFilter(filters); | ||
| Dictionary<string, RecommendationMetadata>? metadataByTypeId = | ||
| await ResolveMetadataFilterMatchesAsync(filters, cancellationToken); |
There was a problem hiding this comment.
Both metadata paths — this filter-resolution call and the enrichment fetch below (GetRecommendationMetadataByTypeIdsAsync) — go through GetTenantResourceAsync, which returns tenants[0] and ignores the --tenant option, whereas the recommendations query itself honors tenant. Advisor metadata is a language-keyed global catalog so this is usually harmless, but it's inconsistent and could hit an unintended or less-privileged tenant for the ARG metadata call. Consider threading tenant through the metadata paths too.
| result.ResultTruncated == ResultTruncated.True); | ||
| } | ||
|
|
||
| private static List<RecommendationMetadata> ParseMetadata(BinaryData data) |
There was a problem hiding this comment.
Nit: this parse block duplicates the array-parse + validation already in ExecuteMetadataQueryAsync. Worth collapsing the two into a single shared helper.
| recommendation.Properties.ExtendedProperties, | ||
| metadata.ServiceRetirement), | ||
| metadata.SubCategory), | ||
| ResourceMetadata = recommendation.Properties.ResourceMetadata, |
There was a problem hiding this comment.
Nit: ResourceMetadata = recommendation.Properties.ResourceMetadata inside the with expression is a no-op (it already holds that value) and can be dropped.
What does this PR do?
This PR adds metadata-backed filtering and enrichment to the List Advisor recommendations tool, enabling precise filtering by recommendation type ID and service-retirement details while enriching recommendation data with comprehensive metadata.
Core Capabilities Added:
1. Four New Filtering Options
2. Intelligent Metadata Enrichment Pipeline
3. Paginated Metadata Resolution
4. Comprehensive Validation Framework
GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline