Repository navigation
Conversation
|
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.
🟡 Changes recommended
Missing upload routing and false-positive stream validation remain unresolved, alongside required test coverage and evaluation evidence.
4 open findings
What changed in this PR
Adds log upload and pre-ingestion validation to the Azure Monitor toolset, addressing #288 and #295.
Changes:
- Adds ingestion commands, service integration, and structured results.
- Adds payload/schema validation and cloud-specific endpoint allow-lists.
- Adds unit tests, command documentation, prompts, and a changelog entry.
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.Tests/Ingestion/IngestionValidateCommandTests.cs | Tests validation command behavior. |
| tools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.Tests/Ingestion/IngestionUploadCommandTests.cs | Tests upload command behavior. |
| tools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.Tests/Ingestion/IngestionLogDataValidatorTests.cs | Tests payload and schema checks. |
| tools/Azure.Mcp.Tools.Monitor/src/Validation/IngestionLogDataValidator.cs | Implements payload and column validation. |
| tools/Azure.Mcp.Tools.Monitor/src/Services/MonitorIngestionService.cs | Implements upload and DCR lookup. |
| tools/Azure.Mcp.Tools.Monitor/src/Services/IMonitorIngestionService.cs | Defines ingestion service contracts. |
| tools/Azure.Mcp.Tools.Monitor/src/Options/Ingestion/IngestionValidateOptions.cs | Defines validation inputs. |
| tools/Azure.Mcp.Tools.Monitor/src/Options/Ingestion/IngestionUploadOptions.cs | Defines upload inputs. |
| tools/Azure.Mcp.Tools.Monitor/src/MonitorSetup.cs | Registers both commands and service. |
| tools/Azure.Mcp.Tools.Monitor/src/Models/Ingestion/IngestionValidationResult.cs | Models validation results. |
| tools/Azure.Mcp.Tools.Monitor/src/Models/Ingestion/IngestionValidationIssue.cs | Models individual validation issues. |
| tools/Azure.Mcp.Tools.Monitor/src/Models/Ingestion/IngestionUploadResult.cs | Models upload status and count. |
| tools/Azure.Mcp.Tools.Monitor/src/Commands/MonitorJsonContext.cs | Registers result serialization. |
| tools/Azure.Mcp.Tools.Monitor/src/Commands/Ingestion/IngestionValidateCommand.cs | Exposes schema validation. |
| tools/Azure.Mcp.Tools.Monitor/src/Commands/Ingestion/IngestionUploadCommand.cs | Exposes log upload. |
| tools/Azure.Mcp.Tools.Monitor/src/Azure.Mcp.Tools.Monitor.csproj | Adds ingestion SDK dependency. |
| servers/Azure.Mcp.Server/src/Resources/consolidated-tools.json | Maps validation into consolidated discovery. |
| servers/Azure.Mcp.Server/docs/e2eTestPrompts.md | Adds prompts for both tools. |
| servers/Azure.Mcp.Server/docs/azmcp-commands.md | Documents commands and parameters. |
| servers/Azure.Mcp.Server/changelog-entries/monitor-ingestion-upload-validate.yaml | Announces ingestion functionality. |
| core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Core.Tests/Helpers/EndpointValidatorTests.cs | Tests ingestion endpoint allow-lists. |
| core/Microsoft.Mcp.Core/src/Helpers/EndpointValidator.AllowLists.cs | Adds cloud-specific ingestion suffixes. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) | ||
| { | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Hmm, this is different than our normal design, could you add why. Is this something we'd want to do in other locations / generally?
There was a problem hiding this comment.
Good point, looking over this special catch is necessary I don't think this is necessary, removed
| ILogger<IngestionValidateCommand> logger, | ||
| IMonitorIngestionService ingestionService, | ||
| ISubscriptionResolver subscriptionResolver) | ||
| : SubscriptionCommand<IngestionValidateOptions, IngestionValidationResult>(subscriptionResolver) |
There was a problem hiding this comment.
Why does validate need a subscription when upload doesn't? Is it possible to validate against the specific ingestion endpoint?
There was a problem hiding this comment.
Validate reads the stream schema from the data collection rule via ARM, which needs the subscription; a data collection endpoint doesn't expose schemas, so it can't validate against the endpoint alone. Upload only needs the endpoint and rule ID.
| var shape = IngestionLogDataValidator.Validate(options.LogData, schema: null); | ||
| foreach (var error in shape.Errors) | ||
| { | ||
| validationResult.AddError(error.Message, "Invalid ingestion log data."); |
There was a problem hiding this comment.
Well, you've found an edge case I didn't think of when I added telemetry safe error information, if there are many errors here you'll have a lot of Invalid ingestion log data. in the telemetry. Maybe copy the design in the service implementation where we space concatenate the error messages and have a single call to add error.
There was a problem hiding this comment.
Done: errors are space-concatenated into a single AddError call.
| response.IsError ? "Failed" : "Succeeded", | ||
| dataCollectionRule, | ||
| stream, | ||
| shape.RecordCount); |
There was a problem hiding this comment.
If the response is an error, are any records uploaded? If the upload is an all or nothing batch this should be 0 on error, if partial batches are possible this number will also be wrong.
There was a problem hiding this comment.
The upload is a single all-or-nothing request, so RecordCount is now 0 on error.
| } | ||
| else | ||
| { | ||
| warnings.Add(new(null, null, $"Stream '{selectedStream}' has no declared schema in the data collection rule, so only payload shape was checked.")); |
There was a problem hiding this comment.
This seems like it should be a thrown exception similar to when there are zero or many declarations when --stream isn't passed.
There was a problem hiding this comment.
Done: an undeclared stream now throws CommandValidationException.
| || !string.IsNullOrEmpty(endpoint.Query) | ||
| || !string.IsNullOrEmpty(endpoint.Fragment)) | ||
| { | ||
| throw new CommandValidationException("--ingestion-endpoint must be an HTTPS data collection endpoint root URL without user information, path, query, or fragment.") |
There was a problem hiding this comment.
This message is telemetry safe already.
There was a problem hiding this comment.
Removed the redundant TelemetrySafeMessage.
| { | ||
| AzureCloudConfiguration.AzureCloud.AzureChinaCloud => LogsIngestionAudience.AzureChina, | ||
| AzureCloudConfiguration.AzureCloud.AzureUSGovernmentCloud => LogsIngestionAudience.AzureGovernment, | ||
| _ => LogsIngestionAudience.AzurePublicCloud |
There was a problem hiding this comment.
I think we should always throw on an unhandled cloud type rather than falling back to the public cloud. This could cause hard to debug issues in the future if we add support for more clouds.
There was a problem hiding this comment.
Done: unsupported cloud types now throw NotSupportedException instead of falling back to public.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83d3236e-c51b-4809-861f-a45a9a63ae7c
6802587 to
f62bfa4
Compare
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83d3236e-c51b-4809-861f-a45a9a63ae7c
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 83d3236e-c51b-4809-861f-a45a9a63ae7c



What does this PR do?
Adds two tools to the Monitor toolset (combined in one PR at the requester's request):
monitor ingestion upload(azmcp-monitor-ingestion-upload): uploads a JSON array of log records via the Logs Ingestion API (--ingestion-endpoint,--data-collection-ruleimmutable ID,--stream,--log-data). Returns status and record count.monitor ingestion validate(azmcp-monitor-ingestion-data-validate): checks payload shape (JSON array of objects, ≤1 MB) and, using the stream declared in the DCR (looked up via ARM by name), column types. Returns errors/warnings as JSON.Notes / design decisions for review:
--streamoption (required by the Logs Ingestion API; optional on validate when the DCR declares exactly one stream).--data-collection-ruleis the immutable ID for upload but the resource name (with--resource-group/--subscription) for validate; documented in option descriptions.IAzureService.ValidateAzureServiceEndpointwith a newmonitor-ingestionallow-list (*.ingest.monitor.azure.{com,cn,us}), with tests.GitHub issue number?
Resolves #288
Resolves #295
Validation
dotnet testintools/Azure.Mcp.Tools.Monitor/tests/Azure.Mcp.Tools.Monitor.Tests: passed (496 succeeded, 0 failed)dotnet testincore/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Core.Tests: passed (833 succeeded, 0 failed)Pre-merge Checklist
consolidated-tools.json(validate added; upload is not read-only)azmcp-commands.mdande2eTestPrompts.mdUpdate-AzCommandsMetadata.ps1, READMEInvoking 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.