From 8d02ee994e2d049cadde6eb02305f32256d9680e Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Mon, 30 Mar 2026 23:20:04 +0000 Subject: [PATCH 1/3] Add telemetry tracking with unique rule/diagnostic IDs for preflight validation (#7114) Add telemetry to the local preflight validation system to track validation outcomes, which checks are failing, and user actions after warnings/errors. Changes: - Add DiagnosticID field to PreflightCheckResult for per-finding telemetry - Add PreflightCheck struct with RuleID for per-rule crash tracking - Add validation.preflight span with outcome, diagnostics, and severity counts - Set usage-level attributes for deployment correlation (false positive and true positive tracking when users accept warnings) - Add telemetry field definitions and event constant - Update checkRoleAssignmentPermissions with diagnostic IDs: role_assignment_missing and role_assignment_conditional - Add DiagnosticID to PreflightReportItem for UX layer propagation - Add tests for telemetry emission and ID propagation Telemetry outcomes: - passed: no issues found - warnings_accepted: user chose to continue despite warnings - aborted_by_errors: blocking errors detected - aborted_by_user: user declined to continue after warnings Fixes #7114 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/internal/tracing/events/events.go | 7 ++ cli/azd/internal/tracing/fields/fields.go | 46 +++++++++ .../provisioning/bicep/bicep_provider.go | 79 ++++++++++++++- .../provisioning/bicep/bicep_provider_test.go | 77 +++++++++++++++ .../provisioning/bicep/local_preflight.go | 29 ++++-- .../bicep/local_preflight_test.go | 97 ++++++++++++++----- cli/azd/pkg/output/ux/preflight_report.go | 3 + 7 files changed, 303 insertions(+), 35 deletions(-) diff --git a/cli/azd/internal/tracing/events/events.go b/cli/azd/internal/tracing/events/events.go index d160d895d7b..3a44fe24b00 100644 --- a/cli/azd/internal/tracing/events/events.go +++ b/cli/azd/internal/tracing/events/events.go @@ -38,3 +38,10 @@ const ( // CopilotSessionEvent tracks session creation or resumption. CopilotSessionEvent = "copilot.session" ) + +// Preflight validation events. +const ( + // PreflightValidationEvent tracks the local preflight validation operation + // and its outcome (passed, warnings accepted, aborted). + PreflightValidationEvent = "validation.preflight" +) diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index 5a73962ccf8..cc13c100e01 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -383,6 +383,52 @@ var ( } ) +// Preflight validation related fields +var ( + // PreflightOutcomeKey records the outcome of preflight validation. + // + // Example: "passed", "warnings_accepted", "aborted_by_errors", "aborted_by_user" + PreflightOutcomeKey = AttributeKey{ + Key: attribute.Key("validation.preflight.outcome"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } + + // PreflightDiagnosticsKey records the list of diagnostic IDs emitted by preflight checks. + // + // Example: ["role_assignment_missing", "role_assignment_conditional"] + PreflightDiagnosticsKey = AttributeKey{ + Key: attribute.Key("validation.preflight.diagnostics"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } + + // PreflightRulesKey records the list of rule IDs that were executed. + // + // Example: ["role_assignment_permissions"] + PreflightRulesKey = AttributeKey{ + Key: attribute.Key("validation.preflight.rules"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } + + // PreflightWarningCountKey records the number of warnings produced by preflight validation. + PreflightWarningCountKey = AttributeKey{ + Key: attribute.Key("validation.preflight.warning.count"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + IsMeasurement: true, + } + + // PreflightErrorCountKey records the number of errors produced by preflight validation. + PreflightErrorCountKey = AttributeKey{ + Key: attribute.Key("validation.preflight.error.count"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + IsMeasurement: true, + } +) + // The value used for ServiceNameKey const ServiceNameAzd = "azd" diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 8f00c63a7ca..4c7b398863b 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -47,6 +47,10 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/prompt" "github.com/azure/azure-dev/cli/azd/pkg/tools/bicep" "github.com/drone/envsubst" + + "github.com/azure/azure-dev/cli/azd/internal/tracing" + "github.com/azure/azure-dev/cli/azd/internal/tracing/events" + "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" ) type bicepFileMode int @@ -2133,6 +2137,14 @@ func (p *BicepProvider) convertToDeployment(bicepTemplate azure.ArmTemplate) pro return result } +// Preflight validation outcome constants for telemetry. +const ( + preflightOutcomePassed = "passed" + preflightOutcomeWarningsAccepted = "warnings_accepted" + preflightOutcomeAbortedByErrors = "aborted_by_errors" + preflightOutcomeAbortedByUser = "aborted_by_user" +) + // validatePreflight runs client-side preflight validation on the ARM template. // It returns (abort, err) where: // - abort=true, err=nil: checks detected issues and the deployment should be skipped (exit code 0). @@ -2147,6 +2159,9 @@ func (p *BicepProvider) validatePreflight( tags map[string]*string, options map[string]any, ) (bool, error) { + ctx, span := tracing.Start(ctx, events.PreflightValidationEvent) + defer span.End() + // Run local preflight validation before sending to Azure. // Local validation catches common issues without requiring a network round-trip. localPreflight := newLocalArmPreflight(modulePath, p.bicepCli, target) @@ -2155,20 +2170,48 @@ func (p *BicepProvider) validatePreflight( // local preflight pipeline. The check inspects whether the template contains // Microsoft.Authorization/roleAssignments and, if so, verifies the current // principal has the required write permission. - localPreflight.AddCheck(p.checkRoleAssignmentPermissions) + localPreflight.AddCheck(PreflightCheck{ + RuleID: "role_assignment_permissions", + Fn: p.checkRoleAssignmentPermissions, + }) + + // Record the rule IDs that will be executed for telemetry. + ruleIDs := make([]string, len(localPreflight.checks)) + for i, check := range localPreflight.checks { + ruleIDs[i] = check.RuleID + } + span.SetAttributes(fields.PreflightRulesKey.StringSlice(ruleIDs)) results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) if err != nil { return false, fmt.Errorf("local preflight validation failed: %w", err) } + // Compute telemetry metrics from the results. + var diagnosticIDs []string + var warningCount, errorCount int + for _, result := range results { + if result.DiagnosticID != "" { + diagnosticIDs = append(diagnosticIDs, result.DiagnosticID) + } + if result.Severity == PreflightCheckError { + errorCount++ + } else { + warningCount++ + } + } + span.SetAttributes(fields.PreflightDiagnosticsKey.StringSlice(diagnosticIDs)) + span.SetAttributes(fields.PreflightWarningCountKey.Int(warningCount)) + span.SetAttributes(fields.PreflightErrorCountKey.Int(errorCount)) + // Build a UX report from the preflight results and display it. if len(results) > 0 { report := &ux.PreflightReport{} for _, result := range results { report.Items = append(report.Items, ux.PreflightReportItem{ - IsError: result.Severity == PreflightCheckError, - Message: result.Message, + IsError: result.Severity == PreflightCheckError, + DiagnosticID: result.DiagnosticID, + Message: result.Message, }) } p.console.MessageUxItem(ctx, report) @@ -2178,6 +2221,7 @@ func (p *BicepProvider) validatePreflight( // successfully detected problems and the deployment is intentionally aborted. // This is not an internal failure, so no error is returned (exit code 0). p.console.Message(ctx, "Preflight validation detected errors, deployment aborted.") + p.setPreflightOutcome(span, preflightOutcomeAbortedByErrors, diagnosticIDs) return true, nil } @@ -2192,14 +2236,37 @@ func (p *BicepProvider) validatePreflight( } if !continueDeployment { // User chose not to continue — this is an intentional abort, not a failure. + p.setPreflightOutcome(span, preflightOutcomeAbortedByUser, diagnosticIDs) return true, nil } + p.setPreflightOutcome(span, preflightOutcomeWarningsAccepted, diagnosticIDs) } + } else { + p.setPreflightOutcome(span, preflightOutcomePassed, nil) } return false, target.ValidatePreflight(ctx, armTemplate, armParameters, tags, options) } +// setPreflightOutcome records the preflight outcome on both the span and as a usage-level +// attribute so it can be correlated with the overall deployment result. +func (p *BicepProvider) setPreflightOutcome( + span tracing.Span, + outcome string, + diagnosticIDs []string, +) { + span.SetAttributes(fields.PreflightOutcomeKey.String(outcome)) + + // Set usage-level attributes so the parent command span (cmd.provision / cmd.up) can + // correlate preflight outcome with the final deployment result. This enables tracking + // false positives (warnings_accepted + deploy succeeds) and true positives + // (warnings_accepted + deploy fails). + tracing.SetUsageAttributes( + fields.PreflightOutcomeKey.String(outcome), + fields.PreflightDiagnosticsKey.StringSlice(diagnosticIDs), + ) +} + // checkRoleAssignmentPermissions is a PreflightCheckFn that verifies the current principal // has Microsoft.Authorization/roleAssignments/write permission when the template contains // role assignments. The PermissionsService is resolved lazily via the service locator so it @@ -2251,7 +2318,8 @@ func (p *BicepProvider) checkRoleAssignmentPermissions( if !hasPermission.HasPermission { return &PreflightCheckResult{ - Severity: PreflightCheckWarning, + Severity: PreflightCheckWarning, + DiagnosticID: "role_assignment_missing", Message: fmt.Sprintf( "the current principal (%s) does not have permission to create role assignments "+ "(Microsoft.Authorization/roleAssignments/write) on subscription %s. "+ @@ -2267,7 +2335,8 @@ func (p *BicepProvider) checkRoleAssignmentPermissions( if hasPermission.Conditional { return &PreflightCheckResult{ - Severity: PreflightCheckWarning, + Severity: PreflightCheckWarning, + DiagnosticID: "role_assignment_conditional", Message: fmt.Sprintf( "the current principal (%s) has conditional permission to create role "+ "assignments (Microsoft.Authorization/roleAssignments/write) on "+ diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index 1bb52ef0a11..adaf29f28b2 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -38,9 +38,11 @@ import ( "github.com/azure/azure-dev/cli/azd/test/mocks/mockaccount" "github.com/azure/azure-dev/cli/azd/test/mocks/mockazapi" "github.com/azure/azure-dev/cli/azd/test/mocks/mockenv" + "github.com/azure/azure-dev/cli/azd/test/mocks/mocktracing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" ) func TestBicepPlan(t *testing.T) { @@ -1910,3 +1912,78 @@ func TestHelperEvalParamEnvSubst(t *testing.T) { require.Contains(t, substResult.mappedEnvVars, "VAR2") require.False(t, substResult.hasUnsetEnvVar) } + +func TestSetPreflightOutcome_SetsSpanAndUsageAttributes(t *testing.T) { + span := &mocktracing.Span{} + provider := &BicepProvider{} + + diagnosticIDs := []string{"role_assignment_missing", "role_assignment_conditional"} + provider.setPreflightOutcome(span, preflightOutcomeWarningsAccepted, diagnosticIDs) + + // Verify outcome is set on the span directly. + outcomeAttr := findSpanAttribute(span.Attributes, "validation.preflight.outcome") + require.NotNil(t, outcomeAttr, "expected outcome attribute on span") + require.Equal(t, preflightOutcomeWarningsAccepted, outcomeAttr.Value.AsString()) + + // Diagnostics and outcome are also set as usage-level attributes for + // correlation on the parent command span (cmd.provision / cmd.up). + // Usage attributes are stored globally via tracing.SetUsageAttributes, + // not on this span, so we only verify the span-level outcome above. +} + +func TestSetPreflightOutcome_AllOutcomeValues(t *testing.T) { + tests := []struct { + name string + outcome string + diagnosticIDs []string + }{ + { + name: "passed", + outcome: preflightOutcomePassed, + diagnosticIDs: nil, + }, + { + name: "warnings accepted", + outcome: preflightOutcomeWarningsAccepted, + diagnosticIDs: []string{"role_assignment_missing"}, + }, + { + name: "aborted by errors", + outcome: preflightOutcomeAbortedByErrors, + diagnosticIDs: []string{"role_assignment_missing"}, + }, + { + name: "aborted by user", + outcome: preflightOutcomeAbortedByUser, + diagnosticIDs: []string{"role_assignment_conditional"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + span := &mocktracing.Span{} + provider := &BicepProvider{} + + provider.setPreflightOutcome(span, tt.outcome, tt.diagnosticIDs) + + outcomeAttr := findSpanAttribute( + span.Attributes, "validation.preflight.outcome", + ) + require.NotNil(t, outcomeAttr) + require.Equal(t, tt.outcome, outcomeAttr.Value.AsString()) + }) + } +} + +// findSpanAttribute searches for an attribute by key and returns a pointer to it, or nil. +func findSpanAttribute( + attrs []attribute.KeyValue, + key attribute.Key, +) *attribute.KeyValue { + for i := range attrs { + if attrs[i].Key == key { + return &attrs[i] + } + } + return nil +} diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go index e2b8ba642ca..46d7b373f0c 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight.go @@ -265,6 +265,10 @@ const ( type PreflightCheckResult struct { // Severity indicates whether this result is a warning or a blocking error. Severity PreflightCheckSeverity + // DiagnosticID is a unique, stable identifier for this specific finding type + // (e.g. "role_assignment_missing"). Used in telemetry to correlate actioned + // warnings with deployment outcomes and to track error frequency over time. + DiagnosticID string // Message is a human-readable description of the finding. Message string } @@ -300,6 +304,17 @@ type PreflightCheckFn func( valCtx *validationContext, ) (*PreflightCheckResult, error) +// PreflightCheck pairs a unique rule identifier with its check function. +// The RuleID is a stable, unique string used in telemetry to identify which rule +// produced a result (e.g. for crash tracking). Each rule may emit results with +// different DiagnosticIDs to distinguish specific finding types. +type PreflightCheck struct { + // RuleID is a unique, stable identifier for the rule (e.g. "role_assignment_permissions"). + RuleID string + // Fn is the check function that performs the validation. + Fn PreflightCheckFn +} + // localArmPreflight provides local (client-side) validation of an ARM template before deployment. // It parses the template and parameters to build a comprehensive view of all resources that would // be deployed, enabling early detection of issues without making Azure API calls. @@ -315,7 +330,7 @@ type localArmPreflight struct { // target is the deployment scope (subscription or resource group) used to derive snapshot options. // It may be nil, in which case snapshot options are left empty. target infra.Deployment - checks []PreflightCheckFn + checks []PreflightCheck } // newLocalArmPreflight creates a new instance of localArmPreflight. @@ -326,10 +341,10 @@ func newLocalArmPreflight(modulePath string, bicepCli *bicep.Cli, target infra.D return &localArmPreflight{modulePath: modulePath, bicepCli: bicepCli, target: target} } -// AddCheck registers a preflight check function to be executed during validate. -// Check functions are invoked in the order they are added. -func (l *localArmPreflight) AddCheck(fn PreflightCheckFn) { - l.checks = append(l.checks, fn) +// AddCheck registers a preflight check to be executed during validate. +// Checks are invoked in the order they are added. +func (l *localArmPreflight) AddCheck(check PreflightCheck) { + l.checks = append(l.checks, check) } // validate performs local preflight validation on the given ARM template and parameters. @@ -417,9 +432,9 @@ func (l *localArmPreflight) validate( var results []PreflightCheckResult for _, check := range l.checks { - result, err := check(ctx, valCtx) + result, err := check.Fn(ctx, valCtx) if err != nil { - return results, fmt.Errorf("preflight check failed: %w", err) + return results, fmt.Errorf("preflight check %q failed: %w", check.RuleID, err) } if result != nil { results = append(results, *result) diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go index ddaa2375894..c695865f909 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go @@ -84,41 +84,52 @@ func TestRegisteredChecks_RunInOrder(t *testing.T) { Props: resourcesProperties{}, } - var checks []PreflightCheckFn + var checks []PreflightCheck // Add a warning check - checks = append(checks, func( - ctx context.Context, - valCtx *validationContext, - ) (*PreflightCheckResult, error) { - return &PreflightCheckResult{ - Severity: PreflightCheckWarning, - Message: "this is a warning", - }, nil + checks = append(checks, PreflightCheck{ + RuleID: "warning_rule", + Fn: func( + ctx context.Context, + valCtx *validationContext, + ) (*PreflightCheckResult, error) { + return &PreflightCheckResult{ + Severity: PreflightCheckWarning, + DiagnosticID: "warning_diag", + Message: "this is a warning", + }, nil + }, }) // Add a check that returns nil (no finding) - checks = append(checks, func( - ctx context.Context, - valCtx *validationContext, - ) (*PreflightCheckResult, error) { - return nil, nil + checks = append(checks, PreflightCheck{ + RuleID: "nil_rule", + Fn: func( + ctx context.Context, + valCtx *validationContext, + ) (*PreflightCheckResult, error) { + return nil, nil + }, }) // Add an error check - checks = append(checks, func( - ctx context.Context, - valCtx *validationContext, - ) (*PreflightCheckResult, error) { - return &PreflightCheckResult{ - Severity: PreflightCheckError, - Message: "this is an error", - }, nil + checks = append(checks, PreflightCheck{ + RuleID: "error_rule", + Fn: func( + ctx context.Context, + valCtx *validationContext, + ) (*PreflightCheckResult, error) { + return &PreflightCheckResult{ + Severity: PreflightCheckError, + DiagnosticID: "error_diag", + Message: "this is an error", + }, nil + }, }) var results []PreflightCheckResult for _, check := range checks { - result, err := check(context.Background(), valCtx) + result, err := check.Fn(t.Context(), valCtx) require.NoError(t, err) if result != nil { results = append(results, *result) @@ -127,11 +138,51 @@ func TestRegisteredChecks_RunInOrder(t *testing.T) { require.Len(t, results, 2) require.Equal(t, PreflightCheckWarning, results[0].Severity) + require.Equal(t, "warning_diag", results[0].DiagnosticID) require.Equal(t, "this is a warning", results[0].Message) require.Equal(t, PreflightCheckError, results[1].Severity) + require.Equal(t, "error_diag", results[1].DiagnosticID) require.Equal(t, "this is an error", results[1].Message) } +func TestPreflightCheck_DiagnosticIDPropagation(t *testing.T) { + valCtx := &validationContext{ + Props: resourcesProperties{}, + } + + check := PreflightCheck{ + RuleID: "test_rule", + Fn: func(ctx context.Context, valCtx *validationContext) (*PreflightCheckResult, error) { + return &PreflightCheckResult{ + Severity: PreflightCheckWarning, + DiagnosticID: "test_diagnostic_id", + Message: "test message", + }, nil + }, + } + + result, err := check.Fn(t.Context(), valCtx) + require.NoError(t, err) + require.NotNil(t, result) + require.Equal(t, "test_diagnostic_id", result.DiagnosticID) + require.Equal(t, "test_rule", check.RuleID) +} + +func TestPreflightCheck_ErrorIncludesRuleID(t *testing.T) { + preflight := &localArmPreflight{} + preflight.AddCheck(PreflightCheck{ + RuleID: "failing_rule", + Fn: func(ctx context.Context, valCtx *validationContext) (*PreflightCheckResult, error) { + return nil, fmt.Errorf("something went wrong") + }, + }) + + // We can't call validate() directly without a valid template, but we can + // verify the check struct is stored correctly. + require.Len(t, preflight.checks, 1) + require.Equal(t, "failing_rule", preflight.checks[0].RuleID) +} + func TestArmField_TypedValue(t *testing.T) { input := `{"sku": {"name": "Standard_LRS", "tier": "Standard"}}` var res armTemplateResource diff --git a/cli/azd/pkg/output/ux/preflight_report.go b/cli/azd/pkg/output/ux/preflight_report.go index 8b70383809e..7ac957da34d 100644 --- a/cli/azd/pkg/output/ux/preflight_report.go +++ b/cli/azd/pkg/output/ux/preflight_report.go @@ -15,6 +15,9 @@ import ( type PreflightReportItem struct { // IsError is true for blocking errors, false for warnings. IsError bool + // DiagnosticID is a unique, stable identifier for this finding type (e.g. + // "role_assignment_missing"). Used in telemetry for error correlation. + DiagnosticID string // Message describes the finding. Message string } From 8e111a87346d1eedafacf52d4c4c9ece2218aa05 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 31 Mar 2026 00:18:53 +0000 Subject: [PATCH 2/3] Address PR review feedback - Fix import grouping: merge internal/tracing imports with azure-dev group - Use named returns + deferred span.EndWithStatus(err) for proper span status - Move rules attribute after validate() to avoid reporting skipped checks as executed - Add 'skipped' outcome when bicep snapshot is unavailable (nil results, nil err) - Add 'error' outcome on all error early-return paths (validate failure, prompt error) - Rename TestPreflightCheck_ErrorIncludesRuleID to _AddCheckStoresRuleID - Add usage attribute verification in TestSetPreflightOutcome test - Add 'skipped' and 'error' outcome test cases - Add 'actioned' to cspell overrides for local_preflight.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/.vscode/cspell.yaml | 3 ++ .../provisioning/bicep/bicep_provider.go | 46 ++++++++++++------- .../provisioning/bicep/bicep_provider_test.go | 26 +++++++++-- .../bicep/local_preflight_test.go | 4 +- 4 files changed, 56 insertions(+), 23 deletions(-) diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index f4f38677b7c..593e78cb8cc 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -321,6 +321,9 @@ overrides: - filename: extensions/azure.ai.models/internal/cmd/custom_create.go words: - Qwen + - filename: pkg/infra/provisioning/bicep/local_preflight.go + words: + - actioned ignorePaths: - "**/*_test.go" - "**/mock*.go" diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 4c7b398863b..738ed334493 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -25,6 +25,9 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cognitiveservices/armcognitiveservices" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + "github.com/azure/azure-dev/cli/azd/internal/tracing" + "github.com/azure/azure-dev/cli/azd/internal/tracing/events" + "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/ai" "github.com/azure/azure-dev/cli/azd/pkg/async" @@ -47,10 +50,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/prompt" "github.com/azure/azure-dev/cli/azd/pkg/tools/bicep" "github.com/drone/envsubst" - - "github.com/azure/azure-dev/cli/azd/internal/tracing" - "github.com/azure/azure-dev/cli/azd/internal/tracing/events" - "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" ) type bicepFileMode int @@ -2143,6 +2142,8 @@ const ( preflightOutcomeWarningsAccepted = "warnings_accepted" preflightOutcomeAbortedByErrors = "aborted_by_errors" preflightOutcomeAbortedByUser = "aborted_by_user" + preflightOutcomeSkipped = "skipped" + preflightOutcomeError = "error" ) // validatePreflight runs client-side preflight validation on the ARM template. @@ -2158,9 +2159,11 @@ func (p *BicepProvider) validatePreflight( armParameters azure.ArmParameters, tags map[string]*string, options map[string]any, -) (bool, error) { +) (abort bool, err error) { ctx, span := tracing.Start(ctx, events.PreflightValidationEvent) - defer span.End() + defer func() { + span.EndWithStatus(err) + }() // Run local preflight validation before sending to Azure. // Local validation catches common issues without requiring a network round-trip. @@ -2175,18 +2178,24 @@ func (p *BicepProvider) validatePreflight( Fn: p.checkRoleAssignmentPermissions, }) - // Record the rule IDs that will be executed for telemetry. + results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) + if err != nil { + p.setPreflightOutcome(span, preflightOutcomeError, nil) + return false, fmt.Errorf("local preflight validation failed: %w", err) + } + + // Record the rule IDs that actually executed. This is done after validate() + // because validate() may skip checks entirely (e.g. when the bicep snapshot + // is unavailable). A nil results with nil error means checks were skipped. + if results == nil { + p.setPreflightOutcome(span, preflightOutcomeSkipped, nil) + } ruleIDs := make([]string, len(localPreflight.checks)) for i, check := range localPreflight.checks { ruleIDs[i] = check.RuleID } span.SetAttributes(fields.PreflightRulesKey.StringSlice(ruleIDs)) - results, err := localPreflight.validate(ctx, p.console, armTemplate, armParameters) - if err != nil { - return false, fmt.Errorf("local preflight validation failed: %w", err) - } - // Compute telemetry metrics from the results. var diagnosticIDs []string var warningCount, errorCount int @@ -2227,12 +2236,17 @@ func (p *BicepProvider) validatePreflight( if report.HasWarnings() { continueDeployment, promptErr := p.console.Confirm(ctx, input.ConsoleOptions{ - Message: "Preflight validation found warnings that may cause the deployment to fail. " + - "Do you want to continue?", + Message: "Preflight validation found warnings that may cause the " + + "deployment to fail. Do you want to continue?", DefaultValue: true, }) if promptErr != nil { - return false, fmt.Errorf("prompting for preflight confirmation: %w", promptErr) + p.setPreflightOutcome( + span, preflightOutcomeError, diagnosticIDs, + ) + return false, fmt.Errorf( + "prompting for preflight confirmation: %w", promptErr, + ) } if !continueDeployment { // User chose not to continue — this is an intentional abort, not a failure. @@ -2241,7 +2255,7 @@ func (p *BicepProvider) validatePreflight( } p.setPreflightOutcome(span, preflightOutcomeWarningsAccepted, diagnosticIDs) } - } else { + } else if results != nil { p.setPreflightOutcome(span, preflightOutcomePassed, nil) } diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index adaf29f28b2..e923778e059 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -22,6 +22,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/appconfiguration/armappconfiguration" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/keyvault/armkeyvault" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + "github.com/azure/azure-dev/cli/azd/internal/tracing" "github.com/azure/azure-dev/cli/azd/pkg/account" "github.com/azure/azure-dev/cli/azd/pkg/azapi" "github.com/azure/azure-dev/cli/azd/pkg/azure" @@ -1925,10 +1926,17 @@ func TestSetPreflightOutcome_SetsSpanAndUsageAttributes(t *testing.T) { require.NotNil(t, outcomeAttr, "expected outcome attribute on span") require.Equal(t, preflightOutcomeWarningsAccepted, outcomeAttr.Value.AsString()) - // Diagnostics and outcome are also set as usage-level attributes for - // correlation on the parent command span (cmd.provision / cmd.up). - // Usage attributes are stored globally via tracing.SetUsageAttributes, - // not on this span, so we only verify the span-level outcome above. + // Verify usage-level attributes are set for parent command span correlation. + usageAttrs := tracing.GetUsageAttributes() + usageOutcome := findSpanAttribute(usageAttrs, "validation.preflight.outcome") + require.NotNil(t, usageOutcome, "expected outcome in usage attributes") + require.Equal(t, preflightOutcomeWarningsAccepted, usageOutcome.Value.AsString()) + + usageDiag := findSpanAttribute( + usageAttrs, "validation.preflight.diagnostics", + ) + require.NotNil(t, usageDiag, "expected diagnostics in usage attributes") + require.Equal(t, diagnosticIDs, usageDiag.Value.AsStringSlice()) } func TestSetPreflightOutcome_AllOutcomeValues(t *testing.T) { @@ -1957,6 +1965,16 @@ func TestSetPreflightOutcome_AllOutcomeValues(t *testing.T) { outcome: preflightOutcomeAbortedByUser, diagnosticIDs: []string{"role_assignment_conditional"}, }, + { + name: "skipped", + outcome: preflightOutcomeSkipped, + diagnosticIDs: nil, + }, + { + name: "error", + outcome: preflightOutcomeError, + diagnosticIDs: nil, + }, } for _, tt := range tests { diff --git a/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go b/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go index c695865f909..2a2b42e4139 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/local_preflight_test.go @@ -168,7 +168,7 @@ func TestPreflightCheck_DiagnosticIDPropagation(t *testing.T) { require.Equal(t, "test_rule", check.RuleID) } -func TestPreflightCheck_ErrorIncludesRuleID(t *testing.T) { +func TestPreflightCheck_AddCheckStoresRuleID(t *testing.T) { preflight := &localArmPreflight{} preflight.AddCheck(PreflightCheck{ RuleID: "failing_rule", @@ -177,8 +177,6 @@ func TestPreflightCheck_ErrorIncludesRuleID(t *testing.T) { }, }) - // We can't call validate() directly without a valid template, but we can - // verify the check struct is stored correctly. require.Len(t, preflight.checks, 1) require.Equal(t, "failing_rule", preflight.checks[0].RuleID) } From 84d49110b7927b68eaf101db52f80b2bf836be53 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 31 Mar 2026 05:36:28 +0000 Subject: [PATCH 3/3] Address second round of PR feedback - Fix import grouping: move envsubst to external group, azd internal imports in separate group (stdlib -> external -> azd internal) - Only set PreflightRulesKey when checks actually ran; use empty slice for skipped outcome - Fix grammar: 'A nil results' -> 'A nil result' - Update PreflightOutcomeKey doc to list all 6 outcomes - Remove duplicate envsubst import Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/internal/tracing/fields/fields.go | 3 ++- .../infra/provisioning/bicep/bicep_provider.go | 18 +++++++++++------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index cc13c100e01..6acda18acf7 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -387,7 +387,8 @@ var ( var ( // PreflightOutcomeKey records the outcome of preflight validation. // - // Example: "passed", "warnings_accepted", "aborted_by_errors", "aborted_by_user" + // Example: "passed", "warnings_accepted", "aborted_by_errors", + // "aborted_by_user", "skipped", "error" PreflightOutcomeKey = AttributeKey{ Key: attribute.Key("validation.preflight.outcome"), Classification: SystemMetadata, diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go index 738ed334493..12079d36cbe 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider.go @@ -25,6 +25,8 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/cognitiveservices/armcognitiveservices" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources" + "github.com/drone/envsubst" + "github.com/azure/azure-dev/cli/azd/internal/tracing" "github.com/azure/azure-dev/cli/azd/internal/tracing/events" "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" @@ -49,7 +51,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/password" "github.com/azure/azure-dev/cli/azd/pkg/prompt" "github.com/azure/azure-dev/cli/azd/pkg/tools/bicep" - "github.com/drone/envsubst" ) type bicepFileMode int @@ -2186,15 +2187,18 @@ func (p *BicepProvider) validatePreflight( // Record the rule IDs that actually executed. This is done after validate() // because validate() may skip checks entirely (e.g. when the bicep snapshot - // is unavailable). A nil results with nil error means checks were skipped. + // is unavailable). A nil result with nil error means checks were skipped. if results == nil { p.setPreflightOutcome(span, preflightOutcomeSkipped, nil) + // No rules actually executed; record an empty slice for telemetry. + span.SetAttributes(fields.PreflightRulesKey.StringSlice([]string{})) + } else { + ruleIDs := make([]string, len(localPreflight.checks)) + for i, check := range localPreflight.checks { + ruleIDs[i] = check.RuleID + } + span.SetAttributes(fields.PreflightRulesKey.StringSlice(ruleIDs)) } - ruleIDs := make([]string, len(localPreflight.checks)) - for i, check := range localPreflight.checks { - ruleIDs[i] = check.RuleID - } - span.SetAttributes(fields.PreflightRulesKey.StringSlice(ruleIDs)) // Compute telemetry metrics from the results. var diagnosticIDs []string