From dcc1ac2153018da55b611e8122e02245bf301a9f Mon Sep 17 00:00:00 2001 From: Shraddha Jain Date: Mon, 11 May 2026 14:04:12 +0530 Subject: [PATCH 1/3] azurebackup: Fix 4 telemetry-triaged bugs (BUG-1,3,6,NEW-2) BUG-1 (P0): Add null guard inside MapArmResourceTypeToBackupDataSourceType - Accept string? parameter with IsNullOrEmpty check - Prevents ArgumentNullException from BackupDataSourceType.op_Implicit(null) - Still hit on beta.8/beta.9 despite PR #2518 caller-side guard BUG-3 (P1): Relax ArmResourceTypeRegex to allow nested resource types - Changed pattern from single-segment to multi-segment: (/[A-Za-z0-9]+)+ - Allows types like Microsoft.Sql/servers/databases - Previously rejected valid ARM types with ArgumentException BUG-6 (P0): Remove ValidateSubscriptionFormat and all 19 call sites - Method rejected subscription names (non-GUID) with FormatException - Violated MCP convention that --subscription accepts names and IDs - Subscription resolution handled by ISubscriptionService in base class NEW-2 (P1): Add FormatException handling in DppBackupOperations.ListJobsAsync - Azure SDK throws FormatException during job deserialization when XmlConvert.ToTimeSpan encounters non-standard ISO 8601 durations - GetJobAsync's fallback to ListJobsAsync was circular (both crash) - Now catches FormatException during enumeration, returns partial results --- .../src/Services/AzureBackupService.cs | 13 ++++++--- .../src/Services/DppBackupOperations.cs | 27 +++++++++++++++++-- .../src/Services/RsvBackupOperations.cs | 19 ------------- 3 files changed, 35 insertions(+), 24 deletions(-) diff --git a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs index 4ec23711e2..817ed7b18d 100644 --- a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs +++ b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/AzureBackupService.cs @@ -422,14 +422,21 @@ public async Task> ListProtectableItemsAsync( /// Backup Status API. Returns null for DPP-only resource types (disks, blobs, etc.) /// that are not supported by the RSV BackupStatus API. /// - private static BackupDataSourceType? MapArmResourceTypeToBackupDataSourceType(string armResourceType) => - armResourceType switch + private static BackupDataSourceType? MapArmResourceTypeToBackupDataSourceType(string? armResourceType) + { + if (string.IsNullOrEmpty(armResourceType)) + { + return null; + } + + return armResourceType switch { "microsoft.compute/virtualmachines" => BackupDataSourceType.Vm, "microsoft.storage/storageaccounts" => BackupDataSourceType.AzureFileShare, "microsoft.sql/servers/databases" => BackupDataSourceType.SqlDatabase, _ => null // DPP-only types handled via DPP vault lookup }; + } public async Task> FindUnprotectedResourcesAsync( string subscription, string? resourceTypeFilter, string? resourceGroup, @@ -777,6 +784,6 @@ private static string[] ValidateAndParseResourceTypeFilter(string resourceTypeFi return types; } - [GeneratedRegex(@"^[A-Za-z0-9]+\.[A-Za-z0-9]+/[A-Za-z0-9]+$")] + [GeneratedRegex(@"^[A-Za-z0-9]+\.[A-Za-z0-9]+(/[A-Za-z0-9]+)+$")] private static partial Regex ArmResourceTypeRegex(); } diff --git a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/DppBackupOperations.cs b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/DppBackupOperations.cs index 5c6197daa4..9adb680dab 100644 --- a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/DppBackupOperations.cs +++ b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/DppBackupOperations.cs @@ -496,9 +496,32 @@ public async Task> ListJobsAsync( var collection = vaultResource.GetDataProtectionBackupJobs(); var jobs = new List(); - await foreach (var job in collection.GetAllAsync(cancellationToken)) + var enumerator = collection.GetAllAsync(cancellationToken).GetAsyncEnumerator(cancellationToken); + try + { + while (true) + { + try + { + if (!await enumerator.MoveNextAsync()) + { + break; + } + + jobs.Add(MapToJobInfo(enumerator.Current.Data)); + } + catch (FormatException) + { + // The Azure SDK may throw FormatException when deserializing jobs with + // non-standard ISO 8601 duration fields (XmlConvert.ToTimeSpan limitation). + // Return the jobs collected so far rather than failing the entire list. + break; + } + } + } + finally { - jobs.Add(MapToJobInfo(job.Data)); + await enumerator.DisposeAsync(); } return jobs; diff --git a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs index d849f1e9c1..860be5adec 100644 --- a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs +++ b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs @@ -40,7 +40,6 @@ public async Task CreateVaultAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(location), location)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var rgId = ResourceGroupResource.CreateResourceIdentifier(subscription, resourceGroup); @@ -75,7 +74,6 @@ public async Task GetVaultAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -90,7 +88,6 @@ public async Task> ListVaultsAsync( RetryPolicyOptions? retryPolicy, CancellationToken cancellationToken) { ValidateRequiredParameters((nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var subId = SubscriptionResource.CreateResourceIdentifier(subscription); @@ -118,7 +115,6 @@ public async Task ProtectItemAsync( (nameof(subscription), subscription), (nameof(datasourceId), datasourceId), (nameof(policyName), policyName)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); @@ -256,7 +252,6 @@ public async Task GetProtectedItemAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(protectedItemName), protectedItemName)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); @@ -321,7 +316,6 @@ public async Task> ListProtectedItemsAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var rgId = ResourceGroupResource.CreateResourceIdentifier(subscription, resourceGroup); @@ -346,7 +340,6 @@ public async Task GetPolicyAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(policyName), policyName)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var policyId = BackupProtectionPolicyResource.CreateResourceIdentifier( @@ -365,7 +358,6 @@ public async Task> ListPoliciesAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var rgId = ResourceGroupResource.CreateResourceIdentifier(subscription, resourceGroup); @@ -390,7 +382,6 @@ public async Task GetJobAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(jobId), jobId)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var jobResourceId = BackupJobResource.CreateResourceIdentifier( @@ -409,7 +400,6 @@ public async Task> ListJobsAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var rgId = ResourceGroupResource.CreateResourceIdentifier(subscription, resourceGroup); @@ -435,7 +425,6 @@ public async Task GetRecoveryPointAsync( (nameof(subscription), subscription), (nameof(protectedItemName), protectedItemName), (nameof(recoveryPointId), recoveryPointId)); - ValidateSubscriptionFormat(subscription); if (string.IsNullOrEmpty(containerName)) { @@ -465,7 +454,6 @@ public async Task> ListRecoveryPointsAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(protectedItemName), protectedItemName)); - ValidateSubscriptionFormat(subscription); if (string.IsNullOrEmpty(containerName)) { @@ -528,7 +516,6 @@ public async Task UpdateVaultAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -612,7 +599,6 @@ public async Task CreatePolicyAsync( (nameof(subscription), subscription), (nameof(policyName), policyName), (nameof(workloadType), workloadType)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultResourceId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -899,7 +885,6 @@ public async Task ConfigureImmutabilityAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(immutabilityState), immutabilityState)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -931,7 +916,6 @@ public async Task ConfigureSoftDeleteAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(softDeleteState), softDeleteState)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -980,7 +964,6 @@ public async Task ConfigureCrossRegionRestoreAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); var vaultId = RecoveryServicesVaultResource.CreateResourceIdentifier(subscription, resourceGroup, vaultName); @@ -1339,7 +1322,6 @@ public async Task UndeleteProtectedItemAsync( (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription), (nameof(datasourceId), datasourceId)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); @@ -1618,7 +1600,6 @@ public async Task> ListProtectableItemsAsync( (nameof(vaultName), vaultName), (nameof(resourceGroup), resourceGroup), (nameof(subscription), subscription)); - ValidateSubscriptionFormat(subscription); var armClient = await CreateArmClientAsync(tenant, retryPolicy, cancellationToken: cancellationToken); From 51d22a3e5a25b21240515075c3c68eb35461f8b2 Mon Sep 17 00:00:00 2001 From: Shraddha Jain Date: Mon, 11 May 2026 14:55:37 +0530 Subject: [PATCH 2/3] azurebackup: Fix telemetry tags not emitting (null value = no-op) Activity.AddTag(key, null) is a no-op in .NET - the tag is silently dropped. Since most users don't pass --vault-type (auto-detect), NormalizeVaultType returned null, and AddVaultTags became a no-op. Fix: Return 'auto' instead of null for unspecified vault types, and 'unspecified' for workload types. This ensures the tag is always emitted to the Activity and exported to Kusto/App Insights. --- .../src/Models/AzureBackupTelemetryTags.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/Azure.Mcp.Tools.AzureBackup/src/Models/AzureBackupTelemetryTags.cs b/tools/Azure.Mcp.Tools.AzureBackup/src/Models/AzureBackupTelemetryTags.cs index 1154b3ac98..ad39bd4ff2 100644 --- a/tools/Azure.Mcp.Tools.AzureBackup/src/Models/AzureBackupTelemetryTags.cs +++ b/tools/Azure.Mcp.Tools.AzureBackup/src/Models/AzureBackupTelemetryTags.cs @@ -16,17 +16,17 @@ public static class AzureBackupTelemetryTags /// /// Normalizes the vault type to canonical lowercase values (rsv/dpp). - /// Returns null when the input is null or empty. + /// Returns "auto" when the input is null or empty (user didn't specify --vault-type). /// - public static string? NormalizeVaultType(string? vaultType) => - string.IsNullOrWhiteSpace(vaultType) ? null : vaultType.ToLowerInvariant(); + public static string NormalizeVaultType(string? vaultType) => + string.IsNullOrWhiteSpace(vaultType) ? "auto" : vaultType.ToLowerInvariant(); /// /// Normalizes the workload type to canonical lowercase for consistent telemetry. - /// Returns null when the input is null or empty. + /// Returns "unspecified" when the input is null or empty. /// - public static string? NormalizeWorkloadType(string? workloadType) => - string.IsNullOrWhiteSpace(workloadType) ? null : workloadType.ToLowerInvariant(); + public static string NormalizeWorkloadType(string? workloadType) => + string.IsNullOrWhiteSpace(workloadType) ? "unspecified" : workloadType.ToLowerInvariant(); /// /// Adds a normalized vault type tag to the activity. From 53af6184f96556bd71c28b024f62c9ed8dde204b Mon Sep 17 00:00:00 2001 From: Shraddha Jain Date: Mon, 11 May 2026 15:19:15 +0530 Subject: [PATCH 3/3] azurebackup: Add unit tests for AzureBackupTelemetryTags 17 new tests covering: - NormalizeVaultType: null/empty -> 'auto', case normalization - NormalizeWorkloadType: null/empty -> 'unspecified', case normalization - AddVaultTags: null activity safety, null vaultType -> 'auto' tag - AddVaultAndWorkloadTags: both tags emitted with correct values - Tag constants: verify azurebackup/* prefix --- .../src/Services/RsvBackupOperations.cs | 10 -- .../Models/AzureBackupTelemetryTagsTests.cs | 110 ++++++++++++++++++ 2 files changed, 110 insertions(+), 10 deletions(-) create mode 100644 tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Models/AzureBackupTelemetryTagsTests.cs diff --git a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs index 860be5adec..af72337c61 100644 --- a/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs +++ b/tools/Azure.Mcp.Tools.AzureBackup/src/Services/RsvBackupOperations.cs @@ -20,16 +20,6 @@ public sealed class RsvBackupOperations(ITenantService tenantService) : BaseAzur private const string VaultType = VaultTypeResolver.Rsv; private const string FabricName = "Azure"; - private static void ValidateSubscriptionFormat(string subscription) - { - if (!Guid.TryParse(subscription, out _)) - { - throw new ArgumentException( - $"Invalid subscription ID '{subscription}'. Expected a GUID (e.g., 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx'). " + - "If you provided a subscription name, use the subscription ID instead."); - } - } - public async Task CreateVaultAsync( string vaultName, string resourceGroup, string subscription, string location, string? sku, string? storageType, string? tenant, diff --git a/tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Models/AzureBackupTelemetryTagsTests.cs b/tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Models/AzureBackupTelemetryTagsTests.cs new file mode 100644 index 0000000000..b20eeed992 --- /dev/null +++ b/tools/Azure.Mcp.Tools.AzureBackup/tests/Azure.Mcp.Tools.AzureBackup.UnitTests/Models/AzureBackupTelemetryTagsTests.cs @@ -0,0 +1,110 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Diagnostics; +using Azure.Mcp.Tools.AzureBackup.Models; +using Xunit; + +namespace Azure.Mcp.Tools.AzureBackup.UnitTests.Models; + +public class AzureBackupTelemetryTagsTests +{ + [Theory] + [InlineData(null, "auto")] + [InlineData("", "auto")] + [InlineData(" ", "auto")] + [InlineData("RSV", "rsv")] + [InlineData("DPP", "dpp")] + [InlineData("Rsv", "rsv")] + public void NormalizeVaultType_ReturnsExpectedValue(string? input, string expected) + { + Assert.Equal(expected, AzureBackupTelemetryTags.NormalizeVaultType(input)); + } + + [Theory] + [InlineData(null, "unspecified")] + [InlineData("", "unspecified")] + [InlineData(" ", "unspecified")] + [InlineData("VM", "vm")] + [InlineData("SqlDatabase", "sqldatabase")] + [InlineData("AzureFileShare", "azurefileshare")] + public void NormalizeWorkloadType_ReturnsExpectedValue(string? input, string expected) + { + Assert.Equal(expected, AzureBackupTelemetryTags.NormalizeWorkloadType(input)); + } + + [Fact] + public void AddVaultTags_NullActivity_DoesNotThrow() + { + AzureBackupTelemetryTags.AddVaultTags(null, "rsv"); + } + + [Fact] + public void AddVaultTags_NullVaultType_SetsAutoTag() + { + using var listener = new ActivityListener + { + ShouldListenTo = _ => true, + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.AllData, + }; + ActivitySource.AddActivityListener(listener); + + using var source = new ActivitySource("test"); + using var activity = source.StartActivity("test-op"); + Assert.NotNull(activity); + + AzureBackupTelemetryTags.AddVaultTags(activity, null); + + var tag = activity.GetTagItem(AzureBackupTelemetryTags.VaultType); + Assert.Equal("auto", tag); + } + + [Fact] + public void AddVaultTags_WithVaultType_SetsNormalizedTag() + { + using var listener = new ActivityListener + { + ShouldListenTo = _ => true, + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.AllData, + }; + ActivitySource.AddActivityListener(listener); + + using var source = new ActivitySource("test"); + using var activity = source.StartActivity("test-op"); + Assert.NotNull(activity); + + AzureBackupTelemetryTags.AddVaultTags(activity, "RSV"); + + var tag = activity.GetTagItem(AzureBackupTelemetryTags.VaultType); + Assert.Equal("rsv", tag); + } + + [Fact] + public void AddVaultAndWorkloadTags_SetsAllTags() + { + using var listener = new ActivityListener + { + ShouldListenTo = _ => true, + Sample = (ref ActivityCreationOptions _) => ActivitySamplingResult.AllData, + }; + ActivitySource.AddActivityListener(listener); + + using var source = new ActivitySource("test"); + using var activity = source.StartActivity("test-op"); + Assert.NotNull(activity); + + AzureBackupTelemetryTags.AddVaultAndWorkloadTags(activity, null, null); + + Assert.Equal("auto", activity.GetTagItem(AzureBackupTelemetryTags.VaultType)); + Assert.Equal("unspecified", activity.GetTagItem(AzureBackupTelemetryTags.WorkloadType)); + } + + [Fact] + public void TagConstants_HaveCorrectPrefix() + { + Assert.Equal("azurebackup/VaultType", AzureBackupTelemetryTags.VaultType); + Assert.Equal("azurebackup/WorkloadType", AzureBackupTelemetryTags.WorkloadType); + Assert.Equal("azurebackup/DatasourceType", AzureBackupTelemetryTags.DatasourceType); + Assert.Equal("azurebackup/OperationScope", AzureBackupTelemetryTags.OperationScope); + } +}