From 711f4338a5eab6a5ac1c2a9cc07aa7ccec340870 Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 14 Aug 2026 21:52:55 -0700 Subject: [PATCH 01/18] use fields not raw attributes so GDPR classification works --- cli/azd/cmd/auth_login.go | 5 +- cli/azd/cmd/telemetry_test.go | 160 +++++++++++++++++++++- cli/azd/internal/tracing/fields/fields.go | 35 +++++ cli/azd/pkg/project/container_helper.go | 3 +- cli/azd/pkg/project/service_target_aks.go | 21 ++- 5 files changed, 211 insertions(+), 13 deletions(-) diff --git a/cli/azd/cmd/auth_login.go b/cli/azd/cmd/auth_login.go index 853d6a239c8..fcaca2f1cee 100644 --- a/cli/azd/cmd/auth_login.go +++ b/cli/azd/cmd/auth_login.go @@ -33,7 +33,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/tools/github" "github.com/spf13/cobra" "github.com/spf13/pflag" - "go.opentelemetry.io/otel/attribute" ) // The parent of the login command. @@ -378,11 +377,11 @@ func (la *loginAction) Run(ctx context.Context) (*actions.ActionResult, error) { if !isServicePrincipalOrMI { if _, err := la.authManager.LogInDetails(ctx); !errors.Is(err, auth.ErrNoCurrentUser) { if err := la.authManager.CleanAllAuthCache(); err != nil { - tracing.SetUsageAttributes(attribute.String("auth.cache_clear_failed", "auth")) + tracing.SetUsageAttributes(fields.AuthCacheClearFailedKey.String("auth")) return nil, fmt.Errorf("clearing auth cache: %w", err) } if err := la.accountSubManager.ClearSubscriptions(ctx); err != nil { - tracing.SetUsageAttributes(attribute.String("auth.cache_clear_failed", "subscriptions")) + tracing.SetUsageAttributes(fields.AuthCacheClearFailedKey.String("subscriptions")) return nil, fmt.Errorf("clearing subscriptions cache: %w", err) } } diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 380c21fb88f..cf8593ee117 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -4,6 +4,13 @@ package cmd import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" "testing" "github.com/stretchr/testify/require" @@ -26,7 +33,8 @@ func TestTelemetryEventConstants(t *testing.T) { // NOTE: This test validates field definitions, not command-level instrumentation. // Command-level coverage is enforced via the documented allowlist in // TestCommandTelemetryCoverageAllowlist (below) and the feature-telemetry-matrix.md. -// Full AST-based scanning of SetUsageAttributes calls is a future enhancement. +// Raw attribute.* string-literal keys in telemetry sinks are additionally +// rejected by TestNoRawTelemetryAttributes (below). func TestTelemetryFieldConstants(t *testing.T) { t.Parallel() // Auth command telemetry fields @@ -47,6 +55,15 @@ func TestTelemetryFieldConstants(t *testing.T) { kv := fields.AuthMethodKey.String(method) require.NotEmpty(t, kv.Value.AsString()) } + + // Cache-clear failure indicator (fixed enum, emitted on `auth login`). + kvCache := fields.AuthCacheClearFailedKey.String("auth") + require.Equal(t, "auth.cache_clear_failed", string(kvCache.Key)) + require.Equal(t, "auth", kvCache.Value.AsString()) + for _, which := range []string{"auth", "subscriptions"} { + kv := fields.AuthCacheClearFailedKey.String(which) + require.NotEmpty(t, kv.Value.AsString()) + } }) // Env command telemetry fields @@ -254,6 +271,147 @@ func TestTelemetryFieldConstants(t *testing.T) { require.NotEmpty(t, kv.Value.AsString()) } }) + + // Container publish telemetry fields + t.Run("ContainerFields", func(t *testing.T) { + t.Parallel() + kv := fields.ContainerPublishRemoteBuildKey.Bool(true) + require.Equal(t, "container.publish.remotebuild", string(kv.Key)) + require.Equal(t, true, kv.Value.AsBool()) + }) + + // AKS service target telemetry fields + t.Run("AksFields", func(t *testing.T) { + t.Parallel() + kv := fields.AksSkipReasonKey.String("cluster_not_provisioned") + require.Equal(t, "skip.reason", string(kv.Key)) + require.Equal(t, "cluster_not_provisioned", kv.Value.AsString()) + }) +} + +// TestNoRawTelemetryAttributes enforces that product code never emits telemetry +// via raw attribute.String("literal", ...) / attribute.Bool("literal", ...) etc. +// Every telemetry attribute must be declared as a fields.AttributeKey (with a +// Classification and Purpose) and emitted through it, e.g. +// fields.SomeKey.String(value). This keeps the telemetry schema discoverable and +// classifiable for the GDPR metadata pipeline (azure-dev issue #1803). +// +// Legitimately excluded from the scan: +// - *_test.go files (test fixtures build raw attributes on purpose). +// - internal/tracing/... — the tracing/baggage plumbing that the +// fields.AttributeKey abstraction is itself built on top of. +// - extensions/... — independent extension modules with their own schema. +func TestNoRawTelemetryAttributes(t *testing.T) { + t.Parallel() + + // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers + // that build a KeyValue from a string-literal key. Using them directly in + // product code bypasses the fields.AttributeKey registry, so the GDPR metadata + // exporter (which discovers only exported AttributeKey vars) can never classify + // the resulting property. See docs/specs/metrics-audit/telemetry-schema.md. + rawAttributeConstructors := map[string]struct{}{ + "String": {}, + "Bool": {}, + "Int": {}, + "Int64": {}, + "Float64": {}, + "Stringer": {}, + "StringSlice": {}, + "BoolSlice": {}, + "Int64Slice": {}, + "Float64Slice": {}, + } + + // The test runs with its package directory (cli/azd/cmd) as the working + // directory, so the module root is one level up. + azdRoot, err := filepath.Abs("..") + require.NoError(t, err) + + var violations []string + + err = filepath.Walk(azdRoot, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + if info.IsDir() { + switch filepath.Base(path) { + case "vendor", "extensions", "testdata", "node_modules", ".git": + return filepath.SkipDir + } + return nil + } + + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + + rel, relErr := filepath.Rel(azdRoot, path) + if relErr != nil { + rel = path + } + rel = filepath.ToSlash(rel) + + // The tracing/baggage plumbing is the sanctioned home for raw attribute + // construction; the fields.AttributeKey abstraction is built on it. + if strings.HasPrefix(rel, "internal/tracing/") { + return nil + } + + fset := token.NewFileSet() + file, parseErr := parser.ParseFile(fset, path, nil, 0) + if parseErr != nil { + return nil // skip unparseable files + } + + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || len(call.Args) == 0 { + return true + } + + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + + pkgIdent, ok := sel.X.(*ast.Ident) + if !ok || pkgIdent.Name != "attribute" { + return true + } + + if _, isConstructor := rawAttributeConstructors[sel.Sel.Name]; !isConstructor { + return true + } + + // Only flag string-literal keys. A non-literal key (e.g. a dynamic + // extension field) is a separate, intentional pattern. + lit, ok := call.Args[0].(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return true + } + + pos := fset.Position(call.Pos()) + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, sel.Sel.Name, lit.Value)) + return true + }) + + return nil + }) + require.NoError(t, err) + + if len(violations) > 0 { + t.Errorf( + "Found %d raw telemetry attribute(s) using a string-literal key.\n"+ + "Declare an exported fields.AttributeKey (with Classification and Purpose) in\n"+ + "internal/tracing/fields/fields.go and emit via it, e.g. fields.MyKey.String(v),\n"+ + "so the property is discoverable and classifiable by the GDPR metadata pipeline.\n\n"+ + "Raw attributes:\n%s", + len(violations), + strings.Join(violations, "\n"), + ) + } } // TestCommandTelemetryCoverage ensures every user-facing command is explicitly categorized diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index 161e3cbd4fd..c416fb11dc0 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -360,6 +360,17 @@ var ( Classification: SystemMetadata, Purpose: FeatureInsight, } + + // AuthCacheClearFailedKey records which cache failed to clear during the + // re-login cleanup that runs before a fresh login. It is a fixed enum + // (not user-derived), so it is emitted raw (not hashed). Emitted on the + // `auth login` usage event. + // Values: "auth" (credential cache), "subscriptions" (subscription cache). + AuthCacheClearFailedKey = AttributeKey{ + Key: attribute.Key("auth.cache_clear_failed"), + Classification: SystemMetadata, + Purpose: PerformanceAndHealth, + } ) // Environment command related fields @@ -1053,6 +1064,19 @@ var ( } ) +// AKS service target related fields +var ( + // AksSkipReasonKey records why AKS postprovision Kubernetes context setup + // was skipped, as a bounded, low-cardinality code (never raw error text). + // Emitted on the `aks.postprovision.skip` event. + // Values: "cluster_not_provisioned". + AksSkipReasonKey = AttributeKey{ + Key: attribute.Key("skip.reason"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } +) + // Mcp related fields var ( // The name of the MCP client. @@ -1129,6 +1153,17 @@ var ( Purpose: FeatureInsight, IsMeasurement: true, } + + // ContainerPublishRemoteBuildKey records whether the container image was + // built remotely (ACR build) rather than locally for a container publish. + // It is a boolean (fixed cardinality), so it is emitted raw (not hashed). + // Emitted on the `container.publish` event. This is distinct from the + // `container.remotebuild` event, which marks the Azure-side remote build. + ContainerPublishRemoteBuildKey = AttributeKey{ + Key: attribute.Key("container.publish.remotebuild"), + Classification: SystemMetadata, + Purpose: FeatureInsight, + } ) // JSON-RPC related fields diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 52c6b2d941e..5f43ae970ae 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -41,7 +41,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/tools/pack" "github.com/benbjohnson/clock" "github.com/sethvargo/go-retry" - "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" ) @@ -625,7 +624,7 @@ func (ch *ContainerHelper) Publish( ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) defer func() { span.EndWithStatus(err) }() span.SetAttributes( - attribute.Bool("container.remotebuild", serviceConfig.Docker.RemoteBuild), + fields.ContainerPublishRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild), ) var remoteImage string diff --git a/cli/azd/pkg/project/service_target_aks.go b/cli/azd/pkg/project/service_target_aks.go index bd85ec71a1c..e2a1a3d718d 100644 --- a/cli/azd/pkg/project/service_target_aks.go +++ b/cli/azd/pkg/project/service_target_aks.go @@ -17,6 +17,7 @@ import ( "github.com/azure/azure-dev/cli/azd/internal/mapper" "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/alpha" "github.com/azure/azure-dev/cli/azd/pkg/async" "github.com/azure/azure-dev/cli/azd/pkg/azapi" @@ -32,7 +33,6 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/tools" "github.com/azure/azure-dev/cli/azd/pkg/tools/kubectl" "github.com/sethvargo/go-retry" - "go.opentelemetry.io/otel/attribute" ) const ( @@ -944,8 +944,7 @@ func (t *aksTarget) setK8sContext( // (credentials, RBAC, namespace) are real failures that should surface // even during postprovision. if targetResource.ResourceName() == "" && eventName == postProvisionEvent { - return t.skipPostprovisionK8sSetup( - ctx, fmt.Errorf("AKS cluster resource not yet provisioned")) + return t.skipPostprovisionK8sSetup(ctx, aksSkipClusterNotProvisioned) } defaultNamespace := t.getK8sNamespace(serviceConfig) @@ -973,13 +972,21 @@ func (t *aksTarget) setK8sContext( return nil } +// aksSkipClusterNotProvisioned is the bounded skip.reason code emitted on +// events.AksPostprovisionSkipEvent when the AKS cluster resource does not exist +// yet (delayed provisioning during multi-phase workflows) and Kubernetes +// context setup is deferred until a deployment is performed. Reason codes must +// stay a small, compile-time enum — never raw or user-derived text. Keep in +// sync with docs/reference/telemetry-data.md. +const aksSkipClusterNotProvisioned = "cluster_not_provisioned" + // skipPostprovisionK8sSetup logs a warning and returns nil so that -// postprovision continues even when Kubernetes context setup fails. +// postprovision continues even when Kubernetes context setup is skipped. // The context will be configured later when a deployment is performed. // Context cancellation/timeouts are always propagated. func (t *aksTarget) skipPostprovisionK8sSetup( ctx context.Context, - reason error, + reason string, ) error { // Propagate context cancellation — the user (or system) asked // to stop; swallowing that would be incorrect. @@ -988,11 +995,11 @@ func (t *aksTarget) skipPostprovisionK8sSetup( } _, span := tracing.Start(ctx, events.AksPostprovisionSkipEvent) - span.SetAttributes(attribute.String("skip.reason", reason.Error())) + span.SetAttributes(fields.AksSkipReasonKey.String(reason)) span.End() log.Printf( - "skipping k8s context setup during postprovision: %v", reason) + "skipping k8s context setup during postprovision: %s", reason) t.console.Message(ctx, output.WithWarningFormat( "AKS cluster not available yet, skipping Kubernetes "+ "context setup. It will be configured when the "+ From 7ff40590688b7581eed92cb2706dae5c642e333b Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 14 Aug 2026 22:13:44 -0700 Subject: [PATCH 02/18] add docs --- cli/azd/AGENTS.md | 29 +++++++---- docs/guides/feature-telemetry.md | 27 +++++++++- docs/reference/telemetry-data.md | 10 ++++ .../metrics-audit/feature-telemetry-matrix.md | 4 +- .../metrics-audit/privacy-review-checklist.md | 10 +++- docs/specs/metrics-audit/telemetry-schema.md | 51 ++++++++++++++++++- 6 files changed, 115 insertions(+), 16 deletions(-) diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index 42c48967c41..4576cba04de 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -279,20 +279,31 @@ public reference, and downstream Kusto/LENS consumers drift out of sync. Verify **1. Code** -- **Field** — define an `AttributeKey` in `cli/azd/internal/tracing/fields/fields.go` (this file - holds the field/key definitions; within the same package `features.go` holds feature-name - attribute values and `domains.go` the Azure host-domain table). Every field MUST set a - `Classification` (e.g. `SystemMetadata`, `OrganizationalIdentifiableInformation`, - `EndUserPseudonymizedInformation`; never emit `CustomerContent`) and a `Purpose` - (`FeatureInsight` / `BusinessInsight` / `PerformanceAndHealth`). +- **Field** — define an **exported, package-level** `AttributeKey` var in + `cli/azd/internal/tracing/fields/fields.go` (this file holds the field/key definitions; within the + same package `features.go` holds feature-name attribute values and `domains.go` the Azure + host-domain table). Every field MUST set a `Classification` (e.g. `SystemMetadata`, + `OrganizationalIdentifiableInformation`, `EndUserPseudonymizedInformation`; never emit + `CustomerContent`) and a `Purpose` (`FeatureInsight` / `BusinessInsight` / + `PerformanceAndHealth`); the `--fields` scan also reads the optional `Endpoint` (→ `EndpointIdType`) + and `IsMeasurement` (→ `$.Measurements` vs `$.Properties`) members. - **Event** — define a constant in `cli/azd/internal/tracing/events/events.go` following the - `prefix.noun.verb` naming convention. + `prefix.noun.verb` value convention. It must be an exported string `const` whose Go identifier + contains `Event` (end it with `Prefix` for a prefix-match group) so the classifier's `--events` + scan discovers it. - **Emit** at the call site via `tracing.Start` (spans/events) plus `tracing.SetUsageAttributes` - or `span.SetAttributes` (attributes). + or `span.SetAttributes` (attributes). Always pass a `fields.AttributeKey` method + (e.g. `fields.MyKey.String(v)` / `.Bool(v)` / `.Int(v)`) — never a raw + `attribute.String("my.key", v)`. The GDPR classifier discovers fields by statically scanning + the `fields` package for exported `AttributeKey` vars; a raw literal key is invisible to it, so the + property reaches App Insights but its data-catalog row stays Unclassified / `Complete=false`. Enforced by + `TestNoRawTelemetryAttributes` (`cli/azd/cmd/telemetry_test.go`); dynamic + `ext.*` keys are the only sanctioned exception. - **Hash user-derived values** with `fields.StringHashed` / `fields.StringSliceHashed` (`cli/azd/internal/tracing/fields/key.go`). Hash anything that embeds a user-chosen name, path, repo URL, or project / env / service / layer identifier (e.g. `exegraph.step.name`, `hooks.name`). - Emit raw only for fixed enums or compile-time literals. + Emit raw only for fixed enums or compile-time literals (the key itself must + still be a `fields.AttributeKey`, per **Emit** above). **2. Documentation — keep all of these in sync** diff --git a/docs/guides/feature-telemetry.md b/docs/guides/feature-telemetry.md index cc06b6eae7c..f528c4337db 100644 --- a/docs/guides/feature-telemetry.md +++ b/docs/guides/feature-telemetry.md @@ -43,16 +43,29 @@ const ( > `events.GetCommandEventName(...)`. You only need to define explicit event constants for > non-command operations (sub-spans, background work, etc.). +> [!IMPORTANT] +> The GDPR classifier discovers events by statically scanning the `events` package for **exported +> string constants whose Go identifier contains `Event`** (e.g. `MyFeatureEvent`). A constant that +> omits `Event` from its identifier is silently skipped and never classified, even if it is emitted. +> End the identifier with `Prefix` (e.g. `MyFeatureEventPrefix`) to register a prefix group that +> classifies every event name starting with that prefix. See +> [Telemetry Schema → Event discovery contract](../specs/metrics-audit/telemetry-schema.md#event-discovery-contract). + ## Step 2: Define Your Fields **File:** `cli/azd/internal/tracing/fields/fields.go` -Add `AttributeKey` variables for any new properties your feature emits. Every field must have: +Add **exported, package-level** `AttributeKey` variables for any new properties your feature emits +(the GDPR classifier only discovers exported `AttributeKey` vars declared in the `fields` package — +see [Field discovery contract](../specs/metrics-audit/telemetry-schema.md#field-attribute-discovery-contract)). +Every field must have: 1. **A key name** — descriptive, dot-separated, lowercase 2. **A classification** — what kind of data is this (see [Data Classifications](#data-classifications)) 3. **A purpose** — why are we collecting it (see [Purposes](#purposes)) -4. **`IsMeasurement: true`** if the value is numeric (goes to `Measurements` column, not `Properties`) +4. **`IsMeasurement: true`** if the value is numeric (routes to `$.Measurements`, not `$.Properties`) +5. **`Endpoint`** (optional) — an identifier-type tag (e.g. `AzureSubscriptionId`) when the value is a + known endpoint identifier; leave unset otherwise (defaults to `N/A`) ```go // In fields.go — add your field keys @@ -106,6 +119,16 @@ tracing.SetUsageAttributes( ## Step 3: Instrument Your Code +> [!IMPORTANT] +> **Always emit through a `fields.AttributeKey` method — never a raw `attribute.*` call with a literal key.** +> Use `fields.MyFeatureStrategyKey.String(v)`, not `attribute.String("myfeature.strategy", v)`. +> The GDPR classification tool discovers fields by statically scanning `fields.go` for exported +> `AttributeKey` variables and reading their `Classification`/`Purpose`. A raw `attribute.*` call +> with a literal key is invisible to that scan, so the property still flows to App Insights but its +> data-catalog row stays **Unclassified / `Complete=false`**. The `TestNoRawTelemetryAttributes` guard +> (`cli/azd/cmd/telemetry_test.go`) fails the build on raw string-literal keys in +> product code. Dynamic, non-fixed keys (e.g. the `ext.*` extension path) are the only exception. + ### For Command Actions The telemetry middleware (`cmd/middleware/telemetry.go`) automatically creates a span for every command. You just need to add your feature-specific attributes: diff --git a/docs/reference/telemetry-data.md b/docs/reference/telemetry-data.md index 63fbbf1fb33..aa17d7aeaad 100644 --- a/docs/reference/telemetry-data.md +++ b/docs/reference/telemetry-data.md @@ -306,6 +306,7 @@ Set **only when an external command-line tool invocation fails**, during error c | Field Key | Type | Values | |-----------|------|--------| | `auth.method` | string | `browser`, `device-code`, `service-principal-secret`, `service-principal-certificate`, `federated-github`, `federated-azure-pipelines`, `federated-oidc`, `managed-identity`, `external`, `oneauth`, `check-status` | +| `auth.cache_clear_failed` | string | `auth`, `subscriptions` — which cache failed to clear during the pre-login cleanup. Emitted on `auth login`. |
@@ -426,6 +427,15 @@ Emitted at provision start by the `microsoft.foundry` provisioning provider (the | Field Key | Type | Description | |-----------|------|-------------| | `container.remoteBuild.count` | measurement | Number of remote container builds performed | +| `container.publish.remotebuild` | bool | Whether the image was built remotely (ACR) rather than locally. Emitted on `container.publish`. Distinct from the `container.remotebuild` event. | +
+ +
+AKS + +| Field Key | Type | Description | +|-----------|------|-------------| +| `skip.reason` | string | Why AKS postprovision Kubernetes context setup was skipped. Bounded enum: `cluster_not_provisioned`. Emitted on `aks.postprovision.skip`. |
diff --git a/docs/specs/metrics-audit/feature-telemetry-matrix.md b/docs/specs/metrics-audit/feature-telemetry-matrix.md index 99f59d393d6..8c2f48910e8 100644 --- a/docs/specs/metrics-audit/feature-telemetry-matrix.md +++ b/docs/specs/metrics-audit/feature-telemetry-matrix.md @@ -161,9 +161,9 @@ reserved field contracts. | **ARM deployment client** | `provision` (any Bicep flow) | `arm.deploy.subscription`, `arm.deploy.resourcegroup`, `arm.stack.deploy.subscription`, `arm.stack.deploy.resourcegroup`, `arm.whatif.subscription`, `arm.whatif.resourcegroup`, `arm.validate.subscription`, `arm.validate.resourcegroup` | ARM operation status + duration | Per-call instrumentation in the ARM client; covers regular + stack deployments at both scopes | | **Multi-layer provision** | `provision` (when `infra.layers[]` is configured in `azure.yaml`) | (none — enriches the `provision` span) | `provision.layer.count`, `provision.layer.max_parallel`, `provision.layer.safe_fallback_count`, `provision.layer.explicit_dependson_count` | All four are integer measurements emitted from `internal/cmd/provision_graph.go`; no per-layer duration or outcome attribute is emitted | | **Execution graph (scheduler)** | `up`, `provision`, `deploy`, `package`, `publish`, `down` | `exegraph.run`, `exegraph.step` | `exegraph.step.count`, `exegraph.max_concurrency`, `exegraph.error_policy`, `exegraph.step.name` (hashed), `exegraph.step.deps` (hashed slice), `exegraph.step.tags` (raw — hardcoded literals only), `exegraph.step.timeout_s` | Step names embed user-defined service / layer names from `azure.yaml`; both `name` and `deps` use `fields.StringHashed` / `fields.StringSliceHashed` | -| **Container lifecycle** | `package`, `deploy` (container service targets) | `container.credentials`, `container.publish`, `container.remotebuild` | `container.publish` sets `container.remotebuild` (bool) only; `container.credentials` and `container.remotebuild` set no attributes (span status carries success/failure and duration) | The hashed `pack.builder.image` / `pack.builder.tag` attributes are emitted on the separate `tools.pack.build` span, not the `container.*` spans | +| **Container lifecycle** | `package`, `deploy` (container service targets) | `container.credentials`, `container.publish`, `container.remotebuild` | `container.publish` sets `container.publish.remotebuild` (bool) only; `container.credentials` and `container.remotebuild` set no attributes (span status carries success/failure and duration) | The hashed `pack.builder.image` / `pack.builder.tag` attributes are emitted on the separate `tools.pack.build` span, not the `container.*` spans | | **App Service deploy** | `deploy`, `publish` (App Service targets) | `deploy.appservice.zip` | `deploy.appservice.linux` (bool), `deploy.appservice.attempt` (retry attempt number) | Zip-deploy path only; outcome / duration are carried by the span status and span timing, not by dedicated attributes | -| **AKS service target** | `provision` (AKS preprovision/postprovision) | `aks.postprovision.skip` | Skip reason | Recorded when cluster is not yet available for context setup | +| **AKS service target** | `provision` (AKS preprovision/postprovision) | `aks.postprovision.skip` | `skip.reason` (bounded enum — `cluster_not_provisioned`) | Recorded when cluster is not yet available for context setup | | **Agent troubleshoot middleware** | Triggered on command failure when troubleshooting is engaged | `agent.troubleshoot` | Error chain attributes, hashed error fields | Emitted from `cmd/middleware/error.go` | | **Up-graph performance** | `up` (graph execution) | (none — enriches the `up` command span) | `perf.provision_duration_ms`, `perf.deploy_duration_ms`, `perf.total_duration_ms` | Emitted from `internal/cmd/up_graph.go` after the graph completes; provision/deploy durations set only when those phases run | | **VS RPC** | `vs-server` long-running session | `vsrpc.*` (event prefix) | Per-RPC attributes documented in `telemetry-schema.md` | Long-running RPC server for VS integration | diff --git a/docs/specs/metrics-audit/privacy-review-checklist.md b/docs/specs/metrics-audit/privacy-review-checklist.md index 8da9aecd8d7..4a59ec531c3 100644 --- a/docs/specs/metrics-audit/privacy-review-checklist.md +++ b/docs/specs/metrics-audit/privacy-review-checklist.md @@ -171,8 +171,11 @@ A new field **must** be hashed if any of the following are true: A new field should **not** be hashed if: -- The value is from a fixed enum (e.g., `auth.method` = `"browser"`, or - `aspire.apphost.language` = `"typescript"` / `"python"` / `"go"` / `"java"` / `"rust"`). +- The value is from a fixed enum (e.g., `auth.method` = `"browser"`, + `aspire.apphost.language` = `"typescript"` / `"python"` / `"go"` / `"java"` / `"rust"`, + `auth.cache_clear_failed` = `"auth"` / `"subscriptions"`, or + `skip.reason` = `"cluster_not_provisioned"`), or a boolean + (e.g., `container.publish.remotebuild`). - The value is a count or duration (measurements). - The value is system-generated metadata (e.g., OS type). - The value is a hardcoded literal in source code (e.g., `exegraph.step.tags`, which @@ -198,6 +201,7 @@ When adding a new telemetry field: - OTel key name - Classification - Purpose + - EndpointIdType (only when the value is a known endpoint identifier; otherwise `N/A`) - Whether it is hashed - Whether it is a measurement - Allowed values (if enum) @@ -222,6 +226,8 @@ Copy this checklist into your PR description when making telemetry changes. ### New Events - [ ] Event constant defined in `events/events.go` +- [ ] Event constant is an exported string `const` whose Go identifier contains `Event` (end it with + `Prefix` for a prefix-match group) so the GDPR `--events` scan discovers it - [ ] Event documented in `docs/specs/metrics-audit/telemetry-schema.md` - [ ] Event follows naming convention (`prefix.noun.verb`) diff --git a/docs/specs/metrics-audit/telemetry-schema.md b/docs/specs/metrics-audit/telemetry-schema.md index f050477953c..9146d22b169 100644 --- a/docs/specs/metrics-audit/telemetry-schema.md +++ b/docs/specs/metrics-audit/telemetry-schema.md @@ -190,6 +190,13 @@ not emitted by azd spans. | Field | OTel Key | Classification | Purpose | Notes | |-------|----------|----------------|---------|-------| | Remote build count | `container.remoteBuild.count` | SystemMetadata | FeatureInsight | **Measurement** | +| Publish remote build | `container.publish.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. Emitted on `container.publish`. Distinct from the `container.remotebuild` event. | + +### AKS + +| Field | OTel Key | Classification | Purpose | Notes | +|-------|----------|----------------|---------|-------| +| Skip reason | `skip.reason` | SystemMetadata | FeatureInsight | Bounded enum (`cluster_not_provisioned`); never raw error text. Emitted on `aks.postprovision.skip`. | ### JSON-RPC @@ -317,6 +324,7 @@ The following fields are defined in `fields.go`. | Field | OTel Key | Classification | Purpose | Values | |-------|----------|----------------|---------|--------| | Auth method | `auth.method` | SystemMetadata | FeatureInsight | `browser`, `device-code`, `service-principal-secret`, `service-principal-certificate`, `federated-github`, `federated-azure-pipelines`, `federated-oidc`, `managed-identity`, `external`, `oneauth`, `check-status` | +| Auth cache-clear failed | `auth.cache_clear_failed` | SystemMetadata | PerformanceAndHealth | Fixed enum (`auth`, `subscriptions`) identifying which cache failed to clear during the pre-login cleanup. Emitted on the `auth login` usage event. | | Env count | `env.count` | SystemMetadata | FeatureInsight | **Measurement** — number of environments | | Hooks name | `hooks.name` | SystemMetadata | FeatureInsight | Built-in hook name (raw) or SHA-256 hash for extension/custom hooks. Known values: `prebuild`, `postbuild`, `predeploy`, `postdeploy`, `predown`, `postdown`, `prepackage`, `postpackage`, `preprovision`, `postprovision`, `prepublish`, `postpublish`, `prerestore`, `postrestore`, `preup`, `postup` | | Hooks type | `hooks.type` | SystemMetadata | FeatureInsight | `project`, `service`, `layer` | @@ -420,7 +428,7 @@ Telemetry for the `infra.layers[]` parallel provisioning feature, emitted from ` ## Data Classifications -Classifications are defined in `internal/telemetry/fields/fields.go` and control how data +Classifications are defined in `cli/azd/internal/tracing/fields/fields.go` and control how data is stored, retained, and who may access it. | Classification | Description | @@ -483,3 +491,44 @@ Fields that are hashed: 3. **Queue**: Envelopes are written to disk under `~/.azd/telemetry/`. 4. **Upload**: The `azd telemetry upload` command (run as a background process) reads the queue and sends data to Azure Monitor. 5. **Analysis**: Data flows into Kusto tables for dashboarding and analysis via LENS jobs and cooked tables. + +## GDPR Data-Catalog Classification + +Runtime emission (above) is separate from **classification**. The GDPR data catalog is kept in +sync by an external tool (in the `azd-queries` repo, run on a schedule pipeline against the `main` branch). A raw `attribute.String("my.key", v)` at a call site is not in scanned location, so its catalog row stays **Unclassified / `Complete=false`**. This is enforced in-repo by `TestNoRawTelemetryAttributes` (`cli/azd/cmd/telemetry_test.go`). + +### Field (attribute) discovery contract + +For a field to be discovered and classified it must be: + +- an **exported**, **package-level** `var` (not a `const`, not function-local, not unexported), and +- typed exactly **`AttributeKey`** (the struct declared in `fields.go`). + +The scanner reads these `AttributeKey` members and maps each into the catalog row: + +| `AttributeKey` member | Catalog field | Notes | +|-----------------------|---------------|-------| +| `Key` | `PropertyName` | The dotted OTel key. May be a string literal, `attribute.Key("…")`, or a string const. If it cannot be resolved, the trailing `// …` line comment on the `var` is used as a fallback name. | +| `Classification` | `DataClassification` | One of the six [Data Classifications](#data-classifications). Any other value silently defaults to `SystemMetadata`. | +| `Purpose` | `BusinessJustification` | The field's [Purpose](#purposes): `FeatureInsight`→`FeatureUsage`, `BusinessInsight`→`BI`, `PerformanceAndHealth`→`PerformanceAndHealth`. Any other value silently defaults to `FeatureUsage`. | +| `Endpoint` | `EndpointIdType` | Optional identifier-type tag (e.g. `MacAddressHash`, `SQMUserId`, `AzureSubscriptionId`). Defaults to `N/A` when unset. | +| `IsMeasurement` | `PropertyPath` | `true` → `$.Measurements`; `false` (default) → `$.Properties`. | + +`Classification` and `Purpose` must be written as **bare identifiers from the `fields` package** +(e.g. `Classification: SystemMetadata`) — the scanner reads the identifier name, so a qualified +`fields.SystemMetadata` reference from another package would not be recognized. Keep all +`AttributeKey` definitions inside the `fields` package. Fields are registered as **common +properties** that apply across events (the scan does not tie an attribute to a specific event). + +### Event discovery contract + +For an event to be discovered its constant must be: + +- an **exported** `const` with a **string** value in the `events` package, and +- named with a Go identifier that **contains the substring `Event`** (e.g. `PackBuildEvent`). A + constant whose identifier omits `Event` is silently skipped even if it is emitted at runtime. + +An identifier that **ends with `Prefix`** (e.g. `CommandEventPrefix`) registers a **prefix group**: +any emitted event name starting with that prefix is classified under it. Every other event constant +registers an exact event name. (Note: this `Event`/`Prefix` rule is about the Go **identifier**; the +string **value** still follows the `prefix.noun.verb` naming convention.) From 9f11be9da204e43f8b6f30c1e8ccd69531813c5a Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 14 Aug 2026 22:23:53 -0700 Subject: [PATCH 03/18] improve docs --- cli/azd/AGENTS.md | 8 ++++---- docs/guides/feature-telemetry.md | 6 +++--- .../metrics-audit/privacy-review-checklist.md | 4 ++-- docs/specs/metrics-audit/telemetry-schema.md | 18 +++++++++--------- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/cli/azd/AGENTS.md b/cli/azd/AGENTS.md index 4576cba04de..b601e8e0688 100644 --- a/cli/azd/AGENTS.md +++ b/cli/azd/AGENTS.md @@ -285,12 +285,12 @@ public reference, and downstream Kusto/LENS consumers drift out of sync. Verify host-domain table). Every field MUST set a `Classification` (e.g. `SystemMetadata`, `OrganizationalIdentifiableInformation`, `EndUserPseudonymizedInformation`; never emit `CustomerContent`) and a `Purpose` (`FeatureInsight` / `BusinessInsight` / - `PerformanceAndHealth`); the `--fields` scan also reads the optional `Endpoint` (→ `EndpointIdType`) - and `IsMeasurement` (→ `$.Measurements` vs `$.Properties`) members. + `PerformanceAndHealth`); the classifier also reads the optional `Endpoint` and `IsMeasurement` + members. - **Event** — define a constant in `cli/azd/internal/tracing/events/events.go` following the `prefix.noun.verb` value convention. It must be an exported string `const` whose Go identifier - contains `Event` (end it with `Prefix` for a prefix-match group) so the classifier's `--events` - scan discovers it. + contains `Event` (end it with `Prefix` for a prefix-match group) so the classifier + discovers it. - **Emit** at the call site via `tracing.Start` (spans/events) plus `tracing.SetUsageAttributes` or `span.SetAttributes` (attributes). Always pass a `fields.AttributeKey` method (e.g. `fields.MyKey.String(v)` / `.Bool(v)` / `.Int(v)`) — never a raw diff --git a/docs/guides/feature-telemetry.md b/docs/guides/feature-telemetry.md index f528c4337db..b76993667bf 100644 --- a/docs/guides/feature-telemetry.md +++ b/docs/guides/feature-telemetry.md @@ -63,9 +63,9 @@ Every field must have: 1. **A key name** — descriptive, dot-separated, lowercase 2. **A classification** — what kind of data is this (see [Data Classifications](#data-classifications)) 3. **A purpose** — why are we collecting it (see [Purposes](#purposes)) -4. **`IsMeasurement: true`** if the value is numeric (routes to `$.Measurements`, not `$.Properties`) -5. **`Endpoint`** (optional) — an identifier-type tag (e.g. `AzureSubscriptionId`) when the value is a - known endpoint identifier; leave unset otherwise (defaults to `N/A`) +4. **`IsMeasurement: true`** if the value is numeric (goes to the `Measurements` column, not `Properties`) +5. **`Endpoint`** (optional) — an identifier-type tag to set when the value is a known endpoint + identifier; leave unset otherwise ```go // In fields.go — add your field keys diff --git a/docs/specs/metrics-audit/privacy-review-checklist.md b/docs/specs/metrics-audit/privacy-review-checklist.md index 4a59ec531c3..e8dea86da43 100644 --- a/docs/specs/metrics-audit/privacy-review-checklist.md +++ b/docs/specs/metrics-audit/privacy-review-checklist.md @@ -201,7 +201,7 @@ When adding a new telemetry field: - OTel key name - Classification - Purpose - - EndpointIdType (only when the value is a known endpoint identifier; otherwise `N/A`) + - EndpointIdType (only when the value is a known endpoint identifier) - Whether it is hashed - Whether it is a measurement - Allowed values (if enum) @@ -227,7 +227,7 @@ Copy this checklist into your PR description when making telemetry changes. ### New Events - [ ] Event constant defined in `events/events.go` - [ ] Event constant is an exported string `const` whose Go identifier contains `Event` (end it with - `Prefix` for a prefix-match group) so the GDPR `--events` scan discovers it + `Prefix` for a prefix-match group) so the GDPR classifier discovers it - [ ] Event documented in `docs/specs/metrics-audit/telemetry-schema.md` - [ ] Event follows naming convention (`prefix.noun.verb`) diff --git a/docs/specs/metrics-audit/telemetry-schema.md b/docs/specs/metrics-audit/telemetry-schema.md index 9146d22b169..ae8393f390f 100644 --- a/docs/specs/metrics-audit/telemetry-schema.md +++ b/docs/specs/metrics-audit/telemetry-schema.md @@ -495,7 +495,7 @@ Fields that are hashed: ## GDPR Data-Catalog Classification Runtime emission (above) is separate from **classification**. The GDPR data catalog is kept in -sync by an external tool (in the `azd-queries` repo, run on a schedule pipeline against the `main` branch). A raw `attribute.String("my.key", v)` at a call site is not in scanned location, so its catalog row stays **Unclassified / `Complete=false`**. This is enforced in-repo by `TestNoRawTelemetryAttributes` (`cli/azd/cmd/telemetry_test.go`). +sync by an external metadata tool that **statically scans the telemetry source** — it does not read this document or observe live telemetry, so it can only classify a property that is declared where the scan looks: an exported `fields.AttributeKey`. A raw `attribute.String("my.key", v)` at a call site is invisible to the scan, so its catalog row stays **Unclassified / `Complete=false`**. This is enforced in-repo by `TestNoRawTelemetryAttributes` (`cli/azd/cmd/telemetry_test.go`). ### Field (attribute) discovery contract @@ -504,15 +504,15 @@ For a field to be discovered and classified it must be: - an **exported**, **package-level** `var` (not a `const`, not function-local, not unexported), and - typed exactly **`AttributeKey`** (the struct declared in `fields.go`). -The scanner reads these `AttributeKey` members and maps each into the catalog row: +The scanner reads these `AttributeKey` members: -| `AttributeKey` member | Catalog field | Notes | -|-----------------------|---------------|-------| -| `Key` | `PropertyName` | The dotted OTel key. May be a string literal, `attribute.Key("…")`, or a string const. If it cannot be resolved, the trailing `// …` line comment on the `var` is used as a fallback name. | -| `Classification` | `DataClassification` | One of the six [Data Classifications](#data-classifications). Any other value silently defaults to `SystemMetadata`. | -| `Purpose` | `BusinessJustification` | The field's [Purpose](#purposes): `FeatureInsight`→`FeatureUsage`, `BusinessInsight`→`BI`, `PerformanceAndHealth`→`PerformanceAndHealth`. Any other value silently defaults to `FeatureUsage`. | -| `Endpoint` | `EndpointIdType` | Optional identifier-type tag (e.g. `MacAddressHash`, `SQMUserId`, `AzureSubscriptionId`). Defaults to `N/A` when unset. | -| `IsMeasurement` | `PropertyPath` | `true` → `$.Measurements`; `false` (default) → `$.Properties`. | +| `AttributeKey` member | What the classifier reads | +|-----------------------|---------------------------| +| `Key` | The dotted OTel key (a string literal, `attribute.Key("…")`, or a string const). | +| `Classification` | One of the six [Data Classifications](#data-classifications). | +| `Purpose` | One or more [Purposes](#purposes). | +| `Endpoint` | Optional identifier-type tag; set only when the value is a known endpoint identifier. | +| `IsMeasurement` | `true` for numeric values (routed to the Measurements column); `false` (default) for Properties. | `Classification` and `Purpose` must be written as **bare identifiers from the `fields` package** (e.g. `Classification: SystemMetadata`) — the scanner reads the identifier name, so a qualified From 1c315f7381393c36bc5c12691e9eb4892c414a1d Mon Sep 17 00:00:00 2001 From: hemarina Date: Fri, 14 Aug 2026 23:34:32 -0700 Subject: [PATCH 04/18] address comments --- cli/azd/cmd/telemetry_test.go | 66 +++++++++++++++++------ cli/azd/internal/tracing/fields/fields.go | 8 ++- cli/azd/pkg/project/container_helper.go | 2 +- 3 files changed, 53 insertions(+), 23 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index cf8593ee117..80cf49b9b23 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -10,6 +10,7 @@ import ( "go/token" "os" "path/filepath" + "strconv" "strings" "testing" @@ -275,8 +276,8 @@ func TestTelemetryFieldConstants(t *testing.T) { // Container publish telemetry fields t.Run("ContainerFields", func(t *testing.T) { t.Parallel() - kv := fields.ContainerPublishRemoteBuildKey.Bool(true) - require.Equal(t, "container.publish.remotebuild", string(kv.Key)) + kv := fields.ContainerRemoteBuildKey.Bool(true) + require.Equal(t, "container.remotebuild", string(kv.Key)) require.Equal(t, true, kv.Value.AsBool()) }) @@ -290,11 +291,13 @@ func TestTelemetryFieldConstants(t *testing.T) { } // TestNoRawTelemetryAttributes enforces that product code never emits telemetry -// via raw attribute.String("literal", ...) / attribute.Bool("literal", ...) etc. -// Every telemetry attribute must be declared as a fields.AttributeKey (with a -// Classification and Purpose) and emitted through it, e.g. -// fields.SomeKey.String(value). This keeps the telemetry schema discoverable and -// classifiable for the GDPR metadata pipeline (azure-dev issue #1803). +// via raw attribute.String(key, ...) / attribute.Bool(key, ...) etc. — whether +// the key is a string literal or a named constant, and whether the package is +// imported under its default name or an alias. Every telemetry attribute must be +// declared as a fields.AttributeKey (with a Classification and Purpose) and +// emitted through it, e.g. fields.SomeKey.String(value). This keeps the telemetry +// schema discoverable and classifiable for the GDPR metadata pipeline (azure-dev +// issue #1803). // // Legitimately excluded from the scan: // - *_test.go files (test fixtures build raw attributes on purpose). @@ -305,14 +308,15 @@ func TestNoRawTelemetryAttributes(t *testing.T) { t.Parallel() // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers - // that build a KeyValue from a string-literal key. Using them directly in - // product code bypasses the fields.AttributeKey registry, so the GDPR metadata + // that build a KeyValue from a key and value. Using them directly in product + // code bypasses the fields.AttributeKey registry, so the GDPR metadata // exporter (which discovers only exported AttributeKey vars) can never classify // the resulting property. See docs/specs/metrics-audit/telemetry-schema.md. rawAttributeConstructors := map[string]struct{}{ "String": {}, "Bool": {}, "Int": {}, + "IntSlice": {}, "Int64": {}, "Float64": {}, "Stringer": {}, @@ -364,6 +368,31 @@ func TestNoRawTelemetryAttributes(t *testing.T) { return nil // skip unparseable files } + // Resolve the local name bound to go.opentelemetry.io/otel/attribute in + // this file. Matching the selector base literally against "attribute" + // would miss an aliased import (e.g. otelattr "...otel/attribute") and + // could also misfire on an unrelated local identifier named "attribute". + // If the file does not import the package, it cannot construct a raw + // attribute, so there is nothing to scan. + attrPkgName := "" + for _, imp := range file.Imports { + importPath, uErr := strconv.Unquote(imp.Path.Value) + if uErr != nil || importPath != "go.opentelemetry.io/otel/attribute" { + continue + } + if imp.Name != nil { + attrPkgName = imp.Name.Name // explicit alias + } else { + attrPkgName = "attribute" // default package name + } + break + } + // A blank ("_") or dot (".") import cannot produce a "pkg.Constructor" + // selector, so there is nothing this AST check can match on. + if attrPkgName == "" || attrPkgName == "_" || attrPkgName == "." { + return nil + } + ast.Inspect(file, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) if !ok || len(call.Args) == 0 { @@ -376,7 +405,7 @@ func TestNoRawTelemetryAttributes(t *testing.T) { } pkgIdent, ok := sel.X.(*ast.Ident) - if !ok || pkgIdent.Name != "attribute" { + if !ok || pkgIdent.Name != attrPkgName { return true } @@ -384,16 +413,19 @@ func TestNoRawTelemetryAttributes(t *testing.T) { return true } - // Only flag string-literal keys. A non-literal key (e.g. a dynamic - // extension field) is a separate, intentional pattern. - lit, ok := call.Args[0].(*ast.BasicLit) - if !ok || lit.Kind != token.STRING { - return true + // Every direct attribute constructor call in product code bypasses + // the fields.AttributeKey registry, so the GDPR classifier can never + // see it — whether the key is a string literal or a named constant. + // Both are violations; surface the literal key when present for a + // friendlier message. + keyDesc := "..." + if lit, ok := call.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { + keyDesc = lit.Value } pos := fset.Position(call.Pos()) violations = append(violations, fmt.Sprintf( - " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, sel.Sel.Name, lit.Value)) + " %s:%d: %s.%s(%s, ...)", rel, pos.Line, attrPkgName, sel.Sel.Name, keyDesc)) return true }) @@ -403,7 +435,7 @@ func TestNoRawTelemetryAttributes(t *testing.T) { if len(violations) > 0 { t.Errorf( - "Found %d raw telemetry attribute(s) using a string-literal key.\n"+ + "Found %d raw telemetry attribute(s) constructed directly.\n"+ "Declare an exported fields.AttributeKey (with Classification and Purpose) in\n"+ "internal/tracing/fields/fields.go and emit via it, e.g. fields.MyKey.String(v),\n"+ "so the property is discoverable and classifiable by the GDPR metadata pipeline.\n\n"+ diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index c416fb11dc0..52c38d769d0 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -1154,13 +1154,11 @@ var ( IsMeasurement: true, } - // ContainerPublishRemoteBuildKey records whether the container image was + // ContainerRemoteBuildKey records whether the container image was // built remotely (ACR build) rather than locally for a container publish. // It is a boolean (fixed cardinality), so it is emitted raw (not hashed). - // Emitted on the `container.publish` event. This is distinct from the - // `container.remotebuild` event, which marks the Azure-side remote build. - ContainerPublishRemoteBuildKey = AttributeKey{ - Key: attribute.Key("container.publish.remotebuild"), + ContainerRemoteBuildKey = AttributeKey{ + Key: attribute.Key("container.remotebuild"), Classification: SystemMetadata, Purpose: FeatureInsight, } diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 5f43ae970ae..3006121f9aa 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -624,7 +624,7 @@ func (ch *ContainerHelper) Publish( ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) defer func() { span.EndWithStatus(err) }() span.SetAttributes( - fields.ContainerPublishRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild), + fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild), ) var remoteImage string From df3e2a714d6bdc7e6d89a0456e30cd96ff856bba Mon Sep 17 00:00:00 2001 From: hemarina Date: Sat, 15 Aug 2026 00:12:58 -0700 Subject: [PATCH 05/18] address comment --- cli/azd/cmd/telemetry_test.go | 8 -------- docs/reference/telemetry-data.md | 2 +- docs/specs/metrics-audit/feature-telemetry-matrix.md | 2 +- docs/specs/metrics-audit/privacy-review-checklist.md | 2 +- docs/specs/metrics-audit/telemetry-schema.md | 2 +- 5 files changed, 4 insertions(+), 12 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 80cf49b9b23..f3106c20885 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -301,8 +301,6 @@ func TestTelemetryFieldConstants(t *testing.T) { // // Legitimately excluded from the scan: // - *_test.go files (test fixtures build raw attributes on purpose). -// - internal/tracing/... — the tracing/baggage plumbing that the -// fields.AttributeKey abstraction is itself built on top of. // - extensions/... — independent extension modules with their own schema. func TestNoRawTelemetryAttributes(t *testing.T) { t.Parallel() @@ -356,12 +354,6 @@ func TestNoRawTelemetryAttributes(t *testing.T) { } rel = filepath.ToSlash(rel) - // The tracing/baggage plumbing is the sanctioned home for raw attribute - // construction; the fields.AttributeKey abstraction is built on it. - if strings.HasPrefix(rel, "internal/tracing/") { - return nil - } - fset := token.NewFileSet() file, parseErr := parser.ParseFile(fset, path, nil, 0) if parseErr != nil { diff --git a/docs/reference/telemetry-data.md b/docs/reference/telemetry-data.md index aa17d7aeaad..04d23943dad 100644 --- a/docs/reference/telemetry-data.md +++ b/docs/reference/telemetry-data.md @@ -427,7 +427,7 @@ Emitted at provision start by the `microsoft.foundry` provisioning provider (the | Field Key | Type | Description | |-----------|------|-------------| | `container.remoteBuild.count` | measurement | Number of remote container builds performed | -| `container.publish.remotebuild` | bool | Whether the image was built remotely (ACR) rather than locally. Emitted on `container.publish`. Distinct from the `container.remotebuild` event. | +| `container.remotebuild` | bool | Whether the image was built remotely (ACR) rather than locally. |
diff --git a/docs/specs/metrics-audit/feature-telemetry-matrix.md b/docs/specs/metrics-audit/feature-telemetry-matrix.md index 8c2f48910e8..6538fbbba13 100644 --- a/docs/specs/metrics-audit/feature-telemetry-matrix.md +++ b/docs/specs/metrics-audit/feature-telemetry-matrix.md @@ -161,7 +161,7 @@ reserved field contracts. | **ARM deployment client** | `provision` (any Bicep flow) | `arm.deploy.subscription`, `arm.deploy.resourcegroup`, `arm.stack.deploy.subscription`, `arm.stack.deploy.resourcegroup`, `arm.whatif.subscription`, `arm.whatif.resourcegroup`, `arm.validate.subscription`, `arm.validate.resourcegroup` | ARM operation status + duration | Per-call instrumentation in the ARM client; covers regular + stack deployments at both scopes | | **Multi-layer provision** | `provision` (when `infra.layers[]` is configured in `azure.yaml`) | (none — enriches the `provision` span) | `provision.layer.count`, `provision.layer.max_parallel`, `provision.layer.safe_fallback_count`, `provision.layer.explicit_dependson_count` | All four are integer measurements emitted from `internal/cmd/provision_graph.go`; no per-layer duration or outcome attribute is emitted | | **Execution graph (scheduler)** | `up`, `provision`, `deploy`, `package`, `publish`, `down` | `exegraph.run`, `exegraph.step` | `exegraph.step.count`, `exegraph.max_concurrency`, `exegraph.error_policy`, `exegraph.step.name` (hashed), `exegraph.step.deps` (hashed slice), `exegraph.step.tags` (raw — hardcoded literals only), `exegraph.step.timeout_s` | Step names embed user-defined service / layer names from `azure.yaml`; both `name` and `deps` use `fields.StringHashed` / `fields.StringSliceHashed` | -| **Container lifecycle** | `package`, `deploy` (container service targets) | `container.credentials`, `container.publish`, `container.remotebuild` | `container.publish` sets `container.publish.remotebuild` (bool) only; `container.credentials` and `container.remotebuild` set no attributes (span status carries success/failure and duration) | The hashed `pack.builder.image` / `pack.builder.tag` attributes are emitted on the separate `tools.pack.build` span, not the `container.*` spans | +| **Container lifecycle** | `package`, `deploy` (container service targets) | `container.credentials`, `container.publish`, `container.remotebuild` | `container.publish` sets a `container.remotebuild` property (bool) only; the `container.credentials` and `container.remotebuild` events set no attributes (span status carries success/failure and duration) | The hashed `pack.builder.image` / `pack.builder.tag` attributes are emitted on the separate `tools.pack.build` span, not the `container.*` spans | | **App Service deploy** | `deploy`, `publish` (App Service targets) | `deploy.appservice.zip` | `deploy.appservice.linux` (bool), `deploy.appservice.attempt` (retry attempt number) | Zip-deploy path only; outcome / duration are carried by the span status and span timing, not by dedicated attributes | | **AKS service target** | `provision` (AKS preprovision/postprovision) | `aks.postprovision.skip` | `skip.reason` (bounded enum — `cluster_not_provisioned`) | Recorded when cluster is not yet available for context setup | | **Agent troubleshoot middleware** | Triggered on command failure when troubleshooting is engaged | `agent.troubleshoot` | Error chain attributes, hashed error fields | Emitted from `cmd/middleware/error.go` | diff --git a/docs/specs/metrics-audit/privacy-review-checklist.md b/docs/specs/metrics-audit/privacy-review-checklist.md index e8dea86da43..ed6d264b858 100644 --- a/docs/specs/metrics-audit/privacy-review-checklist.md +++ b/docs/specs/metrics-audit/privacy-review-checklist.md @@ -175,7 +175,7 @@ A new field should **not** be hashed if: `aspire.apphost.language` = `"typescript"` / `"python"` / `"go"` / `"java"` / `"rust"`, `auth.cache_clear_failed` = `"auth"` / `"subscriptions"`, or `skip.reason` = `"cluster_not_provisioned"`), or a boolean - (e.g., `container.publish.remotebuild`). + (e.g., `container.remotebuild`). - The value is a count or duration (measurements). - The value is system-generated metadata (e.g., OS type). - The value is a hardcoded literal in source code (e.g., `exegraph.step.tags`, which diff --git a/docs/specs/metrics-audit/telemetry-schema.md b/docs/specs/metrics-audit/telemetry-schema.md index ae8393f390f..01fb8e608e5 100644 --- a/docs/specs/metrics-audit/telemetry-schema.md +++ b/docs/specs/metrics-audit/telemetry-schema.md @@ -190,7 +190,7 @@ not emitted by azd spans. | Field | OTel Key | Classification | Purpose | Notes | |-------|----------|----------------|---------|-------| | Remote build count | `container.remoteBuild.count` | SystemMetadata | FeatureInsight | **Measurement** | -| Publish remote build | `container.publish.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. Emitted on `container.publish`. Distinct from the `container.remotebuild` event. | +| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. | ### AKS From 36d6381db876c4ec275055571d724ca4b7b582f7 Mon Sep 17 00:00:00 2001 From: hemarina Date: Sat, 15 Aug 2026 00:26:22 -0700 Subject: [PATCH 06/18] another round of review --- cli/azd/cmd/telemetry_test.go | 275 +++++++++++++------ docs/specs/metrics-audit/telemetry-schema.md | 6 +- 2 files changed, 194 insertions(+), 87 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index f3106c20885..2e2904a6ab1 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -291,9 +291,10 @@ func TestTelemetryFieldConstants(t *testing.T) { } // TestNoRawTelemetryAttributes enforces that product code never emits telemetry -// via raw attribute.String(key, ...) / attribute.Bool(key, ...) etc. — whether -// the key is a string literal or a named constant, and whether the package is -// imported under its default name or an alias. Every telemetry attribute must be +// via raw attribute.String(key, ...) / attribute.Bool(key, ...) — or the chained +// attribute.Key(key).String(...) form — whether the key is a string literal or a +// named constant, and whether the package is imported under its default name or +// an alias. Every telemetry attribute must be // declared as a fields.AttributeKey (with a Classification and Purpose) and // emitted through it, e.g. fields.SomeKey.String(value). This keeps the telemetry // schema discoverable and classifiable for the GDPR metadata pipeline (azure-dev @@ -305,25 +306,6 @@ func TestTelemetryFieldConstants(t *testing.T) { func TestNoRawTelemetryAttributes(t *testing.T) { t.Parallel() - // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers - // that build a KeyValue from a key and value. Using them directly in product - // code bypasses the fields.AttributeKey registry, so the GDPR metadata - // exporter (which discovers only exported AttributeKey vars) can never classify - // the resulting property. See docs/specs/metrics-audit/telemetry-schema.md. - rawAttributeConstructors := map[string]struct{}{ - "String": {}, - "Bool": {}, - "Int": {}, - "IntSlice": {}, - "Int64": {}, - "Float64": {}, - "Stringer": {}, - "StringSlice": {}, - "BoolSlice": {}, - "Int64Slice": {}, - "Float64Slice": {}, - } - // The test runs with its package directory (cli/azd/cmd) as the working // directory, so the module root is one level up. azdRoot, err := filepath.Abs("..") @@ -360,67 +342,7 @@ func TestNoRawTelemetryAttributes(t *testing.T) { return nil // skip unparseable files } - // Resolve the local name bound to go.opentelemetry.io/otel/attribute in - // this file. Matching the selector base literally against "attribute" - // would miss an aliased import (e.g. otelattr "...otel/attribute") and - // could also misfire on an unrelated local identifier named "attribute". - // If the file does not import the package, it cannot construct a raw - // attribute, so there is nothing to scan. - attrPkgName := "" - for _, imp := range file.Imports { - importPath, uErr := strconv.Unquote(imp.Path.Value) - if uErr != nil || importPath != "go.opentelemetry.io/otel/attribute" { - continue - } - if imp.Name != nil { - attrPkgName = imp.Name.Name // explicit alias - } else { - attrPkgName = "attribute" // default package name - } - break - } - // A blank ("_") or dot (".") import cannot produce a "pkg.Constructor" - // selector, so there is nothing this AST check can match on. - if attrPkgName == "" || attrPkgName == "_" || attrPkgName == "." { - return nil - } - - ast.Inspect(file, func(n ast.Node) bool { - call, ok := n.(*ast.CallExpr) - if !ok || len(call.Args) == 0 { - return true - } - - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return true - } - - pkgIdent, ok := sel.X.(*ast.Ident) - if !ok || pkgIdent.Name != attrPkgName { - return true - } - - if _, isConstructor := rawAttributeConstructors[sel.Sel.Name]; !isConstructor { - return true - } - - // Every direct attribute constructor call in product code bypasses - // the fields.AttributeKey registry, so the GDPR classifier can never - // see it — whether the key is a string literal or a named constant. - // Both are violations; surface the literal key when present for a - // friendlier message. - keyDesc := "..." - if lit, ok := call.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { - keyDesc = lit.Value - } - - pos := fset.Position(call.Pos()) - violations = append(violations, fmt.Sprintf( - " %s:%d: %s.%s(%s, ...)", rel, pos.Line, attrPkgName, sel.Sel.Name, keyDesc)) - return true - }) - + violations = append(violations, scanGoFileForRawAttributes(fset, file, rel)...) return nil }) require.NoError(t, err) @@ -438,7 +360,192 @@ func TestNoRawTelemetryAttributes(t *testing.T) { } } -// TestCommandTelemetryCoverage ensures every user-facing command is explicitly categorized +// scanGoFileForRawAttributes returns the raw-telemetry-attribute violations in a +// single parsed Go file. rel is the display path used in messages. It is shared +// by TestNoRawTelemetryAttributes (which walks the module tree) and the fixture +// test TestRawTelemetryAttributeScanner, so the guard's contract is itself tested. +func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) []string { + // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers + // (and the identically named attribute.Key methods) that build a KeyValue from + // a key and value. Using them directly in product code bypasses the + // fields.AttributeKey registry, so the GDPR metadata exporter (which discovers + // only exported AttributeKey vars) can never classify the resulting property. + // See docs/specs/metrics-audit/telemetry-schema.md. + rawAttributeConstructors := map[string]struct{}{ + "String": {}, + "Bool": {}, + "Int": {}, + "IntSlice": {}, + "Int64": {}, + "Float64": {}, + "Stringer": {}, + "StringSlice": {}, + "BoolSlice": {}, + "Int64Slice": {}, + "Float64Slice": {}, + } + + // Resolve the local name bound to go.opentelemetry.io/otel/attribute in this + // file. Matching the selector base literally against "attribute" would miss an + // aliased import (e.g. otelattr "...otel/attribute") and could also misfire on + // an unrelated local identifier named "attribute". If the file does not import + // the package, it cannot construct a raw attribute, so there is nothing to scan. + attrPkgName := "" + for _, imp := range file.Imports { + importPath, uErr := strconv.Unquote(imp.Path.Value) + if uErr != nil || importPath != "go.opentelemetry.io/otel/attribute" { + continue + } + if imp.Name != nil { + attrPkgName = imp.Name.Name // explicit alias + } else { + attrPkgName = "attribute" // default package name + } + break + } + // A blank ("_") or dot (".") import cannot produce a "pkg.Constructor" + // selector, so there is nothing this AST check can match on. + if attrPkgName == "" || attrPkgName == "_" || attrPkgName == "." { + return nil + } + + // literalKey renders the first string-literal argument of a call for a + // friendlier message, or "..." for a non-literal (e.g. const) key. + literalKey := func(c *ast.CallExpr) string { + if len(c.Args) == 0 { + return "..." + } + if lit, ok := c.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { + return lit.Value + } + return "..." + } + + var violations []string + ast.Inspect(file, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return true + } + + // Only the KeyValue-producing constructor / key-method names are of + // interest (String, Bool, Int, ... — see rawAttributeConstructors). + if _, isConstructor := rawAttributeConstructors[sel.Sel.Name]; !isConstructor { + return true + } + + pos := fset.Position(call.Pos()) + + // Form A: attribute.String("key", v) / attribute.Bool(k, v) — the selector + // base is the imported package identifier. This bypasses the + // fields.AttributeKey registry, so the GDPR classifier can never see it, + // whether the key is a string literal or a named constant. + if base, ok := sel.X.(*ast.Ident); ok && base.Name == attrPkgName { + violations = append(violations, fmt.Sprintf( + " %s:%d: %s.%s(%s, ...)", rel, pos.Line, attrPkgName, sel.Sel.Name, literalKey(call))) + return true + } + + // Form B: attribute.Key("key").String(v) — the selector base is an inline + // attribute.Key(...) constructor call. This produces a KeyValue that + // likewise bypasses the fields.AttributeKey registry. (A method call on a + // Key-typed *variable* is indistinguishable at the AST level from the + // sanctioned fields.SomeKey.String(v) promoted-method call, so only the + // inline form is enforceable here.) + if inner, ok := sel.X.(*ast.CallExpr); ok { + if innerSel, ok := inner.Fun.(*ast.SelectorExpr); ok { + if innerBase, ok := innerSel.X.(*ast.Ident); ok && + innerBase.Name == attrPkgName && innerSel.Sel.Name == "Key" { + violations = append(violations, fmt.Sprintf( + " %s:%d: %s.Key(%s).%s(...)", rel, pos.Line, attrPkgName, literalKey(inner), sel.Sel.Name)) + return true + } + } + } + + return true + }) + + return violations +} + +// TestRawTelemetryAttributeScanner is a fixture test for the AST guard used by +// TestNoRawTelemetryAttributes. It pins the contract: raw attribute constructors, +// aliased imports, constant keys, and the chained attribute.Key(k).String(v) form +// are all flagged, while the sanctioned promoted-method pattern and a bare +// attribute.Key(k) (which does not build a KeyValue) are not. +func TestRawTelemetryAttributeScanner(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + src string + wantViolation bool + }{ + { + name: "literal key", + src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.String("raw.key", "v")`, + wantViolation: true, + }, + { + name: "constant key", + src: `package p; import "go.opentelemetry.io/otel/attribute"; const k = "raw.key"; var _ = attribute.String(k, "v")`, + wantViolation: true, + }, + { + name: "aliased import", + src: `package p; import otelattr "go.opentelemetry.io/otel/attribute"; var _ = otelattr.Bool("raw.key", true)`, + wantViolation: true, + }, + { + name: "int slice constructor", + src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.IntSlice("raw.key", []int{1})`, + wantViolation: true, + }, + { + name: "chained key method", + src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.Key("raw.key").String("v")`, + wantViolation: true, + }, + { + name: "bare key builder without value", + src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.Key("raw.key")`, + wantViolation: false, + }, + { + name: "promoted method on key-typed value", + src: `package p; import "go.opentelemetry.io/otel/attribute"; func f(k attribute.Key) { _ = k.String("v") }`, + wantViolation: false, + }, + { + name: "file without the attribute import", + src: `package p; var _ = 1`, + wantViolation: false, + }, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, tc.name+".go", tc.src, 0) + require.NoError(t, err) + + got := scanGoFileForRawAttributes(fset, file, tc.name+".go") + if tc.wantViolation { + require.NotEmpty(t, got, "expected a violation for %q", tc.name) + } else { + require.Empty(t, got, "expected no violation for %q, got %v", tc.name, got) + } + }) + } +} // for telemetry coverage. When a new command is added to the CLI, it must be added to one // of the lists below. This forces developers to consciously decide whether the command needs // command-specific telemetry attributes or whether global middleware telemetry is sufficient. diff --git a/docs/specs/metrics-audit/telemetry-schema.md b/docs/specs/metrics-audit/telemetry-schema.md index 01fb8e608e5..8f63f01d3d6 100644 --- a/docs/specs/metrics-audit/telemetry-schema.md +++ b/docs/specs/metrics-audit/telemetry-schema.md @@ -190,13 +190,13 @@ not emitted by azd spans. | Field | OTel Key | Classification | Purpose | Notes | |-------|----------|----------------|---------|-------| | Remote build count | `container.remoteBuild.count` | SystemMetadata | FeatureInsight | **Measurement** | -| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. | +| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. Not hashed; not a measurement. | ### AKS | Field | OTel Key | Classification | Purpose | Notes | |-------|----------|----------------|---------|-------| -| Skip reason | `skip.reason` | SystemMetadata | FeatureInsight | Bounded enum (`cluster_not_provisioned`); never raw error text. Emitted on `aks.postprovision.skip`. | +| Skip reason | `skip.reason` | SystemMetadata | FeatureInsight | Bounded enum (`cluster_not_provisioned`); never raw error text. Emitted on `aks.postprovision.skip`. Not hashed; not a measurement. | ### JSON-RPC @@ -324,7 +324,7 @@ The following fields are defined in `fields.go`. | Field | OTel Key | Classification | Purpose | Values | |-------|----------|----------------|---------|--------| | Auth method | `auth.method` | SystemMetadata | FeatureInsight | `browser`, `device-code`, `service-principal-secret`, `service-principal-certificate`, `federated-github`, `federated-azure-pipelines`, `federated-oidc`, `managed-identity`, `external`, `oneauth`, `check-status` | -| Auth cache-clear failed | `auth.cache_clear_failed` | SystemMetadata | PerformanceAndHealth | Fixed enum (`auth`, `subscriptions`) identifying which cache failed to clear during the pre-login cleanup. Emitted on the `auth login` usage event. | +| Auth cache-clear failed | `auth.cache_clear_failed` | SystemMetadata | PerformanceAndHealth | Fixed enum (`auth`, `subscriptions`) identifying which cache failed to clear during the pre-login cleanup. Emitted on the `auth login` usage event. Not hashed; not a measurement. | | Env count | `env.count` | SystemMetadata | FeatureInsight | **Measurement** — number of environments | | Hooks name | `hooks.name` | SystemMetadata | FeatureInsight | Built-in hook name (raw) or SHA-256 hash for extension/custom hooks. Known values: `prebuild`, `postbuild`, `predeploy`, `postdeploy`, `predown`, `postdown`, `prepackage`, `postpackage`, `preprovision`, `postprovision`, `prepublish`, `postpublish`, `prerestore`, `postrestore`, `preup`, `postup` | | Hooks type | `hooks.type` | SystemMetadata | FeatureInsight | `project`, `service`, `layer` | From ecea9a26a043bb7c613dc589b030b6c92e5c01d3 Mon Sep 17 00:00:00 2001 From: hemarina Date: Sat, 15 Aug 2026 00:41:29 -0700 Subject: [PATCH 07/18] address feedback --- cli/azd/cmd/telemetry_test.go | 158 ++++++++++++++++-- .../metrics-audit/feature-telemetry-matrix.md | 3 +- .../metrics-audit/privacy-review-checklist.md | 2 +- 3 files changed, 143 insertions(+), 20 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 2e2904a6ab1..061859cd172 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -421,6 +421,78 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) return "..." } + // isAttrKeyType reports whether an expression is the raw attribute.Key type + // reference (e.g. `attribute.Key`), respecting the resolved package alias. + isAttrKeyType := func(expr ast.Expr) bool { + sel, ok := expr.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Key" { + return false + } + base, ok := sel.X.(*ast.Ident) + return ok && base.Name == attrPkgName + } + // isAttrKeyCall reports whether an expression is an attribute.Key(...) call, + // which yields a raw attribute.Key value. + isAttrKeyCall := func(expr ast.Expr) bool { + call, ok := expr.(*ast.CallExpr) + return ok && isAttrKeyType(call.Fun) + } + + // rawKeyIdents collects identifiers whose type is the raw attribute.Key (never + // the classified fields.AttributeKey, which is a distinct named struct type). + // A KeyValue-producing method call on one of these — e.g. a parameter + // `k attribute.Key` used as `k.String(v)` — emits an unregistered key just + // like a direct constructor call. This is lightweight declaration tracking, + // not full type inference: it covers explicitly typed function parameters / + // results / receivers, var / const declarations, and `k := attribute.Key(...)` + // short declarations — the realistically reachable forms. + rawKeyIdents := map[string]struct{}{} + addFieldNames := func(fl *ast.FieldList) { + if fl == nil { + return + } + for _, f := range fl.List { + if isAttrKeyType(f.Type) { + for _, name := range f.Names { + rawKeyIdents[name.Name] = struct{}{} + } + } + } + } + ast.Inspect(file, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.FuncDecl: + addFieldNames(node.Recv) + if node.Type != nil { + addFieldNames(node.Type.Params) + addFieldNames(node.Type.Results) + } + case *ast.FuncLit: + if node.Type != nil { + addFieldNames(node.Type.Params) + addFieldNames(node.Type.Results) + } + case *ast.ValueSpec: + if node.Type != nil && isAttrKeyType(node.Type) { + for _, name := range node.Names { + rawKeyIdents[name.Name] = struct{}{} + } + } + case *ast.AssignStmt: + if node.Tok == token.DEFINE { + for i, lhs := range node.Lhs { + if i >= len(node.Rhs) { + break + } + if id, ok := lhs.(*ast.Ident); ok && isAttrKeyCall(node.Rhs[i]) { + rawKeyIdents[id.Name] = struct{}{} + } + } + } + } + return true + }) + var violations []string ast.Inspect(file, func(n ast.Node) bool { call, ok := n.(*ast.CallExpr) @@ -451,12 +523,22 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) return true } + // Form C: k.String(v) where k is a value of the raw attribute.Key type + // (a parameter, var/const, or `k := attribute.Key(...)`). This emits an + // unregistered key. It is distinct from the sanctioned + // fields.SomeKey.String(v) promoted-method call, whose receiver is the + // classified fields.AttributeKey struct type, not attribute.Key. + if base, ok := sel.X.(*ast.Ident); ok { + if _, isRawKey := rawKeyIdents[base.Name]; isRawKey { + violations = append(violations, fmt.Sprintf( + " %s:%d: %s.%s(...) on a raw attribute.Key value", rel, pos.Line, base.Name, sel.Sel.Name)) + return true + } + } + // Form B: attribute.Key("key").String(v) — the selector base is an inline // attribute.Key(...) constructor call. This produces a KeyValue that - // likewise bypasses the fields.AttributeKey registry. (A method call on a - // Key-typed *variable* is indistinguishable at the AST level from the - // sanctioned fields.SomeKey.String(v) promoted-method call, so only the - // inline form is enforceable here.) + // likewise bypasses the fields.AttributeKey registry. if inner, ok := sel.X.(*ast.CallExpr); ok { if innerSel, ok := inner.Fun.(*ast.SelectorExpr); ok { if innerBase, ok := innerSel.X.(*ast.Ident); ok && @@ -476,55 +558,92 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) // TestRawTelemetryAttributeScanner is a fixture test for the AST guard used by // TestNoRawTelemetryAttributes. It pins the contract: raw attribute constructors, -// aliased imports, constant keys, and the chained attribute.Key(k).String(v) form -// are all flagged, while the sanctioned promoted-method pattern and a bare -// attribute.Key(k) (which does not build a KeyValue) are not. +// aliased imports, constant keys, the chained attribute.Key(k).String(v) form, +// and KeyValue-producing method calls on a raw attribute.Key value are all +// flagged, while the sanctioned fields.AttributeKey promoted-method pattern, a +// bare attribute.Key(k) (which does not build a KeyValue), and non-KeyValue uses +// of a key (e.g. a map lookup) are not. func TestRawTelemetryAttributeScanner(t *testing.T) { t.Parallel() + // Each fixture is composed as "package p" + an import line + a body so the + // individual source strings stay within the line-length limit. + const ( + stdImport = `import "go.opentelemetry.io/otel/attribute"` + aliasImport = `import otelattr "go.opentelemetry.io/otel/attribute"` + noImport = `` + ) + cases := []struct { name string - src string + imports string + body string wantViolation bool }{ { name: "literal key", - src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.String("raw.key", "v")`, + imports: stdImport, + body: `var _ = attribute.String("raw.key", "v")`, wantViolation: true, }, { name: "constant key", - src: `package p; import "go.opentelemetry.io/otel/attribute"; const k = "raw.key"; var _ = attribute.String(k, "v")`, + imports: stdImport, + body: `const k = "raw.key"; var _ = attribute.String(k, "v")`, wantViolation: true, }, { name: "aliased import", - src: `package p; import otelattr "go.opentelemetry.io/otel/attribute"; var _ = otelattr.Bool("raw.key", true)`, + imports: aliasImport, + body: `var _ = otelattr.Bool("raw.key", true)`, wantViolation: true, }, { name: "int slice constructor", - src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.IntSlice("raw.key", []int{1})`, + imports: stdImport, + body: `var _ = attribute.IntSlice("raw.key", []int{1})`, wantViolation: true, }, { name: "chained key method", - src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.Key("raw.key").String("v")`, + imports: stdImport, + body: `var _ = attribute.Key("raw.key").String("v")`, + wantViolation: true, + }, + { + name: "method on key-typed parameter", + imports: stdImport, + body: `func f(k attribute.Key) { _ = k.String("v") }`, + wantViolation: true, + }, + { + name: "method on locally built key value", + imports: stdImport, + body: `func f() { k := attribute.Key("raw.key"); _ = k.String("v") }`, wantViolation: true, }, { name: "bare key builder without value", - src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.Key("raw.key")`, + imports: stdImport, + body: `var _ = attribute.Key("raw.key")`, wantViolation: false, }, { - name: "promoted method on key-typed value", - src: `package p; import "go.opentelemetry.io/otel/attribute"; func f(k attribute.Key) { _ = k.String("v") }`, + name: "promoted method on classified field", + imports: stdImport, + body: `var _ = fields.SomeKey.String("v")`, + wantViolation: false, + }, + { + name: "map lookup on key-typed parameter", + imports: stdImport, + body: `func f(m map[attribute.Key]int, k attribute.Key) int { return m[k] }`, wantViolation: false, }, { name: "file without the attribute import", - src: `package p; var _ = 1`, + imports: noImport, + body: `var _ = 1`, wantViolation: false, }, } @@ -533,8 +652,9 @@ func TestRawTelemetryAttributeScanner(t *testing.T) { tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() + src := "package p\n" + tc.imports + "\n" + tc.body + "\n" fset := token.NewFileSet() - file, err := parser.ParseFile(fset, tc.name+".go", tc.src, 0) + file, err := parser.ParseFile(fset, tc.name+".go", src, 0) require.NoError(t, err) got := scanGoFileForRawAttributes(fset, file, tc.name+".go") @@ -546,6 +666,8 @@ func TestRawTelemetryAttributeScanner(t *testing.T) { }) } } + +// TestCommandTelemetryCoverage ensures every user-facing command is explicitly categorized // for telemetry coverage. When a new command is added to the CLI, it must be added to one // of the lists below. This forces developers to consciously decide whether the command needs // command-specific telemetry attributes or whether global middleware telemetry is sufficient. diff --git a/docs/specs/metrics-audit/feature-telemetry-matrix.md b/docs/specs/metrics-audit/feature-telemetry-matrix.md index 6538fbbba13..23d5c118f18 100644 --- a/docs/specs/metrics-audit/feature-telemetry-matrix.md +++ b/docs/specs/metrics-audit/feature-telemetry-matrix.md @@ -43,7 +43,7 @@ These commands emit attributes or events beyond the global middleware span. | Command | Subcommands | Global Span | Command-Specific Attrs | Feature Events | Notes | |---------|-------------|:-----------:|:----------------------:|:--------------:|-------| | **Auth** | | | | | | -| `auth login` | — | ✅ | ✅ | ❌ | `auth.method` (browser, device-code, service-principal-secret, etc.) | +| `auth login` | — | ✅ | ✅ | ❌ | `auth.method` (browser, device-code, service-principal-secret, etc.); `auth.cache_clear_failed` (which credential cache failed to clear during pre-login cleanup) | | `auth logout` | — | ✅ | ❌ | ❌ | Global telemetry sufficient — no command-specific attributes emitted | | `auth status` | — | ✅ | ❌ | ❌ | Global telemetry sufficient — simple pass/fail check | | `auth token` | — | ✅ | ❌ | ❌ | Global telemetry sufficient | @@ -116,6 +116,7 @@ command-specific telemetry fields provide analytical value beyond the command na | Field | OTel Key | Commands | Justification | |-------|----------|----------|---------------| | Auth method | `auth.method` | `auth login`, `auth logout` | Distinguishes authentication flow type (browser, device-code, SP, federated, etc.) | +| Auth cache-clear failed | `auth.cache_clear_failed` | `auth login` | Identifies which credential cache failed to clear during the pre-login cleanup (`auth` / `subscriptions`) | | Env count | `env.count` | `env list` | Measurement — number of environments is a quantitative metric | | Hooks name | `hooks.name` | `hooks run` | Identifies which hook script ran (hashed — user-defined name) | | Hooks type | `hooks.type` | `hooks run` | Distinguishes project / service / **layer** hooks | diff --git a/docs/specs/metrics-audit/privacy-review-checklist.md b/docs/specs/metrics-audit/privacy-review-checklist.md index ed6d264b858..c9da6730aae 100644 --- a/docs/specs/metrics-audit/privacy-review-checklist.md +++ b/docs/specs/metrics-audit/privacy-review-checklist.md @@ -201,7 +201,7 @@ When adding a new telemetry field: - OTel key name - Classification - Purpose - - EndpointIdType (only when the value is a known endpoint identifier) + - Endpoint (only when the value is a known endpoint identifier) - Whether it is hashed - Whether it is a measurement - Allowed values (if enum) From dc520a28f790a19b0ce32cf0f5702908727e13ca Mon Sep 17 00:00:00 2001 From: hemarina Date: Sat, 15 Aug 2026 00:54:56 -0700 Subject: [PATCH 08/18] improve tests --- cli/azd/cmd/telemetry_test.go | 75 ++++++++++++++++++++++++++--------- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 061859cd172..489afa06c0e 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -438,15 +438,30 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) return ok && isAttrKeyType(call.Fun) } - // rawKeyIdents collects identifiers whose type is the raw attribute.Key (never - // the classified fields.AttributeKey, which is a distinct named struct type). - // A KeyValue-producing method call on one of these — e.g. a parameter - // `k attribute.Key` used as `k.String(v)` — emits an unregistered key just - // like a direct constructor call. This is lightweight declaration tracking, - // not full type inference: it covers explicitly typed function parameters / - // results / receivers, var / const declarations, and `k := attribute.Key(...)` - // short declarations — the realistically reachable forms. - rawKeyIdents := map[string]struct{}{} + // rawKeyObjs records the *ast.Object identity — not merely the name — of every + // value declared with the raw attribute.Key type. A KeyValue-producing method + // call on one of these (e.g. a parameter `k attribute.Key` used as + // `k.String(v)`) emits an unregistered key just like a direct constructor + // call. Tracking object identity (resolved by the parser's scope resolver, so + // each declaration is distinct) keeps the guard sound across scopes: a + // shadowing classified `k` in another function resolves to a different object + // and is not misreported. Both explicitly typed declarations and inferred ones + // (`var k = attribute.Key("raw.key")`, `k := attribute.Key(...)`) are covered. + // + // ast.Object is deprecated (SA1019) because Ident/Object relationships cannot + // be resolved without type information in the general case (e.g. composite + // literal keys). That ambiguity does not apply here: this guard only records + // objects whose declaration type/initializer is attribute.Key and only + // consults Obj for identifiers in method-receiver (value) position, where the + // parser's per-file scope resolution is exact. A full go/types pass would add + // package loading and build dependencies for no additional soundness here. + //nolint:staticcheck // ast.Object scope resolution is exact for this per-file guard + rawKeyObjs := map[*ast.Object]struct{}{} + record := func(id *ast.Ident) { + if id != nil && id.Obj != nil { + rawKeyObjs[id.Obj] = struct{}{} + } + } addFieldNames := func(fl *ast.FieldList) { if fl == nil { return @@ -454,7 +469,7 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) for _, f := range fl.List { if isAttrKeyType(f.Type) { for _, name := range f.Names { - rawKeyIdents[name.Name] = struct{}{} + record(name) } } } @@ -473,19 +488,30 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) addFieldNames(node.Type.Results) } case *ast.ValueSpec: - if node.Type != nil && isAttrKeyType(node.Type) { - for _, name := range node.Names { - rawKeyIdents[name.Name] = struct{}{} + // Explicitly typed: var k attribute.Key + if node.Type != nil { + if isAttrKeyType(node.Type) { + for _, name := range node.Names { + record(name) + } + } + break + } + // Inferred: var k = attribute.Key("raw.key") + for i, name := range node.Names { + if i < len(node.Values) && isAttrKeyCall(node.Values[i]) { + record(name) } } case *ast.AssignStmt: + // Short declaration: k := attribute.Key("raw.key") if node.Tok == token.DEFINE { for i, lhs := range node.Lhs { if i >= len(node.Rhs) { break } if id, ok := lhs.(*ast.Ident); ok && isAttrKeyCall(node.Rhs[i]) { - rawKeyIdents[id.Name] = struct{}{} + record(id) } } } @@ -527,9 +553,11 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) // (a parameter, var/const, or `k := attribute.Key(...)`). This emits an // unregistered key. It is distinct from the sanctioned // fields.SomeKey.String(v) promoted-method call, whose receiver is the - // classified fields.AttributeKey struct type, not attribute.Key. - if base, ok := sel.X.(*ast.Ident); ok { - if _, isRawKey := rawKeyIdents[base.Name]; isRawKey { + // classified fields.AttributeKey struct type, not attribute.Key. The + // receiver is matched by object identity so a shadowing classified `k` + // elsewhere is not misreported. + if base, ok := sel.X.(*ast.Ident); ok && base.Obj != nil { + if _, isRawKey := rawKeyObjs[base.Obj]; isRawKey { violations = append(violations, fmt.Sprintf( " %s:%d: %s.%s(...) on a raw attribute.Key value", rel, pos.Line, base.Name, sel.Sel.Name)) return true @@ -622,6 +650,12 @@ func TestRawTelemetryAttributeScanner(t *testing.T) { body: `func f() { k := attribute.Key("raw.key"); _ = k.String("v") }`, wantViolation: true, }, + { + name: "method on inferred key declaration", + imports: stdImport, + body: `var k = attribute.Key("raw.key"); var _ = k.String("v")`, + wantViolation: true, + }, { name: "bare key builder without value", imports: stdImport, @@ -634,6 +668,12 @@ func TestRawTelemetryAttributeScanner(t *testing.T) { body: `var _ = fields.SomeKey.String("v")`, wantViolation: false, }, + { + name: "shadowed classified key across scopes", + imports: stdImport, + body: `func a(k attribute.Key) { _ = k }; func b(k fields.AttributeKey) { _ = k.Bool(true) }`, + wantViolation: false, + }, { name: "map lookup on key-typed parameter", imports: stdImport, @@ -649,7 +689,6 @@ func TestRawTelemetryAttributeScanner(t *testing.T) { } for _, tc := range cases { - tc := tc t.Run(tc.name, func(t *testing.T) { t.Parallel() src := "package p\n" + tc.imports + "\n" + tc.body + "\n" From 2417a091845f5f0ce1f94f68d63549e1d7bce99f Mon Sep 17 00:00:00 2001 From: hemarina Date: Sat, 15 Aug 2026 01:40:14 -0700 Subject: [PATCH 09/18] enhance testing --- cli/azd/cmd/telemetry_test.go | 607 ++++++++++++++++++---------------- cli/azd/go.mod | 2 + cli/azd/go.sum | 4 + cli/azd/pkg/azapi/webapp.go | 4 +- 4 files changed, 323 insertions(+), 294 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 489afa06c0e..76eea43d700 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -6,14 +6,14 @@ package cmd import ( "fmt" "go/ast" - "go/parser" "go/token" - "os" + "go/types" "path/filepath" - "strconv" "strings" "testing" + "golang.org/x/tools/go/packages" + "github.com/stretchr/testify/require" "github.com/azure/azure-dev/cli/azd/internal" @@ -291,18 +291,27 @@ func TestTelemetryFieldConstants(t *testing.T) { } // TestNoRawTelemetryAttributes enforces that product code never emits telemetry -// via raw attribute.String(key, ...) / attribute.Bool(key, ...) — or the chained -// attribute.Key(key).String(...) form — whether the key is a string literal or a -// named constant, and whether the package is imported under its default name or -// an alias. Every telemetry attribute must be +// via a raw attribute constructor — attribute.String(key, ...), attribute.Bool, +// the chained attribute.Key(key).String(...) form, or a KeyValue-producing method +// called on any value of type attribute.Key. Every telemetry attribute must be // declared as a fields.AttributeKey (with a Classification and Purpose) and // emitted through it, e.g. fields.SomeKey.String(value). This keeps the telemetry // schema discoverable and classifiable for the GDPR metadata pipeline (azure-dev // issue #1803). // +// The scan is type-aware (go/types via go/packages) rather than purely +// syntactic. Type information is required for soundness: the sanctioned +// fields.SomeKey.String(v) is a promoted method whose receiver is the classified +// fields.AttributeKey struct, which is AST-indistinguishable from a bare +// attribute.Key value's method call. Only the resolved types tell the two named +// types apart, and they also let the guard follow keys reached through import +// aliases, dot imports, struct fields, or function results. +// // Legitimately excluded from the scan: -// - *_test.go files (test fixtures build raw attributes on purpose). -// - extensions/... — independent extension modules with their own schema. +// - *_test.go files (test fixtures build raw attributes on purpose): Tests is +// false, so go/packages does not load them. +// - Nested modules (extensions/*, test/evals, test data samples) have their own +// go.mod and are not matched by the "./..." pattern. func TestNoRawTelemetryAttributes(t *testing.T) { t.Parallel() @@ -311,41 +320,41 @@ func TestNoRawTelemetryAttributes(t *testing.T) { azdRoot, err := filepath.Abs("..") require.NoError(t, err) - var violations []string - - err = filepath.Walk(azdRoot, func(path string, info os.FileInfo, err error) error { - if err != nil { - return err - } - - if info.IsDir() { - switch filepath.Base(path) { - case "vendor", "extensions", "testdata", "node_modules", ".git": - return filepath.SkipDir - } - return nil - } + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | + packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports, + Dir: azdRoot, + Tests: false, + } + pkgs, err := packages.Load(cfg, "./...") + require.NoError(t, err) + require.NotEmpty(t, pkgs, "no packages loaded from the cli/azd module") - if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { - return nil + var ( + violations []string + loadErrors []string + ) + for _, pkg := range pkgs { + for _, e := range pkg.Errors { + loadErrors = append(loadErrors, fmt.Sprintf(" %s: %s", pkg.PkgPath, e.Error())) } - - rel, relErr := filepath.Rel(azdRoot, path) - if relErr != nil { - rel = path + if pkg.TypesInfo == nil { + continue } - rel = filepath.ToSlash(rel) - - fset := token.NewFileSet() - file, parseErr := parser.ParseFile(fset, path, nil, 0) - if parseErr != nil { - return nil // skip unparseable files + for _, file := range pkg.Syntax { + filename := pkg.Fset.Position(file.Pos()).Filename + rel, relErr := filepath.Rel(azdRoot, filename) + if relErr != nil { + rel = filename + } + rel = filepath.ToSlash(rel) + violations = append(violations, scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, rel)...) } + } - violations = append(violations, scanGoFileForRawAttributes(fset, file, rel)...) - return nil - }) - require.NoError(t, err) + // A package that failed to type-check would silently hide violations, so a + // load error is a failure rather than a false pass. + require.Empty(t, loadErrors, "packages failed to load/type-check:\n%s", strings.Join(loadErrors, "\n")) if len(violations) > 0 { t.Errorf( @@ -360,164 +369,92 @@ func TestNoRawTelemetryAttributes(t *testing.T) { } } -// scanGoFileForRawAttributes returns the raw-telemetry-attribute violations in a -// single parsed Go file. rel is the display path used in messages. It is shared -// by TestNoRawTelemetryAttributes (which walks the module tree) and the fixture -// test TestRawTelemetryAttributeScanner, so the guard's contract is itself tested. -func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) []string { +// rawAttributePkgPath is the import path of the OpenTelemetry attribute package +// whose constructors and Key methods bypass the fields.AttributeKey registry. +const rawAttributePkgPath = "go.opentelemetry.io/otel/attribute" + +// isRawAttributeKeyType reports whether t is exactly the +// go.opentelemetry.io/otel/attribute.Key named type. A struct that merely embeds +// it — such as the sanctioned fields.AttributeKey — is a different named type and +// returns false, which is what keeps the guard from flagging fields.SomeKey.String. +func isRawAttributeKeyType(t types.Type) bool { + named, ok := t.(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj != nil && obj.Pkg() != nil && + obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "Key" +} + +// scanFileForRawAttributes returns the raw-telemetry-attribute violations in a +// single type-checked Go file. info must be the go/types information for the +// file's package; rel is the display path used in messages. Classifying calls by +// the resolved type of the callee and receiver — rather than by syntactic shape — +// makes the guard sound across import aliases, dot imports, and keys reached +// through parameters, struct fields, or function results. It is shared by +// TestNoRawTelemetryAttributes (which walks the module) and the fixture test +// TestRawTelemetryAttributeScanner, so the guard's contract is itself tested. +func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.Info, rel string) []string { // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers // (and the identically named attribute.Key methods) that build a KeyValue from - // a key and value. Using them directly in product code bypasses the + // a key and a value. Using them directly in product code bypasses the // fields.AttributeKey registry, so the GDPR metadata exporter (which discovers // only exported AttributeKey vars) can never classify the resulting property. // See docs/specs/metrics-audit/telemetry-schema.md. rawAttributeConstructors := map[string]struct{}{ - "String": {}, - "Bool": {}, - "Int": {}, - "IntSlice": {}, - "Int64": {}, - "Float64": {}, - "Stringer": {}, - "StringSlice": {}, - "BoolSlice": {}, - "Int64Slice": {}, - "Float64Slice": {}, - } - - // Resolve the local name bound to go.opentelemetry.io/otel/attribute in this - // file. Matching the selector base literally against "attribute" would miss an - // aliased import (e.g. otelattr "...otel/attribute") and could also misfire on - // an unrelated local identifier named "attribute". If the file does not import - // the package, it cannot construct a raw attribute, so there is nothing to scan. - attrPkgName := "" - for _, imp := range file.Imports { - importPath, uErr := strconv.Unquote(imp.Path.Value) - if uErr != nil || importPath != "go.opentelemetry.io/otel/attribute" { - continue - } - if imp.Name != nil { - attrPkgName = imp.Name.Name // explicit alias - } else { - attrPkgName = "attribute" // default package name - } - break - } - // A blank ("_") or dot (".") import cannot produce a "pkg.Constructor" - // selector, so there is nothing this AST check can match on. - if attrPkgName == "" || attrPkgName == "_" || attrPkgName == "." { - return nil + "String": {}, "Bool": {}, "Int": {}, "IntSlice": {}, "Int64": {}, + "Float64": {}, "Stringer": {}, "StringSlice": {}, "BoolSlice": {}, + "Int64Slice": {}, "Float64Slice": {}, } // literalKey renders the first string-literal argument of a call for a - // friendlier message, or "..." for a non-literal (e.g. const) key. + // friendlier message, or "..." for a non-literal (e.g. a named constant). literalKey := func(c *ast.CallExpr) string { - if len(c.Args) == 0 { - return "..." - } - if lit, ok := c.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { - return lit.Value + if len(c.Args) > 0 { + if lit, ok := c.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { + return lit.Value + } } return "..." } - // isAttrKeyType reports whether an expression is the raw attribute.Key type - // reference (e.g. `attribute.Key`), respecting the resolved package alias. - isAttrKeyType := func(expr ast.Expr) bool { - sel, ok := expr.(*ast.SelectorExpr) - if !ok || sel.Sel.Name != "Key" { + // isRawAttributeConstructorFunc reports whether obj is a package-level function + // of the attribute package that builds a KeyValue from a key and value (e.g. + // attribute.String). Resolving via the types object makes this independent of + // how the package was imported: default name, alias, or dot import. + isRawAttributeConstructorFunc := func(obj types.Object) bool { + fn, ok := obj.(*types.Func) + if !ok || fn.Pkg() == nil || fn.Pkg().Path() != rawAttributePkgPath { return false } - base, ok := sel.X.(*ast.Ident) - return ok && base.Name == attrPkgName - } - // isAttrKeyCall reports whether an expression is an attribute.Key(...) call, - // which yields a raw attribute.Key value. - isAttrKeyCall := func(expr ast.Expr) bool { - call, ok := expr.(*ast.CallExpr) - return ok && isAttrKeyType(call.Fun) - } - - // rawKeyObjs records the *ast.Object identity — not merely the name — of every - // value declared with the raw attribute.Key type. A KeyValue-producing method - // call on one of these (e.g. a parameter `k attribute.Key` used as - // `k.String(v)`) emits an unregistered key just like a direct constructor - // call. Tracking object identity (resolved by the parser's scope resolver, so - // each declaration is distinct) keeps the guard sound across scopes: a - // shadowing classified `k` in another function resolves to a different object - // and is not misreported. Both explicitly typed declarations and inferred ones - // (`var k = attribute.Key("raw.key")`, `k := attribute.Key(...)`) are covered. - // - // ast.Object is deprecated (SA1019) because Ident/Object relationships cannot - // be resolved without type information in the general case (e.g. composite - // literal keys). That ambiguity does not apply here: this guard only records - // objects whose declaration type/initializer is attribute.Key and only - // consults Obj for identifiers in method-receiver (value) position, where the - // parser's per-file scope resolution is exact. A full go/types pass would add - // package loading and build dependencies for no additional soundness here. - //nolint:staticcheck // ast.Object scope resolution is exact for this per-file guard - rawKeyObjs := map[*ast.Object]struct{}{} - record := func(id *ast.Ident) { - if id != nil && id.Obj != nil { - rawKeyObjs[id.Obj] = struct{}{} + sig, ok := fn.Type().(*types.Signature) + if !ok || sig.Recv() != nil { + return false } + _, isCtor := rawAttributeConstructors[fn.Name()] + return isCtor } - addFieldNames := func(fl *ast.FieldList) { - if fl == nil { - return + + // isReemittedKeyValueKey reports whether expr is `.Key` where has type + // attribute.KeyValue — i.e. a method call like kv.Key.String(v) merely re-emits + // the key of a KeyValue that was already built (and, at its build site, already + // subject to this guard). The telemetry baggage plumbing in + // internal/tracing legitimately rebuilds caller-supplied KeyValues with merged + // values this way; it introduces no new key literal, so it is not a violation. + isReemittedKeyValueKey := func(expr ast.Expr) bool { + sel, ok := expr.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Key" { + return false } - for _, f := range fl.List { - if isAttrKeyType(f.Type) { - for _, name := range f.Names { - record(name) - } - } + named, ok := info.TypeOf(sel.X).(*types.Named) + if !ok { + return false } + obj := named.Obj() + return obj != nil && obj.Pkg() != nil && + obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "KeyValue" } - ast.Inspect(file, func(n ast.Node) bool { - switch node := n.(type) { - case *ast.FuncDecl: - addFieldNames(node.Recv) - if node.Type != nil { - addFieldNames(node.Type.Params) - addFieldNames(node.Type.Results) - } - case *ast.FuncLit: - if node.Type != nil { - addFieldNames(node.Type.Params) - addFieldNames(node.Type.Results) - } - case *ast.ValueSpec: - // Explicitly typed: var k attribute.Key - if node.Type != nil { - if isAttrKeyType(node.Type) { - for _, name := range node.Names { - record(name) - } - } - break - } - // Inferred: var k = attribute.Key("raw.key") - for i, name := range node.Names { - if i < len(node.Values) && isAttrKeyCall(node.Values[i]) { - record(name) - } - } - case *ast.AssignStmt: - // Short declaration: k := attribute.Key("raw.key") - if node.Tok == token.DEFINE { - for i, lhs := range node.Lhs { - if i >= len(node.Rhs) { - break - } - if id, ok := lhs.(*ast.Ident); ok && isAttrKeyCall(node.Rhs[i]) { - record(id) - } - } - } - } - return true - }) var violations []string ast.Inspect(file, func(n ast.Node) bool { @@ -525,184 +462,270 @@ func scanGoFileForRawAttributes(fset *token.FileSet, file *ast.File, rel string) if !ok { return true } - - sel, ok := call.Fun.(*ast.SelectorExpr) - if !ok { - return true - } - - // Only the KeyValue-producing constructor / key-method names are of - // interest (String, Bool, Int, ... — see rawAttributeConstructors). - if _, isConstructor := rawAttributeConstructors[sel.Sel.Name]; !isConstructor { - return true - } - pos := fset.Position(call.Pos()) - // Form A: attribute.String("key", v) / attribute.Bool(k, v) — the selector - // base is the imported package identifier. This bypasses the - // fields.AttributeKey registry, so the GDPR classifier can never see it, - // whether the key is a string literal or a named constant. - if base, ok := sel.X.(*ast.Ident); ok && base.Name == attrPkgName { - violations = append(violations, fmt.Sprintf( - " %s:%d: %s.%s(%s, ...)", rel, pos.Line, attrPkgName, sel.Sel.Name, literalKey(call))) - return true - } - - // Form C: k.String(v) where k is a value of the raw attribute.Key type - // (a parameter, var/const, or `k := attribute.Key(...)`). This emits an - // unregistered key. It is distinct from the sanctioned - // fields.SomeKey.String(v) promoted-method call, whose receiver is the - // classified fields.AttributeKey struct type, not attribute.Key. The - // receiver is matched by object identity so a shadowing classified `k` - // elsewhere is not misreported. - if base, ok := sel.X.(*ast.Ident); ok && base.Obj != nil { - if _, isRawKey := rawKeyObjs[base.Obj]; isRawKey { - violations = append(violations, fmt.Sprintf( - " %s:%d: %s.%s(...) on a raw attribute.Key value", rel, pos.Line, base.Name, sel.Sel.Name)) + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + // A method-value selection (x.String(v)) — including a method promoted + // through embedding — carries the resolved receiver type. It is a raw + // attribute only when that receiver is exactly attribute.Key. The + // sanctioned fields.SomeKey.String(v) has receiver type + // fields.AttributeKey (a distinct named struct that embeds + // attribute.Key) and is correctly left alone. This single check covers + // keys held in parameters, locals, struct fields, and function + // results, as well as the chained attribute.Key("k").String(v) form. + // Re-emitting an existing KeyValue's key (kv.Key.String(v)) is excluded + // because the key was already checked where the KeyValue was built. + if sel := info.Selections[fun]; sel != nil && sel.Kind() == types.MethodVal { + if m, ok := sel.Obj().(*types.Func); ok { + if _, isCtor := rawAttributeConstructors[m.Name()]; isCtor && + isRawAttributeKeyType(sel.Recv()) && !isReemittedKeyValueKey(fun.X) { + violations = append(violations, fmt.Sprintf( + " %s:%d: .%s(...) on a raw attribute.Key value", rel, pos.Line, m.Name())) + } + } return true } - } - - // Form B: attribute.Key("key").String(v) — the selector base is an inline - // attribute.Key(...) constructor call. This produces a KeyValue that - // likewise bypasses the fields.AttributeKey registry. - if inner, ok := sel.X.(*ast.CallExpr); ok { - if innerSel, ok := inner.Fun.(*ast.SelectorExpr); ok { - if innerBase, ok := innerSel.X.(*ast.Ident); ok && - innerBase.Name == attrPkgName && innerSel.Sel.Name == "Key" { - violations = append(violations, fmt.Sprintf( - " %s:%d: %s.Key(%s).%s(...)", rel, pos.Line, attrPkgName, literalKey(inner), sel.Sel.Name)) - return true - } + // Otherwise the selector is a qualified identifier for a package-level + // constructor, e.g. attribute.String("k", v) (possibly via an alias). + if isRawAttributeConstructorFunc(info.Uses[fun.Sel]) { + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, fun.Sel.Name, literalKey(call))) + } + case *ast.Ident: + // A bare identifier call resolves to a package-level constructor only + // when the attribute package is dot-imported, e.g. String("k", v). + if isRawAttributeConstructorFunc(info.Uses[fun]) { + violations = append(violations, fmt.Sprintf( + " %s:%d: %s(%s, ...) (dot-imported attribute constructor)", + rel, pos.Line, fun.Name, literalKey(call))) } } - return true }) return violations } -// TestRawTelemetryAttributeScanner is a fixture test for the AST guard used by -// TestNoRawTelemetryAttributes. It pins the contract: raw attribute constructors, -// aliased imports, constant keys, the chained attribute.Key(k).String(v) form, -// and KeyValue-producing method calls on a raw attribute.Key value are all -// flagged, while the sanctioned fields.AttributeKey promoted-method pattern, a -// bare attribute.Key(k) (which does not build a KeyValue), and non-KeyValue uses -// of a key (e.g. a map lookup) are not. +// TestRawTelemetryAttributeScanner is a fixture test for the type-aware guard +// used by TestNoRawTelemetryAttributes. It pins the contract: raw attribute +// constructors (default, aliased, and dot-imported), constant keys, the chained +// attribute.Key(k).String(v) form, and KeyValue-producing method calls on a raw +// attribute.Key value reached through a parameter, a local, a struct field, or a +// function result are all flagged; while the sanctioned promoted-method call on a +// struct that embeds attribute.Key, a bare attribute.Key(k) conversion (which +// does not build a KeyValue), and non-KeyValue uses of a key (e.g. a map lookup) +// are not. Fixtures are type-checked against the real attribute package via +// go/packages, so the guard runs with the same type information it uses on the +// module. func TestRawTelemetryAttributeScanner(t *testing.T) { t.Parallel() - // Each fixture is composed as "package p" + an import line + a body so the - // individual source strings stay within the line-length limit. - const ( - stdImport = `import "go.opentelemetry.io/otel/attribute"` - aliasImport = `import otelattr "go.opentelemetry.io/otel/attribute"` - noImport = `` - ) + root, err := filepath.Abs("..") + require.NoError(t, err) cases := []struct { name string - imports string - body string + src string wantViolation bool }{ { - name: "literal key", - imports: stdImport, - body: `var _ = attribute.String("raw.key", "v")`, + name: "literal key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.String("raw.key", "v") +`, + wantViolation: true, + }, + { + name: "constant key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +const k = "raw.key" +var _ = attribute.String(k, "v") +`, + wantViolation: true, + }, + { + name: "aliased import", + src: `package p +import otelattr "go.opentelemetry.io/otel/attribute" +var _ = otelattr.Bool("raw.key", true) +`, + wantViolation: true, + }, + { + name: "dot-imported constructor", + src: `package p +import . "go.opentelemetry.io/otel/attribute" +var _ = String("raw.key", "v") +`, + wantViolation: true, + }, + { + name: "int slice constructor", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.IntSlice("raw.key", []int{1}) +`, wantViolation: true, }, { - name: "constant key", - imports: stdImport, - body: `const k = "raw.key"; var _ = attribute.String(k, "v")`, + name: "chained key method", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.Key("raw.key").String("v") +`, wantViolation: true, }, { - name: "aliased import", - imports: aliasImport, - body: `var _ = otelattr.Bool("raw.key", true)`, + name: "method on key-typed parameter", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f(k attribute.Key) { _ = k.String("v") } +`, wantViolation: true, }, { - name: "int slice constructor", - imports: stdImport, - body: `var _ = attribute.IntSlice("raw.key", []int{1})`, + name: "method on locally built key value", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f() { k := attribute.Key("raw.key"); _ = k.String("v") } +`, wantViolation: true, }, { - name: "chained key method", - imports: stdImport, - body: `var _ = attribute.Key("raw.key").String("v")`, + name: "method on inferred key declaration", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var k = attribute.Key("raw.key") +var _ = k.String("v") +`, wantViolation: true, }, { - name: "method on key-typed parameter", - imports: stdImport, - body: `func f(k attribute.Key) { _ = k.String("v") }`, + name: "method on struct-field key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type holder struct{ key attribute.Key } +func f(h holder) { _ = h.key.String("v") } +`, wantViolation: true, }, { - name: "method on locally built key value", - imports: stdImport, - body: `func f() { k := attribute.Key("raw.key"); _ = k.String("v") }`, + name: "method on function-result key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func mk() attribute.Key { return attribute.Key("raw.key") } +func f() { _ = mk().String("v") } +`, wantViolation: true, }, { - name: "method on inferred key declaration", - imports: stdImport, - body: `var k = attribute.Key("raw.key"); var _ = k.String("v")`, + name: "method via embedded Key field of a struct", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type classified struct{ attribute.Key } +func f(c classified) { _ = c.Key.Bool(true) } +`, wantViolation: true, }, { - name: "bare key builder without value", - imports: stdImport, - body: `var _ = attribute.Key("raw.key")`, + name: "reemit method on KeyValue key field", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f(kv attribute.KeyValue) { _ = kv.Key.String("v") } +`, wantViolation: false, }, { - name: "promoted method on classified field", - imports: stdImport, - body: `var _ = fields.SomeKey.String("v")`, + name: "bare key conversion without value", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.Key("raw.key") +`, wantViolation: false, }, { - name: "shadowed classified key across scopes", - imports: stdImport, - body: `func a(k attribute.Key) { _ = k }; func b(k fields.AttributeKey) { _ = k.Bool(true) }`, + name: "promoted method on embedding struct", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type classified struct{ attribute.Key } +var c classified +var _ = c.String("v") +`, wantViolation: false, }, { - name: "map lookup on key-typed parameter", - imports: stdImport, - body: `func f(m map[attribute.Key]int, k attribute.Key) int { return m[k] }`, + name: "shadowed classified key across scopes", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type classified struct{ attribute.Key } +func a(k attribute.Key) { _ = k } +func b(k classified) { _ = k.Bool(true) } +`, wantViolation: false, }, { - name: "file without the attribute import", - imports: noImport, - body: `var _ = 1`, + name: "map lookup on key-typed parameter", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f(m map[attribute.Key]int, k attribute.Key) int { return m[k] } +`, + wantViolation: false, + }, + { + name: "file without the attribute import", + src: `package p +var _ = 1 +`, wantViolation: false, }, } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - src := "package p\n" + tc.imports + "\n" + tc.body + "\n" - fset := token.NewFileSet() - file, err := parser.ParseFile(fset, tc.name+".go", src, 0) - require.NoError(t, err) - - got := scanGoFileForRawAttributes(fset, file, tc.name+".go") - if tc.wantViolation { - require.NotEmpty(t, got, "expected a violation for %q", tc.name) - } else { - require.Empty(t, got, "expected no violation for %q, got %v", tc.name, got) + // Type-check every fixture against the real attribute package via an in-memory + // overlay. The virtual files live under a directory that does not exist on + // disk, so they neither collide with the module walk nor require cleanup. + overlay := make(map[string][]byte, len(cases)) + patterns := make([]string, 0, len(cases)) + pathToCase := make(map[string]int, len(cases)) + for i, tc := range cases { + p := filepath.Join(root, "cmd", fmt.Sprintf("zz_rawscan_fixture_%02d", i), "fixture.go") + overlay[p] = []byte(tc.src) + patterns = append(patterns, "file="+p) + pathToCase[filepath.ToSlash(p)] = i + } + + cfg := &packages.Config{ + Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | + packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports, + Dir: root, + Overlay: overlay, + } + pkgs, err := packages.Load(cfg, patterns...) + require.NoError(t, err) + + got := make([]bool, len(cases)) + checked := make([]bool, len(cases)) + for _, pkg := range pkgs { + require.Empty(t, pkg.Errors, "fixture package %s failed to type-check", pkg.PkgPath) + if pkg.TypesInfo == nil { + continue + } + for _, file := range pkg.Syntax { + fname := filepath.ToSlash(pkg.Fset.Position(file.Pos()).Filename) + idx, ok := pathToCase[fname] + if !ok { + continue } - }) + got[idx] = len(scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, cases[idx].name)) > 0 + checked[idx] = true + } + } + + for i, tc := range cases { + require.True(t, checked[i], "fixture %q was not loaded/type-checked", tc.name) + require.Equal(t, tc.wantViolation, got[i], "fixture %q: unexpected violation result", tc.name) } } diff --git a/cli/azd/go.mod b/cli/azd/go.mod index bb372f4d03a..89094102181 100644 --- a/cli/azd/go.mod +++ b/cli/azd/go.mod @@ -86,6 +86,7 @@ require ( golang.org/x/sys v0.46.0 golang.org/x/term v0.44.0 golang.org/x/time v0.9.0 + golang.org/x/tools v0.45.0 google.golang.org/genproto/googleapis/rpc v0.0.0-20260414002931-afd174a4e478 google.golang.org/grpc v1.82.1 google.golang.org/protobuf v1.36.11 @@ -148,6 +149,7 @@ require ( go.opentelemetry.io/proto/otlp v1.10.0 // indirect golang.org/x/crypto v0.53.0 // indirect golang.org/x/exp v0.0.0-20250911091902-df9299821621 // indirect + golang.org/x/mod v0.36.0 // indirect golang.org/x/net v0.56.0 // indirect golang.org/x/text v0.38.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260414002931-afd174a4e478 // indirect diff --git a/cli/azd/go.sum b/cli/azd/go.sum index 49853c170fa..fa66976b57b 100644 --- a/cli/azd/go.sum +++ b/cli/azd/go.sum @@ -356,6 +356,8 @@ golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsi golang.org/x/exp v0.0.0-20250911091902-df9299821621 h1:2id6c1/gto0kaHYyrixvknJ8tUK/Qs5IsmBtrc+FtgU= golang.org/x/exp v0.0.0-20250911091902-df9299821621/go.mod h1:TwQYMMnGpvZyc+JpB/UAuTNIsVJifOlSkrZkhcvpVUk= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= +golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= +golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= @@ -395,6 +397,8 @@ golang.org/x/time v0.9.0/go.mod h1:3BpzKBy/shNhVucY/MWOyx10tF3SFh9QdLuxbVysPQM= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= +golang.org/x/tools v0.45.0 h1:18qN3FAooORvApf5XjCXgsuayZOEtXf6JK18I3+ONa8= +golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= diff --git a/cli/azd/pkg/azapi/webapp.go b/cli/azd/pkg/azapi/webapp.go index aed9a4d708c..86550e495df 100644 --- a/cli/azd/pkg/azapi/webapp.go +++ b/cli/azd/pkg/azapi/webapp.go @@ -189,7 +189,7 @@ func (cli *AzureClient) DeployAppServiceZip( } isLinux := isLinuxWebApp(app) - span.SetAttributes(fields.DeployLinuxKey.Key.Bool(isLinux)) + span.SetAttributes(fields.DeployLinuxKey.Bool(isLinux)) // Deployment Status API only support linux web app for now if isLinux && !skipStatusCheck && !isAppStopped(app) { @@ -200,7 +200,7 @@ func (cli *AzureClient) DeployAppServiceZip( // entire zip deploy when the build fails, giving the SCM time to stabilize. const maxBuildRetries = 2 for attempt := range maxBuildRetries + 1 { - span.SetAttributes(fields.DeployAttemptKey.Key.Int(attempt + 1)) + span.SetAttributes(fields.DeployAttemptKey.Int(attempt + 1)) if attempt > 0 { // Exponential backoff: 5s, 10s between retries to avoid hammering From 568b1dacb0fbe492e0693f2a0c3fb2d9cec8791b Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 13:21:02 -0700 Subject: [PATCH 10/18] update tests --- cli/azd/cmd/telemetry_test.go | 97 ++++++++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 19 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 76eea43d700..67c044930d4 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -311,7 +311,11 @@ func TestTelemetryFieldConstants(t *testing.T) { // - *_test.go files (test fixtures build raw attributes on purpose): Tests is // false, so go/packages does not load them. // - Nested modules (extensions/*, test/evals, test data samples) have their own -// go.mod and are not matched by the "./..." pattern. +// go.mod and are not matched by the "./..." pattern. Extension telemetry is +// out of scope by design, not merely by mechanics: extensions are separate +// modules whose attributes (the "ext.*" namespace) are reviewed together with +// the extension that reports them, per docs/specs/metrics-audit/ +// privacy-review-checklist.md, rather than against the core fields catalog. func TestNoRawTelemetryAttributes(t *testing.T) { t.Parallel() @@ -387,6 +391,41 @@ func isRawAttributeKeyType(t types.Type) bool { obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "Key" } +// fieldsPkgPath is the import path of the package that defines the sanctioned +// classified attribute wrapper, fields.AttributeKey. +const fieldsPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" + +// isFieldsAttributeKeyType reports whether t is exactly the sanctioned +// fields.AttributeKey named type. That wrapper is the only type the metadata +// classifier discovers and reads Classification/Purpose/Endpoint from, so it is +// the only receiver on which a KeyValue-producing attribute.Key method is +// allowed. A bare attribute.Key, or any other struct that merely embeds +// attribute.Key, produces a key the classifier cannot see and is a violation. +func isFieldsAttributeKeyType(t types.Type) bool { + named, ok := t.(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj != nil && obj.Pkg() != nil && + obj.Pkg().Path() == fieldsPkgPath && obj.Name() == "AttributeKey" +} + +// isAttributeKeyBuilderMethod reports whether m is a method defined on +// attribute.Key. The check is on the method's defining receiver type, so it +// matches whether the method is invoked directly on an attribute.Key value or +// promoted through an embedding struct, and it never matches an unrelated +// String()/Stringer() method on some other type. Callers additionally restrict +// to the KeyValue-producing names (rawAttributeConstructors) so attribute.Key's +// non-builder methods (e.g. Defined) are not flagged. +func isAttributeKeyBuilderMethod(m *types.Func) bool { + sig, ok := m.Type().(*types.Signature) + if !ok || sig.Recv() == nil { + return false + } + return isRawAttributeKeyType(sig.Recv().Type()) +} + // scanFileForRawAttributes returns the raw-telemetry-attribute violations in a // single type-checked Go file. info must be the go/types information for the // file's package; rel is the display path used in messages. Classifying calls by @@ -467,21 +506,25 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I switch fun := call.Fun.(type) { case *ast.SelectorExpr: // A method-value selection (x.String(v)) — including a method promoted - // through embedding — carries the resolved receiver type. It is a raw - // attribute only when that receiver is exactly attribute.Key. The - // sanctioned fields.SomeKey.String(v) has receiver type - // fields.AttributeKey (a distinct named struct that embeds - // attribute.Key) and is correctly left alone. This single check covers - // keys held in parameters, locals, struct fields, and function - // results, as well as the chained attribute.Key("k").String(v) form. - // Re-emitting an existing KeyValue's key (kv.Key.String(v)) is excluded - // because the key was already checked where the KeyValue was built. + // through embedding — resolves both the method's defining type and the + // receiver expression's type. A KeyValue-producing attribute.Key method + // is sanctioned only when the receiver is the classified + // fields.AttributeKey wrapper (the sole type the metadata classifier + // discovers). Called on a bare attribute.Key, or on any other struct + // that merely embeds attribute.Key, it produces an unclassified key and + // is a violation. This single check covers keys held in parameters, + // locals, struct fields, and function results, as well as the chained + // attribute.Key("k").String(v) form. Re-emitting an existing KeyValue's + // key (kv.Key.String(v)) is excluded because the key was already checked + // where the KeyValue was built. if sel := info.Selections[fun]; sel != nil && sel.Kind() == types.MethodVal { if m, ok := sel.Obj().(*types.Func); ok { if _, isCtor := rawAttributeConstructors[m.Name()]; isCtor && - isRawAttributeKeyType(sel.Recv()) && !isReemittedKeyValueKey(fun.X) { + isAttributeKeyBuilderMethod(m) && + !isFieldsAttributeKeyType(sel.Recv()) && !isReemittedKeyValueKey(fun.X) { violations = append(violations, fmt.Sprintf( - " %s:%d: .%s(...) on a raw attribute.Key value", rel, pos.Line, m.Name())) + " %s:%d: .%s(...) produces an unclassified key "+ + "(receiver is not fields.AttributeKey)", rel, pos.Line, m.Name())) } } return true @@ -625,8 +668,8 @@ func f() { _ = mk().String("v") } name: "method via embedded Key field of a struct", src: `package p import "go.opentelemetry.io/otel/attribute" -type classified struct{ attribute.Key } -func f(c classified) { _ = c.Key.Bool(true) } +type wrapper struct{ attribute.Key } +func f(c wrapper) { _ = c.Key.Bool(true) } `, wantViolation: true, }, @@ -647,12 +690,28 @@ var _ = attribute.Key("raw.key") wantViolation: false, }, { - name: "promoted method on embedding struct", + name: "promoted method on non-fields embedding struct", src: `package p import "go.opentelemetry.io/otel/attribute" -type classified struct{ attribute.Key } -var c classified +type wrapper struct{ attribute.Key } +var c wrapper var _ = c.String("v") +`, + wantViolation: true, + }, + { + name: "promoted method on classified fields.AttributeKey", + src: `package p +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +var _ = fields.ServiceNameKey.String("v") +`, + wantViolation: false, + }, + { + name: "dynamic sanctioned extension usage attribute", + src: `package p +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +var _ = fields.ExtensionUsageAttribute("foo").String("v") `, wantViolation: false, }, @@ -660,9 +719,9 @@ var _ = c.String("v") name: "shadowed classified key across scopes", src: `package p import "go.opentelemetry.io/otel/attribute" -type classified struct{ attribute.Key } +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" func a(k attribute.Key) { _ = k } -func b(k classified) { _ = k.Bool(true) } +func b(k fields.AttributeKey) { _ = k.Bool(true) } `, wantViolation: false, }, From 8fd82e8655604152e5e52106a51fcb872ce27298 Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 14:05:21 -0700 Subject: [PATCH 11/18] add tests --- cli/azd/cmd/telemetry_test.go | 61 +++++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 67c044930d4..039b085769a 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -352,7 +352,7 @@ func TestNoRawTelemetryAttributes(t *testing.T) { rel = filename } rel = filepath.ToSlash(rel) - violations = append(violations, scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, rel)...) + violations = append(violations, scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, pkg.PkgPath, rel)...) } } @@ -428,13 +428,15 @@ func isAttributeKeyBuilderMethod(m *types.Func) bool { // scanFileForRawAttributes returns the raw-telemetry-attribute violations in a // single type-checked Go file. info must be the go/types information for the -// file's package; rel is the display path used in messages. Classifying calls by -// the resolved type of the callee and receiver — rather than by syntactic shape — -// makes the guard sound across import aliases, dot imports, and keys reached -// through parameters, struct fields, or function results. It is shared by -// TestNoRawTelemetryAttributes (which walks the module) and the fixture test -// TestRawTelemetryAttributeScanner, so the guard's contract is itself tested. -func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.Info, rel string) []string { +// file's package; pkgPath is that package's import path (used to exempt the +// fields package from the construction rule below); rel is the display path used +// in messages. Classifying calls by the resolved type of the callee and receiver +// — rather than by syntactic shape — makes the guard sound across import aliases, +// dot imports, and keys reached through parameters, struct fields, or function +// results. It is shared by TestNoRawTelemetryAttributes (which walks the module) +// and the fixture test TestRawTelemetryAttributeScanner, so the guard's contract +// is itself tested. +func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.Info, pkgPath, rel string) []string { // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers // (and the identically named attribute.Key methods) that build a KeyValue from // a key and a value. Using them directly in product code bypasses the @@ -497,6 +499,26 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I var violations []string ast.Inspect(file, func(n ast.Node) bool { + // Constructing a fields.AttributeKey outside the fields package fabricates a + // key the classifier never sees. The GDPR scanner discovers only the + // exported package-level AttributeKey vars declared in the fields package, + // so a locally built one — e.g. fields.AttributeKey{Key: attribute.Key("x")} + // — carries an uncatalogued key even though its type is fields.AttributeKey, + // and the emission branch below would (correctly, to keep registered keys + // passed through parameters valid) treat that receiver type as sanctioned. + // The only sanctioned ways to obtain an AttributeKey in product code are to + // reference a registered fields.* var or to call fields.ExtensionUsageAttribute; + // both live in the fields package, which is why that package is exempt here. + if cl, ok := n.(*ast.CompositeLit); ok { + if pkgPath != fieldsPkgPath && isFieldsAttributeKeyType(info.TypeOf(cl)) { + pos := fset.Position(cl.Pos()) + violations = append(violations, fmt.Sprintf( + " %s:%d: fields.AttributeKey{...} constructed outside the fields "+ + "package (its key is not in the classifier catalog; reference a "+ + "registered fields.* key or fields.ExtensionUsageAttribute)", rel, pos.Line)) + } + return true + } call, ok := n.(*ast.CallExpr) if !ok { return true @@ -715,6 +737,27 @@ var _ = fields.ExtensionUsageAttribute("foo").String("v") `, wantViolation: false, }, + { + name: "locally constructed unregistered fields.AttributeKey emitted inline", + src: `package p +import "go.opentelemetry.io/otel/attribute" +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +var _ = fields.AttributeKey{Key: attribute.Key("raw.key")}.String("v") +`, + wantViolation: true, + }, + { + name: "unregistered fields.AttributeKey via local variable", + src: `package p +import "go.opentelemetry.io/otel/attribute" +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +func f() { + k := fields.AttributeKey{Key: attribute.Key("raw.key")} + _ = k.Bool(true) +} +`, + wantViolation: true, + }, { name: "shadowed classified key across scopes", src: `package p @@ -777,7 +820,7 @@ var _ = 1 if !ok { continue } - got[idx] = len(scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, cases[idx].name)) > 0 + got[idx] = len(scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, pkg.PkgPath, cases[idx].name)) > 0 checked[idx] = true } } From 175fa77d58d7f01e645c235d414ca4a5f69373ca Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 15:00:49 -0700 Subject: [PATCH 12/18] Guard raw attribute.KeyValue literals and method expressions Detect direct attribute.KeyValue{Key: , ...} construction and attribute.Key builder method expressions (called or captured), closing two bypasses in the telemetry raw-attribute guard. Add container.remotebuild to the Container Build feature-telemetry mapping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/cmd/telemetry_test.go | 270 +++++++++++++++++++++++-------- docs/reference/telemetry-data.md | 2 +- 2 files changed, 207 insertions(+), 65 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 039b085769a..e3f0e20ab00 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -391,6 +391,20 @@ func isRawAttributeKeyType(t types.Type) bool { obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "Key" } +// isRawAttributeKeyValueType reports whether t is exactly the +// go.opentelemetry.io/otel/attribute.KeyValue named struct. Constructing one +// directly with a key literal bypasses both the attribute constructors and the +// fields registry, so the guard inspects these composite literals too. +func isRawAttributeKeyValueType(t types.Type) bool { + named, ok := t.(*types.Named) + if !ok { + return false + } + obj := named.Obj() + return obj != nil && obj.Pkg() != nil && + obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "KeyValue" +} + // fieldsPkgPath is the import path of the package that defines the sanctioned // classified attribute wrapper, fields.AttributeKey. const fieldsPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" @@ -460,6 +474,16 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I return "..." } + // literalKeyExpr renders a constant key expression for a friendlier message, + // preferring its resolved constant string value (covers both "k" and + // attribute.Key("k") forms), and falling back to "...". + literalKeyExpr := func(expr ast.Expr) string { + if tv, ok := info.Types[expr]; ok && tv.Value != nil { + return tv.Value.String() + } + return "..." + } + // isRawAttributeConstructorFunc reports whether obj is a package-level function // of the attribute package that builds a KeyValue from a key and value (e.g. // attribute.String). Resolving via the types object makes this independent of @@ -497,74 +521,145 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "KeyValue" } + // constantKey reports whether expr evaluates to a compile-time constant. A + // KeyValue whose Key is a constant introduces a fixed key literal, whereas a + // non-constant Key (a variable, a k.Key field access, a function result) + // merely forwards an existing key. This is what distinguishes a new raw key + // from the fields/baggage plumbing that only re-emits caller-supplied keys. + constantKey := func(expr ast.Expr) bool { + tv, ok := info.Types[expr] + return ok && tv.Value != nil + } + + // keyValueLiteralKey returns the expression assigned to the Key field of an + // attribute.KeyValue composite literal, handling both keyed + // (KeyValue{Key: ...}) and positional (KeyValue{k, v}) forms. It returns nil + // when no key element is present (the zero value). + keyValueLiteralKey := func(cl *ast.CompositeLit) ast.Expr { + if len(cl.Elts) == 0 { + return nil + } + if _, keyed := cl.Elts[0].(*ast.KeyValueExpr); keyed { + for _, elt := range cl.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + if id, ok := kv.Key.(*ast.Ident); ok && id.Name == "Key" { + return kv.Value + } + } + return nil + } + // Positional: Key is the first field of attribute.KeyValue. + return cl.Elts[0] + } + + // rawAttributeBuilderSelection reports whether sel selects a KeyValue-producing + // builder (String/Bool/…) defined on the raw attribute.Key type, on a receiver + // that is NOT the classified fields.AttributeKey. It matches both a method + // value (recvExpr.String) and a method expression (attribute.Key.String) so the + // guard also covers a call written in method-expression form and a builder + // captured as a function value (builder := attribute.Key.String). A method + // value that only re-emits an existing KeyValue's key (kv.Key.String) is not a + // new key and returns false. + rawAttributeBuilderSelection := func(sel *ast.SelectorExpr) bool { + selection := info.Selections[sel] + if selection == nil { + return false + } + kind := selection.Kind() + if kind != types.MethodVal && kind != types.MethodExpr { + return false + } + m, ok := selection.Obj().(*types.Func) + if !ok { + return false + } + if _, isCtor := rawAttributeConstructors[m.Name()]; !isCtor || !isAttributeKeyBuilderMethod(m) { + return false + } + if isFieldsAttributeKeyType(selection.Recv()) { + return false + } + if kind == types.MethodVal && isReemittedKeyValueKey(sel.X) { + return false + } + return true + } + var violations []string ast.Inspect(file, func(n ast.Node) bool { - // Constructing a fields.AttributeKey outside the fields package fabricates a - // key the classifier never sees. The GDPR scanner discovers only the - // exported package-level AttributeKey vars declared in the fields package, - // so a locally built one — e.g. fields.AttributeKey{Key: attribute.Key("x")} - // — carries an uncatalogued key even though its type is fields.AttributeKey, - // and the emission branch below would (correctly, to keep registered keys - // passed through parameters valid) treat that receiver type as sanctioned. - // The only sanctioned ways to obtain an AttributeKey in product code are to - // reference a registered fields.* var or to call fields.ExtensionUsageAttribute; - // both live in the fields package, which is why that package is exempt here. - if cl, ok := n.(*ast.CompositeLit); ok { - if pkgPath != fieldsPkgPath && isFieldsAttributeKeyType(info.TypeOf(cl)) { - pos := fset.Position(cl.Pos()) + switch node := n.(type) { + case *ast.CompositeLit: + pos := fset.Position(node.Pos()) + clType := info.TypeOf(node) + // Constructing a fields.AttributeKey outside the fields package + // fabricates a key the classifier never sees. The GDPR scanner discovers + // only the exported package-level AttributeKey vars declared in the + // fields package, so a locally built one — e.g. + // fields.AttributeKey{Key: attribute.Key("x")} — carries an uncatalogued + // key even though its type is fields.AttributeKey, and the method branch + // below would (correctly, to keep registered keys passed through + // parameters valid) treat that receiver type as sanctioned. The only + // sanctioned ways to obtain an AttributeKey in product code are to + // reference a registered fields.* var or to call + // fields.ExtensionUsageAttribute; both live in the fields package, which + // is why that package is exempt here. + if pkgPath != fieldsPkgPath && isFieldsAttributeKeyType(clType) { violations = append(violations, fmt.Sprintf( " %s:%d: fields.AttributeKey{...} constructed outside the fields "+ "package (its key is not in the classifier catalog; reference a "+ "registered fields.* key or fields.ExtensionUsageAttribute)", rel, pos.Line)) + return true } - return true - } - call, ok := n.(*ast.CallExpr) - if !ok { - return true - } - pos := fset.Position(call.Pos()) - - switch fun := call.Fun.(type) { - case *ast.SelectorExpr: - // A method-value selection (x.String(v)) — including a method promoted - // through embedding — resolves both the method's defining type and the - // receiver expression's type. A KeyValue-producing attribute.Key method - // is sanctioned only when the receiver is the classified - // fields.AttributeKey wrapper (the sole type the metadata classifier - // discovers). Called on a bare attribute.Key, or on any other struct - // that merely embeds attribute.Key, it produces an unclassified key and - // is a violation. This single check covers keys held in parameters, - // locals, struct fields, and function results, as well as the chained - // attribute.Key("k").String(v) form. Re-emitting an existing KeyValue's - // key (kv.Key.String(v)) is excluded because the key was already checked - // where the KeyValue was built. - if sel := info.Selections[fun]; sel != nil && sel.Kind() == types.MethodVal { - if m, ok := sel.Obj().(*types.Func); ok { - if _, isCtor := rawAttributeConstructors[m.Name()]; isCtor && - isAttributeKeyBuilderMethod(m) && - !isFieldsAttributeKeyType(sel.Recv()) && !isReemittedKeyValueKey(fun.X) { - violations = append(violations, fmt.Sprintf( - " %s:%d: .%s(...) produces an unclassified key "+ - "(receiver is not fields.AttributeKey)", rel, pos.Line, m.Name())) - } + // A raw attribute.KeyValue struct literal that sets Key to a constant + // introduces an unclassified key literal directly (e.g. + // attribute.KeyValue{Key: attribute.Key("raw.key"), Value: ...}), which + // span.SetAttributes would emit without ever passing through a + // fields.AttributeKey. The plumbing that legitimately builds KeyValues + // (fields.StringHashed and baggage re-emission) sets Key from an existing + // key expression (k.Key, a ranged variable), which is not constant and is + // therefore left alone. + if isRawAttributeKeyValueType(clType) { + if keyExpr := keyValueLiteralKey(node); keyExpr != nil && constantKey(keyExpr) { + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.KeyValue{Key: %s, ...} introduces an unclassified "+ + "key literal (build it from a fields.AttributeKey instead)", + rel, pos.Line, literalKeyExpr(keyExpr))) } - return true } - // Otherwise the selector is a qualified identifier for a package-level - // constructor, e.g. attribute.String("k", v) (possibly via an alias). - if isRawAttributeConstructorFunc(info.Uses[fun.Sel]) { + return true + case *ast.SelectorExpr: + if rawAttributeBuilderSelection(node) { + pos := fset.Position(node.Pos()) violations = append(violations, fmt.Sprintf( - " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, fun.Sel.Name, literalKey(call))) + " %s:%d: attribute.Key builder .%s used on a non-fields.AttributeKey "+ + "receiver (produces an unclassified key)", rel, pos.Line, node.Sel.Name)) } - case *ast.Ident: - // A bare identifier call resolves to a package-level constructor only - // when the attribute package is dot-imported, e.g. String("k", v). - if isRawAttributeConstructorFunc(info.Uses[fun]) { - violations = append(violations, fmt.Sprintf( - " %s:%d: %s(%s, ...) (dot-imported attribute constructor)", - rel, pos.Line, fun.Name, literalKey(call))) + return true + case *ast.CallExpr: + pos := fset.Position(node.Pos()) + switch fun := node.Fun.(type) { + case *ast.SelectorExpr: + // A package-qualified constructor call, e.g. attribute.String("k", v) + // (possibly via an alias). Method selections on a value/type are + // handled by the *ast.SelectorExpr case above, so this only fires for + // package functions (info.Uses resolves the aliased import too). + if isRawAttributeConstructorFunc(info.Uses[fun.Sel]) { + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, fun.Sel.Name, literalKey(node))) + } + case *ast.Ident: + // A bare identifier call resolves to a package-level constructor only + // when the attribute package is dot-imported, e.g. String("k", v). + if isRawAttributeConstructorFunc(info.Uses[fun]) { + violations = append(violations, fmt.Sprintf( + " %s:%d: %s(%s, ...) (dot-imported attribute constructor)", + rel, pos.Line, fun.Name, literalKey(node))) + } } + return true } return true }) @@ -575,14 +670,18 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // TestRawTelemetryAttributeScanner is a fixture test for the type-aware guard // used by TestNoRawTelemetryAttributes. It pins the contract: raw attribute // constructors (default, aliased, and dot-imported), constant keys, the chained -// attribute.Key(k).String(v) form, and KeyValue-producing method calls on a raw -// attribute.Key value reached through a parameter, a local, a struct field, or a -// function result are all flagged; while the sanctioned promoted-method call on a -// struct that embeds attribute.Key, a bare attribute.Key(k) conversion (which -// does not build a KeyValue), and non-KeyValue uses of a key (e.g. a map lookup) -// are not. Fixtures are type-checked against the real attribute package via -// go/packages, so the guard runs with the same type information it uses on the -// module. +// attribute.Key(k).String(v) form, KeyValue-producing attribute.Key builders +// reached as a method value (through a parameter, local, struct field, or +// function result) or as a method expression (attribute.Key.String, including +// when captured as a function value), a locally constructed fields.AttributeKey, +// and a raw attribute.KeyValue literal that introduces a constant key are all +// flagged; while the sanctioned promoted-method call on the classified +// fields.AttributeKey, fields.ExtensionUsageAttribute, an attribute.KeyValue that +// only re-emits an existing (non-constant) key, a bare attribute.Key(k) +// conversion (which does not build a KeyValue), and non-KeyValue uses of a key +// (e.g. a map lookup) are not. Fixtures are type-checked against the real +// attribute and fields packages via go/packages, so the guard runs with the same +// type information it uses on the module. func TestRawTelemetryAttributeScanner(t *testing.T) { t.Parallel() @@ -640,6 +739,49 @@ var _ = attribute.IntSlice("raw.key", []int{1}) src: `package p import "go.opentelemetry.io/otel/attribute" var _ = attribute.Key("raw.key").String("v") +`, + wantViolation: true, + }, + { + name: "raw attribute.KeyValue struct literal", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.KeyValue{Key: attribute.Key("raw.key"), Value: attribute.StringValue("v")} +`, + wantViolation: true, + }, + { + name: "raw attribute.KeyValue positional literal", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.KeyValue{attribute.Key("raw.key"), attribute.StringValue("v")} +`, + wantViolation: true, + }, + { + name: "attribute.KeyValue re-emitting an existing key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f(k attribute.Key, v attribute.Value) attribute.KeyValue { + return attribute.KeyValue{Key: k, Value: v} +} +`, + wantViolation: false, + }, + { + name: "attribute.Key builder method expression call", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var _ = attribute.Key.String(attribute.Key("raw.key"), "v") +`, + wantViolation: true, + }, + { + name: "attribute.Key builder captured as function value", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var builder = attribute.Key.String +var _ = builder(attribute.Key("raw.key"), "v") `, wantViolation: true, }, diff --git a/docs/reference/telemetry-data.md b/docs/reference/telemetry-data.md index 04d23943dad..520ee6fc0ac 100644 --- a/docs/reference/telemetry-data.md +++ b/docs/reference/telemetry-data.md @@ -795,7 +795,7 @@ How to find telemetry for a given feature area. Start here if you know the featu | **Execution Environment** | All events | `execution.environment` | Usage by environment, CI vs local | | **Self-Update** | `cmd.update` | `update.installMethod`, `update.fromVersion` | Update adoption | | **Hooks** | `hooks.exec` | `hooks.name`, `hooks.type`, `hooks.kind` | Hook usage by type | -| **Container Build** | `container.publish`, `container.remotebuild`, `tools.pack.build` | `pack.builder.image` | Build method usage, success rates | +| **Container Build** | `container.publish`, `container.remotebuild`, `tools.pack.build` | `pack.builder.image`, `container.remotebuild` | Build method usage (local vs. remote ACR build), success rates | | **App Detection (Aspire polyglot)** | `aspire.apphost.unsupported` | `aspire.apphost.language` (`typescript`/`python`/`go`/`java`/`rust`) | How often an unsupported Aspire polyglot (non-C#) AppHost is encountered, by language. **Emitted only during app detection for `init` and fresh `up` (no existing `azure.yaml`)** — not for already-initialized projects, so absence does not mean zero unsupported AppHosts. | | **Tool Management (`azd tool`)** | `cmd.tool.install`, `cmd.tool.update`, `cmd.tool.uninstall`, `cmd.tool.check` | `tool.id`, `tool.install.strategy` | Install/update/uninstall success, update availability | From cfcf4de0d8389fe2c9fc48bbde3d56e93e809a29 Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 15:52:38 -0700 Subject: [PATCH 13/18] Guard fabricated KeyValue keys and captured attribute constructors Address review 4954993193: - Flag attribute.KeyValue literals whose Key is fabricated via a run-time attribute.Key(x) conversion, not just a compile-time constant. - Detect raw attribute constructors captured as function values (builder := attribute.String) by resolving the identifier object instead of only inspecting call positions. - Record the container.remotebuild attribute from the build method actually used: correct it to false when a remote build fails and falls back to a local build, matching the field's documented meaning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/cmd/telemetry_test.go | 115 +++++++++++++++--------- cli/azd/pkg/project/container_helper.go | 14 ++- 2 files changed, 81 insertions(+), 48 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index e3f0e20ab00..e727ab477aa 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -463,17 +463,6 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I "Int64Slice": {}, "Float64Slice": {}, } - // literalKey renders the first string-literal argument of a call for a - // friendlier message, or "..." for a non-literal (e.g. a named constant). - literalKey := func(c *ast.CallExpr) string { - if len(c.Args) > 0 { - if lit, ok := c.Args[0].(*ast.BasicLit); ok && lit.Kind == token.STRING { - return lit.Value - } - } - return "..." - } - // literalKeyExpr renders a constant key expression for a friendlier message, // preferring its resolved constant string value (covers both "k" and // attribute.Key("k") forms), and falling back to "...". @@ -531,6 +520,28 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I return ok && tv.Value != nil } + // fabricatesRawKey reports whether the Key expression of an attribute.KeyValue + // literal mints a new key rather than forwarding an existing one. A + // compile-time constant (a "literal" or attribute.Key("literal")) is a fixed + // key literal. An explicit attribute.Key(x) conversion also fabricates a key — + // even when x is only known at run time — because it turns an arbitrary string + // into a key the classifier never sees; the sole sanctioned way to build a + // dynamic key is fields.ExtensionUsageAttribute. Forwarding forms (a bare + // attribute.Key variable, or a k.Key field access) are neither constant nor a + // conversion, so the fields/baggage plumbing that re-emits caller-supplied keys + // is left alone. + fabricatesRawKey := func(expr ast.Expr) bool { + if constantKey(expr) { + return true + } + call, ok := expr.(*ast.CallExpr) + if !ok { + return false + } + tv, ok := info.Types[call.Fun] + return ok && tv.IsType() && isRawAttributeKeyType(tv.Type) + } + // keyValueLiteralKey returns the expression assigned to the Key field of an // attribute.KeyValue composite literal, handling both keyed // (KeyValue{Key: ...}) and positional (KeyValue{k, v}) forms. It returns nil @@ -613,16 +624,17 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I "registered fields.* key or fields.ExtensionUsageAttribute)", rel, pos.Line)) return true } - // A raw attribute.KeyValue struct literal that sets Key to a constant - // introduces an unclassified key literal directly (e.g. - // attribute.KeyValue{Key: attribute.Key("raw.key"), Value: ...}), which + // A raw attribute.KeyValue struct literal whose Key is fabricated + // introduces an unclassified key directly (e.g. + // attribute.KeyValue{Key: attribute.Key("raw.key"), Value: ...}, or the + // same with a run-time attribute.Key(x) conversion), which // span.SetAttributes would emit without ever passing through a // fields.AttributeKey. The plumbing that legitimately builds KeyValues // (fields.StringHashed and baggage re-emission) sets Key from an existing - // key expression (k.Key, a ranged variable), which is not constant and is - // therefore left alone. + // key expression (k.Key, a ranged variable), which is neither a constant + // nor a conversion and is therefore left alone. if isRawAttributeKeyValueType(clType) { - if keyExpr := keyValueLiteralKey(node); keyExpr != nil && constantKey(keyExpr) { + if keyExpr := keyValueLiteralKey(node); keyExpr != nil && fabricatesRawKey(keyExpr) { violations = append(violations, fmt.Sprintf( " %s:%d: attribute.KeyValue{Key: %s, ...} introduces an unclassified "+ "key literal (build it from a fields.AttributeKey instead)", @@ -638,26 +650,21 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I "receiver (produces an unclassified key)", rel, pos.Line, node.Sel.Name)) } return true - case *ast.CallExpr: - pos := fset.Position(node.Pos()) - switch fun := node.Fun.(type) { - case *ast.SelectorExpr: - // A package-qualified constructor call, e.g. attribute.String("k", v) - // (possibly via an alias). Method selections on a value/type are - // handled by the *ast.SelectorExpr case above, so this only fires for - // package functions (info.Uses resolves the aliased import too). - if isRawAttributeConstructorFunc(info.Uses[fun.Sel]) { - violations = append(violations, fmt.Sprintf( - " %s:%d: attribute.%s(%s, ...)", rel, pos.Line, fun.Sel.Name, literalKey(node))) - } - case *ast.Ident: - // A bare identifier call resolves to a package-level constructor only - // when the attribute package is dot-imported, e.g. String("k", v). - if isRawAttributeConstructorFunc(info.Uses[fun]) { - violations = append(violations, fmt.Sprintf( - " %s:%d: %s(%s, ...) (dot-imported attribute constructor)", - rel, pos.Line, fun.Name, literalKey(node))) - } + case *ast.Ident: + // A reference to a package-level attribute constructor — attribute.String, + // an aliased import of it, or a dot-imported String — bypasses the + // fields.AttributeKey registry. Resolving the identifier's object flags + // every form: the selector's Sel in attribute.String(...), a bare + // dot-imported String(...), and, crucially, the constructor captured as a + // function value (builder := attribute.String; builder("raw.key", v)), + // which a call-position-only check would miss. attribute.Key's methods are + // handled by the *ast.SelectorExpr case above and are excluded here because + // isRawAttributeConstructorFunc rejects any func with a receiver. + if isRawAttributeConstructorFunc(info.Uses[node]) { + pos := fset.Position(node.Pos()) + violations = append(violations, fmt.Sprintf( + " %s:%d: %s is a raw attribute constructor (build telemetry from a "+ + "fields.AttributeKey instead)", rel, pos.Line, node.Name)) } return true } @@ -669,15 +676,16 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // TestRawTelemetryAttributeScanner is a fixture test for the type-aware guard // used by TestNoRawTelemetryAttributes. It pins the contract: raw attribute -// constructors (default, aliased, and dot-imported), constant keys, the chained -// attribute.Key(k).String(v) form, KeyValue-producing attribute.Key builders -// reached as a method value (through a parameter, local, struct field, or -// function result) or as a method expression (attribute.Key.String, including -// when captured as a function value), a locally constructed fields.AttributeKey, -// and a raw attribute.KeyValue literal that introduces a constant key are all +// constructors (default, aliased, dot-imported, and captured as a function +// value), constant keys, the chained attribute.Key(k).String(v) form, KeyValue- +// producing attribute.Key builders reached as a method value (through a +// parameter, local, struct field, or function result) or as a method expression +// (attribute.Key.String, including when captured as a function value), a locally +// constructed fields.AttributeKey, and a raw attribute.KeyValue literal whose Key +// is fabricated (a constant, or a run-time attribute.Key(x) conversion) are all // flagged; while the sanctioned promoted-method call on the classified // fields.AttributeKey, fields.ExtensionUsageAttribute, an attribute.KeyValue that -// only re-emits an existing (non-constant) key, a bare attribute.Key(k) +// only re-emits an existing (forwarded) key, a bare attribute.Key(k) // conversion (which does not build a KeyValue), and non-KeyValue uses of a key // (e.g. a map lookup) are not. Fixtures are type-checked against the real // attribute and fields packages via go/packages, so the guard runs with the same @@ -723,6 +731,15 @@ var _ = otelattr.Bool("raw.key", true) src: `package p import . "go.opentelemetry.io/otel/attribute" var _ = String("raw.key", "v") +`, + wantViolation: true, + }, + { + name: "constructor captured as function value", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var builder = attribute.String +var _ = builder("raw.key", "v") `, wantViolation: true, }, @@ -755,6 +772,16 @@ var _ = attribute.KeyValue{Key: attribute.Key("raw.key"), Value: attribute.Strin src: `package p import "go.opentelemetry.io/otel/attribute" var _ = attribute.KeyValue{attribute.Key("raw.key"), attribute.StringValue("v")} +`, + wantViolation: true, + }, + { + name: "attribute.KeyValue with run-time key conversion", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f(runtimeKey string) attribute.KeyValue { + return attribute.KeyValue{Key: attribute.Key(runtimeKey), Value: attribute.StringValue("v")} +} `, wantViolation: true, }, diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 3006121f9aa..95085ca68cb 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -622,10 +622,15 @@ func (ch *ContainerHelper) Publish( options *PublishOptions, ) (_ *ServicePublishResult, err error) { ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) - defer func() { span.EndWithStatus(err) }() - span.SetAttributes( - fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild), - ) + // Record whether the image was actually built remotely. It starts from the + // configured preference and is corrected below when a remote build fails and + // we fall back to a local build, so the attribute reflects the method used + // rather than the one requested (matching the field's documented meaning). + remoteBuildUsed := serviceConfig.Docker.RemoteBuild + defer func() { + span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(remoteBuildUsed)) + span.EndWithStatus(err) + }() var remoteImage string @@ -650,6 +655,7 @@ func (ch *ContainerHelper) Publish( "Remote build failed: %s\nFalling back to local Docker build.", err), HidePrefix: false, }) + remoteBuildUsed = false remoteImage, err = ch.publishLocalImage( ctx, serviceConfig, serviceContext, env, progress, imageOverride) } From 7c1e5c7b625ccf3d9ecd51071066c2d5e40e7bcf Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 16:44:14 -0700 Subject: [PATCH 14/18] address feedback --- cli/azd/cmd/telemetry_test.go | 237 ++++++++++++------------ cli/azd/pkg/project/container_helper.go | 14 +- 2 files changed, 123 insertions(+), 128 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index e727ab477aa..77fcccdb748 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -409,6 +409,11 @@ func isRawAttributeKeyValueType(t types.Type) bool { // classified attribute wrapper, fields.AttributeKey. const fieldsPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +// baggagePkgPath is the import path of the telemetry baggage package, which +// legitimately rebuilds caller-supplied attribute.KeyValue structs (re-emitting +// keys that were already subject to this guard at their original build site). +const baggagePkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/baggage" + // isFieldsAttributeKeyType reports whether t is exactly the sanctioned // fields.AttributeKey named type. That wrapper is the only type the metadata // classifier discovers and reads Classification/Purpose/Endpoint from, so it is @@ -443,13 +448,14 @@ func isAttributeKeyBuilderMethod(m *types.Func) bool { // scanFileForRawAttributes returns the raw-telemetry-attribute violations in a // single type-checked Go file. info must be the go/types information for the // file's package; pkgPath is that package's import path (used to exempt the -// fields package from the construction rule below); rel is the display path used -// in messages. Classifying calls by the resolved type of the callee and receiver -// — rather than by syntactic shape — makes the guard sound across import aliases, -// dot imports, and keys reached through parameters, struct fields, or function -// results. It is shared by TestNoRawTelemetryAttributes (which walks the module) -// and the fixture test TestRawTelemetryAttributeScanner, so the guard's contract -// is itself tested. +// fields package's registry/factory from the fields.AttributeKey construction +// rule, and the fields/baggage plumbing from the attribute.KeyValue construction +// rule); rel is the display path used in messages. Classifying calls by the +// resolved type of the callee and receiver — rather than by syntactic shape — +// makes the guard sound across import aliases, dot imports, and keys reached +// through parameters, struct fields, or function results. It is shared by +// TestNoRawTelemetryAttributes (which walks the module) and the fixture test +// TestRawTelemetryAttributeScanner, so the guard's contract is itself tested. func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.Info, pkgPath, rel string) []string { // rawAttributeConstructors are the go.opentelemetry.io/otel/attribute helpers // (and the identically named attribute.Key methods) that build a KeyValue from @@ -463,16 +469,6 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I "Int64Slice": {}, "Float64Slice": {}, } - // literalKeyExpr renders a constant key expression for a friendlier message, - // preferring its resolved constant string value (covers both "k" and - // attribute.Key("k") forms), and falling back to "...". - literalKeyExpr := func(expr ast.Expr) string { - if tv, ok := info.Types[expr]; ok && tv.Value != nil { - return tv.Value.String() - } - return "..." - } - // isRawAttributeConstructorFunc reports whether obj is a package-level function // of the attribute package that builds a KeyValue from a key and value (e.g. // attribute.String). Resolving via the types object makes this independent of @@ -510,62 +506,6 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "KeyValue" } - // constantKey reports whether expr evaluates to a compile-time constant. A - // KeyValue whose Key is a constant introduces a fixed key literal, whereas a - // non-constant Key (a variable, a k.Key field access, a function result) - // merely forwards an existing key. This is what distinguishes a new raw key - // from the fields/baggage plumbing that only re-emits caller-supplied keys. - constantKey := func(expr ast.Expr) bool { - tv, ok := info.Types[expr] - return ok && tv.Value != nil - } - - // fabricatesRawKey reports whether the Key expression of an attribute.KeyValue - // literal mints a new key rather than forwarding an existing one. A - // compile-time constant (a "literal" or attribute.Key("literal")) is a fixed - // key literal. An explicit attribute.Key(x) conversion also fabricates a key — - // even when x is only known at run time — because it turns an arbitrary string - // into a key the classifier never sees; the sole sanctioned way to build a - // dynamic key is fields.ExtensionUsageAttribute. Forwarding forms (a bare - // attribute.Key variable, or a k.Key field access) are neither constant nor a - // conversion, so the fields/baggage plumbing that re-emits caller-supplied keys - // is left alone. - fabricatesRawKey := func(expr ast.Expr) bool { - if constantKey(expr) { - return true - } - call, ok := expr.(*ast.CallExpr) - if !ok { - return false - } - tv, ok := info.Types[call.Fun] - return ok && tv.IsType() && isRawAttributeKeyType(tv.Type) - } - - // keyValueLiteralKey returns the expression assigned to the Key field of an - // attribute.KeyValue composite literal, handling both keyed - // (KeyValue{Key: ...}) and positional (KeyValue{k, v}) forms. It returns nil - // when no key element is present (the zero value). - keyValueLiteralKey := func(cl *ast.CompositeLit) ast.Expr { - if len(cl.Elts) == 0 { - return nil - } - if _, keyed := cl.Elts[0].(*ast.KeyValueExpr); keyed { - for _, elt := range cl.Elts { - kv, ok := elt.(*ast.KeyValueExpr) - if !ok { - continue - } - if id, ok := kv.Key.(*ast.Ident); ok && id.Name == "Key" { - return kv.Value - } - } - return nil - } - // Positional: Key is the first field of attribute.KeyValue. - return cl.Elts[0] - } - // rawAttributeBuilderSelection reports whether sel selects a KeyValue-producing // builder (String/Bool/…) defined on the raw attribute.Key type, on a receiver // that is NOT the classified fields.AttributeKey. It matches both a method @@ -599,47 +539,96 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I return true } + // sanctionedFieldsKeyLit collects the fields.AttributeKey composite literals in + // this file that are legitimate inside the fields package: the exported + // package-level var initializers that make up the registry the GDPR classifier + // scans, and the literal returned by the ExtensionUsageAttribute factory (the + // one sanctioned source of a dynamic key). It is only populated for the fields + // package; a function-local or unexported fields.AttributeKey built anywhere + // else in that package would carry a key the classifier never discovers while + // its sanctioned receiver type would let the method branch accept emissions + // through it, so those are flagged. + sanctionedFieldsKeyLit := map[ast.Node]bool{} + if pkgPath == fieldsPkgPath { + for _, decl := range file.Decls { + switch d := decl.(type) { + case *ast.GenDecl: + if d.Tok != token.VAR { + continue + } + for _, spec := range d.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + for i, name := range vs.Names { + if name.IsExported() && i < len(vs.Values) { + sanctionedFieldsKeyLit[vs.Values[i]] = true + } + } + } + case *ast.FuncDecl: + if d.Name.Name == "ExtensionUsageAttribute" && d.Body != nil { + ast.Inspect(d.Body, func(n ast.Node) bool { + if cl, ok := n.(*ast.CompositeLit); ok { + sanctionedFieldsKeyLit[cl] = true + } + return true + }) + } + } + } + } + + // isKeyValuePlumbingPkgPath reports whether p is one of the sanctioned + // telemetry-plumbing packages allowed to build attribute.KeyValue structs + // directly: fields (StringHashed forwards a classified key's .Key) and baggage + // (re-emits caller-supplied keys ranged from an existing KeyValue set). Every + // other package must emit through a fields.AttributeKey method instead. + isKeyValuePlumbingPkgPath := func(p string) bool { + return p == fieldsPkgPath || p == baggagePkgPath + } + var violations []string ast.Inspect(file, func(n ast.Node) bool { switch node := n.(type) { case *ast.CompositeLit: pos := fset.Position(node.Pos()) clType := info.TypeOf(node) - // Constructing a fields.AttributeKey outside the fields package - // fabricates a key the classifier never sees. The GDPR scanner discovers - // only the exported package-level AttributeKey vars declared in the - // fields package, so a locally built one — e.g. - // fields.AttributeKey{Key: attribute.Key("x")} — carries an uncatalogued - // key even though its type is fields.AttributeKey, and the method branch - // below would (correctly, to keep registered keys passed through - // parameters valid) treat that receiver type as sanctioned. The only - // sanctioned ways to obtain an AttributeKey in product code are to - // reference a registered fields.* var or to call - // fields.ExtensionUsageAttribute; both live in the fields package, which - // is why that package is exempt here. - if pkgPath != fieldsPkgPath && isFieldsAttributeKeyType(clType) { - violations = append(violations, fmt.Sprintf( - " %s:%d: fields.AttributeKey{...} constructed outside the fields "+ - "package (its key is not in the classifier catalog; reference a "+ - "registered fields.* key or fields.ExtensionUsageAttribute)", rel, pos.Line)) - return true - } - // A raw attribute.KeyValue struct literal whose Key is fabricated - // introduces an unclassified key directly (e.g. - // attribute.KeyValue{Key: attribute.Key("raw.key"), Value: ...}, or the - // same with a run-time attribute.Key(x) conversion), which - // span.SetAttributes would emit without ever passing through a - // fields.AttributeKey. The plumbing that legitimately builds KeyValues - // (fields.StringHashed and baggage re-emission) sets Key from an existing - // key expression (k.Key, a ranged variable), which is neither a constant - // nor a conversion and is therefore left alone. - if isRawAttributeKeyValueType(clType) { - if keyExpr := keyValueLiteralKey(node); keyExpr != nil && fabricatesRawKey(keyExpr) { + // fields.AttributeKey{...} wrapper construction. Outside the fields + // package this always fabricates a key the classifier never sees (it + // discovers only the exported package-level AttributeKey vars declared in + // the fields package). Inside the fields package it is allowed only for + // the registry itself — an exported package-level var initializer — or the + // sanctioned dynamic factory ExtensionUsageAttribute; anything else builds + // an uncatalogued key whose fields.AttributeKey type would nonetheless let + // the method branch below accept emissions through it. + if isFieldsAttributeKeyType(clType) { + if pkgPath != fieldsPkgPath { + violations = append(violations, fmt.Sprintf( + " %s:%d: fields.AttributeKey{...} constructed outside the fields "+ + "package (its key is not in the classifier catalog; reference a "+ + "registered fields.* key or fields.ExtensionUsageAttribute)", rel, pos.Line)) + } else if !sanctionedFieldsKeyLit[node] { violations = append(violations, fmt.Sprintf( - " %s:%d: attribute.KeyValue{Key: %s, ...} introduces an unclassified "+ - "key literal (build it from a fields.AttributeKey instead)", - rel, pos.Line, literalKeyExpr(keyExpr))) + " %s:%d: fields.AttributeKey{...} built in the fields package outside an "+ + "exported package-level var or ExtensionUsageAttribute (the classifier "+ + "discovers only exported package-level keys)", rel, pos.Line)) } + return true + } + // attribute.KeyValue{...} raw struct construction. Building the struct + // directly bypasses the fields.AttributeKey methods entirely and lets any + // key expression through — including a variable initialized from + // attribute.Key("raw.key"), which no key-shape check can catch. It is + // allowed only in the sanctioned plumbing packages that re-emit + // caller-supplied keys (fields.StringHashed and baggage), identified by + // package path; everywhere else it is a violation regardless of how the + // key is spelled. + if isRawAttributeKeyValueType(clType) && !isKeyValuePlumbingPkgPath(pkgPath) { + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.KeyValue{...} constructed outside the telemetry "+ + "plumbing (build it from a fields.AttributeKey instead)", rel, pos.Line)) } return true case *ast.SelectorExpr: @@ -680,16 +669,19 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // value), constant keys, the chained attribute.Key(k).String(v) form, KeyValue- // producing attribute.Key builders reached as a method value (through a // parameter, local, struct field, or function result) or as a method expression -// (attribute.Key.String, including when captured as a function value), a locally -// constructed fields.AttributeKey, and a raw attribute.KeyValue literal whose Key -// is fabricated (a constant, or a run-time attribute.Key(x) conversion) are all -// flagged; while the sanctioned promoted-method call on the classified -// fields.AttributeKey, fields.ExtensionUsageAttribute, an attribute.KeyValue that -// only re-emits an existing (forwarded) key, a bare attribute.Key(k) -// conversion (which does not build a KeyValue), and non-KeyValue uses of a key -// (e.g. a map lookup) are not. Fixtures are type-checked against the real -// attribute and fields packages via go/packages, so the guard runs with the same -// type information it uses on the module. +// (attribute.Key.String, including when captured as a function value), a +// fields.AttributeKey constructed outside the fields package, and any raw +// attribute.KeyValue struct built outside the sanctioned plumbing packages +// (whatever its key — a literal, a run-time attribute.Key(x) conversion, or a +// variable forwarding one) are all flagged; while the sanctioned promoted-method +// call on the classified fields.AttributeKey, fields.ExtensionUsageAttribute, a +// bare attribute.Key(k) conversion (which does not build a KeyValue), and +// non-KeyValue uses of a key (e.g. a map lookup) are not. The in-package +// exemptions — the fields registry vars, ExtensionUsageAttribute, and the +// fields/baggage KeyValue plumbing — are keyed on package path and so are +// exercised by the module walk rather than these package-p fixtures. Fixtures are +// type-checked against the real attribute and fields packages via go/packages, so +// the guard runs with the same type information it uses on the module. func TestRawTelemetryAttributeScanner(t *testing.T) { t.Parallel() @@ -786,14 +778,23 @@ func f(runtimeKey string) attribute.KeyValue { wantViolation: true, }, { - name: "attribute.KeyValue re-emitting an existing key", + name: "attribute.KeyValue re-emitting a key outside the plumbing packages", src: `package p import "go.opentelemetry.io/otel/attribute" func f(k attribute.Key, v attribute.Value) attribute.KeyValue { return attribute.KeyValue{Key: k, Value: v} } `, - wantViolation: false, + wantViolation: true, + }, + { + name: "attribute.KeyValue with a key from a converted variable", + src: `package p +import "go.opentelemetry.io/otel/attribute" +var rawKey = attribute.Key("raw.key") +var _ = attribute.KeyValue{Key: rawKey, Value: attribute.StringValue("v")} +`, + wantViolation: true, }, { name: "attribute.Key builder method expression call", diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 95085ca68cb..3d4f0f63d78 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -622,15 +622,8 @@ func (ch *ContainerHelper) Publish( options *PublishOptions, ) (_ *ServicePublishResult, err error) { ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) - // Record whether the image was actually built remotely. It starts from the - // configured preference and is corrected below when a remote build fails and - // we fall back to a local build, so the attribute reflects the method used - // rather than the one requested (matching the field's documented meaning). - remoteBuildUsed := serviceConfig.Docker.RemoteBuild - defer func() { - span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(remoteBuildUsed)) - span.EndWithStatus(err) - }() + defer func() { span.EndWithStatus(err) }() + span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild)) var remoteImage string @@ -655,9 +648,10 @@ func (ch *ContainerHelper) Publish( "Remote build failed: %s\nFalling back to local Docker build.", err), HidePrefix: false, }) - remoteBuildUsed = false remoteImage, err = ch.publishLocalImage( ctx, serviceConfig, serviceContext, env, progress, imageOverride) + } else { + remoteBuildUsed = true } } else if useDotnetPublishForDockerBuild(serviceConfig) { remoteImage, err = ch.runDotnetPublish(ctx, serviceConfig, targetResource, env, progress) From 4ff6809dfcea86fd6dfd31a5895ebc1bed82ca5b Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 16:55:33 -0700 Subject: [PATCH 15/18] Fix undefined remoteBuildUsed in container publish The previous commit kept the else branch that sets remoteBuildUsed = true but reverted the variable's declaration, leaving it undefined and breaking the build (go-fix, golangci-lint typecheck, and magefile-tests all failed). Restore the intended semantics: initialize remoteBuildUsed to false and set it true only after a remote build succeeds, so early returns and a local fallback report false, matching the container.remotebuild field documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/pkg/project/container_helper.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 3d4f0f63d78..5be33ca1792 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -622,8 +622,16 @@ func (ch *ContainerHelper) Publish( options *PublishOptions, ) (_ *ServicePublishResult, err error) { ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) - defer func() { span.EndWithStatus(err) }() - span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild)) + // Record whether the image was actually built remotely. It stays false until a + // remote build completes successfully, so early returns (invalid publish + // options, or a remote-build failure with no local runtime) and a fallback to a + // local build all report false — matching the field's documented meaning + // ("was built remotely") rather than the method requested. + remoteBuildUsed := false + defer func() { + span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(remoteBuildUsed)) + span.EndWithStatus(err) + }() var remoteImage string From 4928d080bbf475260aa2a2e3ca8abd8002331253 Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 17:03:48 -0700 Subject: [PATCH 16/18] Record configured remote-build preference for container.remotebuild Emit the user-supplied serviceConfig.Docker.RemoteBuild value directly, which already holds the requested true/false, instead of tracking the build method actually used. Update the field doc and telemetry docs to describe it as the requested/configured preference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/internal/tracing/fields/fields.go | 7 ++++--- cli/azd/pkg/project/container_helper.go | 18 ++++++------------ docs/reference/telemetry-data.md | 2 +- docs/specs/metrics-audit/telemetry-schema.md | 2 +- 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/cli/azd/internal/tracing/fields/fields.go b/cli/azd/internal/tracing/fields/fields.go index 52c38d769d0..45c70000f7a 100644 --- a/cli/azd/internal/tracing/fields/fields.go +++ b/cli/azd/internal/tracing/fields/fields.go @@ -1154,9 +1154,10 @@ var ( IsMeasurement: true, } - // ContainerRemoteBuildKey records whether the container image was - // built remotely (ACR build) rather than locally for a container publish. - // It is a boolean (fixed cardinality), so it is emitted raw (not hashed). + // ContainerRemoteBuildKey records the user-configured remote-build + // preference (serviceConfig.Docker.RemoteBuild) requested for a container + // publish — true when a remote (ACR) build was requested, false for a local + // build. It is a boolean (fixed cardinality), so it is emitted raw (not hashed). ContainerRemoteBuildKey = AttributeKey{ Key: attribute.Key("container.remotebuild"), Classification: SystemMetadata, diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index 5be33ca1792..52a583f86f1 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -622,16 +622,12 @@ func (ch *ContainerHelper) Publish( options *PublishOptions, ) (_ *ServicePublishResult, err error) { ctx, span := tracing.Start(ctx, events.ContainerPublishEvent) - // Record whether the image was actually built remotely. It stays false until a - // remote build completes successfully, so early returns (invalid publish - // options, or a remote-build failure with no local runtime) and a fallback to a - // local build all report false — matching the field's documented meaning - // ("was built remotely") rather than the method requested. - remoteBuildUsed := false - defer func() { - span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(remoteBuildUsed)) - span.EndWithStatus(err) - }() + defer func() { span.EndWithStatus(err) }() + // Record the user-configured remote-build preference (serviceConfig.Docker.RemoteBuild), + // which already carries the requested true/false value. + span.SetAttributes( + fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild), + ) var remoteImage string @@ -658,8 +654,6 @@ func (ch *ContainerHelper) Publish( }) remoteImage, err = ch.publishLocalImage( ctx, serviceConfig, serviceContext, env, progress, imageOverride) - } else { - remoteBuildUsed = true } } else if useDotnetPublishForDockerBuild(serviceConfig) { remoteImage, err = ch.runDotnetPublish(ctx, serviceConfig, targetResource, env, progress) diff --git a/docs/reference/telemetry-data.md b/docs/reference/telemetry-data.md index 520ee6fc0ac..63e8d079594 100644 --- a/docs/reference/telemetry-data.md +++ b/docs/reference/telemetry-data.md @@ -427,7 +427,7 @@ Emitted at provision start by the `microsoft.foundry` provisioning provider (the | Field Key | Type | Description | |-----------|------|-------------| | `container.remoteBuild.count` | measurement | Number of remote container builds performed | -| `container.remotebuild` | bool | Whether the image was built remotely (ACR) rather than locally. | +| `container.remotebuild` | bool | Whether a remote (ACR) build was requested (the configured preference) rather than a local build. |
diff --git a/docs/specs/metrics-audit/telemetry-schema.md b/docs/specs/metrics-audit/telemetry-schema.md index 8f63f01d3d6..51fe7cb368f 100644 --- a/docs/specs/metrics-audit/telemetry-schema.md +++ b/docs/specs/metrics-audit/telemetry-schema.md @@ -190,7 +190,7 @@ not emitted by azd spans. | Field | OTel Key | Classification | Purpose | Notes | |-------|----------|----------------|---------|-------| | Remote build count | `container.remoteBuild.count` | SystemMetadata | FeatureInsight | **Measurement** | -| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. Not hashed; not a measurement. | +| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether a remote (ACR) build was requested (the configured `Docker.RemoteBuild` preference) rather than a local build. Not hashed; not a measurement. | ### AKS From 3b14a72a5377685f4dd808adc5e5f96017e4bc95 Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 17:32:08 -0700 Subject: [PATCH 17/18] Normalize type aliases and scan all GOOS in telemetry guard Handle Go type aliases in the raw-telemetry-attribute guard by normalizing types with types.Unalias before the *types.Named identity assertions, so an aliased attribute.KeyValue/attribute.Key still trips the guard and an aliased fields.AttributeKey stays exempt. Add alias fixtures covering all three cases. Gate the KeyValue re-emit exemption (kv.Key.String) to the sanctioned telemetry plumbing packages (internal/tracing, fields, baggage); the same pattern in product code on a caller-supplied KeyValue is now flagged. Scan the module under every shipped GOOS (linux/windows/darwin) so a raw attribute in platform-specific product code cannot slip past a single-GOOS run. The host GOOS is scanned strictly; cross-compiled runs are best-effort to tolerate cgo/host-bound packages that cannot load off-platform. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/cmd/telemetry_test.go | 184 ++++++++++++++++++++++++++++------ 1 file changed, 151 insertions(+), 33 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 77fcccdb748..57eb6f454d0 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -8,7 +8,10 @@ import ( "go/ast" "go/token" "go/types" + "os" "path/filepath" + "runtime" + "sort" "strings" "testing" @@ -324,20 +327,90 @@ func TestNoRawTelemetryAttributes(t *testing.T) { azdRoot, err := filepath.Abs("..") require.NoError(t, err) + // The guard is a repository-wide invariant, but go/packages only loads the + // files that the active GOOS and build tags select. A raw attribute added to + // platform-specific product code (for example a *_windows.go file) would slip + // past a scan that runs under a single GOOS, because Linux CI never compiles + // the Windows-only files. Scan under every shipped GOOS so the invariant covers + // all code that ships. The host GOOS is scanned strictly: a load/type-check + // error fails the test, because a package that does not type-check can silently + // hide a violation. The cross-compiled GOOS runs are best-effort — packages + // that need a cross C toolchain (cgo, e.g. pkg/oneauth) or are otherwise + // host-bound can legitimately fail to load off their own platform, so their + // load errors are tolerated while every file that does load is still scanned. + goosSet := map[string]bool{"linux": true, "windows": true, "darwin": true} + goosSet[runtime.GOOS] = true + targets := make([]string, 0, len(goosSet)) + for goos := range goosSet { + targets = append(targets, goos) + } + sort.Strings(targets) + + seen := map[string]bool{} + var violations []string + for _, goos := range targets { + strict := goos == runtime.GOOS + found, loadErrors := scanModuleForRawAttributes(azdRoot, goos, strict) + + // A package that failed to type-check under the host GOOS would silently + // hide violations, so a load error there is a failure rather than a false + // pass. Cross-compiled runs tolerate load errors (see above). + if strict { + require.Empty(t, loadErrors, + "packages failed to load/type-check under GOOS=%s:\n%s", + goos, strings.Join(loadErrors, "\n")) + } + for _, v := range found { + if !seen[v] { + seen[v] = true + violations = append(violations, v) + } + } + } + sort.Strings(violations) + + if len(violations) > 0 { + t.Errorf( + "Found %d raw telemetry attribute(s) constructed directly.\n"+ + "Declare an exported fields.AttributeKey (with Classification and Purpose) in\n"+ + "internal/tracing/fields/fields.go and emit via it, e.g. fields.MyKey.String(v),\n"+ + "so the property is discoverable and classifiable by the GDPR metadata pipeline.\n\n"+ + "Raw attributes:\n%s", + len(violations), + strings.Join(violations, "\n"), + ) + } +} + +// scanModuleForRawAttributes loads the cli/azd module for the given GOOS and runs +// the raw-attribute guard over every file that resolves with type information. It +// returns the violation messages and, separately, any package load/type-check +// errors so the caller can decide whether they are fatal (host GOOS) or tolerated +// (cross-compiled GOOS). Cross-compiled runs disable cgo so a missing cross C +// toolchain does not abort the load; the affected packages surface as tolerated +// load errors instead. +func scanModuleForRawAttributes(azdRoot, goos string, strict bool) (violations, loadErrors []string) { + env := os.Environ() + env = append(env, "GOOS="+goos) + if !strict { + env = append(env, "CGO_ENABLED=0") + } + cfg := &packages.Config{ Mode: packages.NeedName | packages.NeedFiles | packages.NeedSyntax | packages.NeedTypes | packages.NeedTypesInfo | packages.NeedImports, Dir: azdRoot, Tests: false, + Env: env, } pkgs, err := packages.Load(cfg, "./...") - require.NoError(t, err) - require.NotEmpty(t, pkgs, "no packages loaded from the cli/azd module") + if err != nil { + return nil, []string{fmt.Sprintf(" GOOS=%s: %s", goos, err.Error())} + } + if len(pkgs) == 0 { + return nil, []string{fmt.Sprintf(" GOOS=%s: no packages loaded from the cli/azd module", goos)} + } - var ( - violations []string - loadErrors []string - ) for _, pkg := range pkgs { for _, e := range pkg.Errors { loadErrors = append(loadErrors, fmt.Sprintf(" %s: %s", pkg.PkgPath, e.Error())) @@ -352,25 +425,13 @@ func TestNoRawTelemetryAttributes(t *testing.T) { rel = filename } rel = filepath.ToSlash(rel) - violations = append(violations, scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, pkg.PkgPath, rel)...) + violations = append( + violations, + scanFileForRawAttributes(pkg.Fset, file, pkg.TypesInfo, pkg.PkgPath, rel)..., + ) } } - - // A package that failed to type-check would silently hide violations, so a - // load error is a failure rather than a false pass. - require.Empty(t, loadErrors, "packages failed to load/type-check:\n%s", strings.Join(loadErrors, "\n")) - - if len(violations) > 0 { - t.Errorf( - "Found %d raw telemetry attribute(s) constructed directly.\n"+ - "Declare an exported fields.AttributeKey (with Classification and Purpose) in\n"+ - "internal/tracing/fields/fields.go and emit via it, e.g. fields.MyKey.String(v),\n"+ - "so the property is discoverable and classifiable by the GDPR metadata pipeline.\n\n"+ - "Raw attributes:\n%s", - len(violations), - strings.Join(violations, "\n"), - ) - } + return violations, loadErrors } // rawAttributePkgPath is the import path of the OpenTelemetry attribute package @@ -381,8 +442,12 @@ const rawAttributePkgPath = "go.opentelemetry.io/otel/attribute" // go.opentelemetry.io/otel/attribute.Key named type. A struct that merely embeds // it — such as the sanctioned fields.AttributeKey — is a different named type and // returns false, which is what keeps the guard from flagging fields.SomeKey.String. +// t is normalized with types.Unalias first so a type alias (e.g. +// type K = attribute.Key), which go/types now models as *types.Alias, is matched +// by the same identity check; the isRawAttributeKeyValueType and +// isFieldsAttributeKeyType helpers do the same. func isRawAttributeKeyType(t types.Type) bool { - named, ok := t.(*types.Named) + named, ok := types.Unalias(t).(*types.Named) if !ok { return false } @@ -396,7 +461,7 @@ func isRawAttributeKeyType(t types.Type) bool { // directly with a key literal bypasses both the attribute constructors and the // fields registry, so the guard inspects these composite literals too. func isRawAttributeKeyValueType(t types.Type) bool { - named, ok := t.(*types.Named) + named, ok := types.Unalias(t).(*types.Named) if !ok { return false } @@ -414,6 +479,13 @@ const fieldsPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/field // keys that were already subject to this guard at their original build site). const baggagePkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/baggage" +// tracingPkgPath is the import path of the core telemetry package whose attribute +// merge logic (attributes.go) legitimately re-emits an existing KeyValue's key via +// a method call (kv.Key.String(...)). Only this plumbing is exempt from the raw +// attribute.Key method rule for re-emission; a product package doing the same on a +// caller-supplied KeyValue would emit a key the classifier never sees. +const tracingPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing" + // isFieldsAttributeKeyType reports whether t is exactly the sanctioned // fields.AttributeKey named type. That wrapper is the only type the metadata // classifier discovers and reads Classification/Purpose/Endpoint from, so it is @@ -421,7 +493,7 @@ const baggagePkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/bagg // allowed. A bare attribute.Key, or any other struct that merely embeds // attribute.Key, produces a key the classifier cannot see and is a violation. func isFieldsAttributeKeyType(t types.Type) bool { - named, ok := t.(*types.Named) + named, ok := types.Unalias(t).(*types.Named) if !ok { return false } @@ -489,15 +561,17 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // isReemittedKeyValueKey reports whether expr is `.Key` where has type // attribute.KeyValue — i.e. a method call like kv.Key.String(v) merely re-emits // the key of a KeyValue that was already built (and, at its build site, already - // subject to this guard). The telemetry baggage plumbing in + // subject to this guard). The telemetry attribute-merge plumbing in // internal/tracing legitimately rebuilds caller-supplied KeyValues with merged - // values this way; it introduces no new key literal, so it is not a violation. + // values this way; it introduces no new key literal. This predicate only + // recognizes the shape — the caller additionally gates it to the sanctioned + // plumbing packages so the same pattern in product code is still flagged. isReemittedKeyValueKey := func(expr ast.Expr) bool { sel, ok := expr.(*ast.SelectorExpr) if !ok || sel.Sel.Name != "Key" { return false } - named, ok := info.TypeOf(sel.X).(*types.Named) + named, ok := types.Unalias(info.TypeOf(sel.X)).(*types.Named) if !ok { return false } @@ -506,6 +580,17 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I obj.Pkg().Path() == rawAttributePkgPath && obj.Name() == "KeyValue" } + // isReemitPlumbingPkgPath reports whether p is a sanctioned telemetry-plumbing + // package allowed to re-emit an existing KeyValue's key through a method call + // (kv.Key.String(...)). Only internal/tracing's attribute merge legitimately + // does this; fields and baggage are included as the other sanctioned plumbing + // packages. Any other package calling kv.Key.String on a caller-supplied + // KeyValue would emit a key the classifier never discovers, so the re-emit + // exemption does not apply there and the call is flagged. + isReemitPlumbingPkgPath := func(p string) bool { + return p == tracingPkgPath || p == fieldsPkgPath || p == baggagePkgPath + } + // rawAttributeBuilderSelection reports whether sel selects a KeyValue-producing // builder (String/Bool/…) defined on the raw attribute.Key type, on a receiver // that is NOT the classified fields.AttributeKey. It matches both a method @@ -513,7 +598,8 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // guard also covers a call written in method-expression form and a builder // captured as a function value (builder := attribute.Key.String). A method // value that only re-emits an existing KeyValue's key (kv.Key.String) is not a - // new key and returns false. + // new key and returns false, but only inside the sanctioned plumbing packages + // (see isReemitPlumbingPkgPath); elsewhere it is still a violation. rawAttributeBuilderSelection := func(sel *ast.SelectorExpr) bool { selection := info.Selections[sel] if selection == nil { @@ -533,7 +619,12 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I if isFieldsAttributeKeyType(selection.Recv()) { return false } - if kind == types.MethodVal && isReemittedKeyValueKey(sel.X) { + // Re-emitting an existing KeyValue's key (kv.Key.String(...)) introduces no + // new key literal, but only the telemetry plumbing legitimately does this + // (internal/tracing's attribute merge). A product package re-emitting a + // caller-supplied KeyValue would emit a key the classifier never sees, so + // the exemption is gated to the sanctioned plumbing packages. + if kind == types.MethodVal && isReemitPlumbingPkgPath(pkgPath) && isReemittedKeyValueKey(sel.X) { return false } return true @@ -796,6 +887,33 @@ var _ = attribute.KeyValue{Key: rawKey, Value: attribute.StringValue("v")} `, wantViolation: true, }, + { + name: "aliased attribute.KeyValue struct literal", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type KV = attribute.KeyValue +var _ = KV{Key: attribute.Key("raw.key"), Value: attribute.StringValue("v")} +`, + wantViolation: true, + }, + { + name: "aliased attribute.Key builder", + src: `package p +import "go.opentelemetry.io/otel/attribute" +type K = attribute.Key +var _ = K("raw.key").String("v") +`, + wantViolation: true, + }, + { + name: "aliased fields.AttributeKey method call", + src: `package p +import "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +type FK = fields.AttributeKey +func f(k FK) { _ = k.String("v") } +`, + wantViolation: false, + }, { name: "attribute.Key builder method expression call", src: `package p @@ -866,12 +984,12 @@ func f(c wrapper) { _ = c.Key.Bool(true) } wantViolation: true, }, { - name: "reemit method on KeyValue key field", + name: "reemit method on KeyValue key field outside plumbing packages", src: `package p import "go.opentelemetry.io/otel/attribute" func f(kv attribute.KeyValue) { _ = kv.Key.String("v") } `, - wantViolation: false, + wantViolation: true, }, { name: "bare key conversion without value", From c23ac0333bd7e5bc5b2ccf047caec2e9ed996bab Mon Sep 17 00:00:00 2001 From: hemarina Date: Mon, 17 Aug 2026 18:45:18 -0700 Subject: [PATCH 18/18] Flag Key-field mutations that bypass the telemetry guard A classified fields.AttributeKey that is copied or zero-valued and then has its embedded Key overwritten keeps the fields.AttributeKey type, so the method-call exemption would accept its emission even though the key is unclassified. A field-by-field attribute.KeyValue assembled the same way sidesteps the KeyValue construction rule. Detect writes to the embedded Key field of a fields.AttributeKey (outside the fields package) and of an attribute.KeyValue (outside the sanctioned plumbing packages, which now include internal/cmd where MapError re-keys already- classified attributes under the error.* namespace). Add fixtures for the copy-mutate, zero-value mutate, and field-by-field forms, plus a negative fixture for an unrelated Key field. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a89c535f-943c-46ff-879d-972c7b2617d2 --- cli/azd/cmd/telemetry_test.go | 118 ++++++++++++++++++++++++++++++++-- 1 file changed, 112 insertions(+), 6 deletions(-) diff --git a/cli/azd/cmd/telemetry_test.go b/cli/azd/cmd/telemetry_test.go index 57eb6f454d0..5e3df5a14ca 100644 --- a/cli/azd/cmd/telemetry_test.go +++ b/cli/azd/cmd/telemetry_test.go @@ -486,6 +486,13 @@ const baggagePkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing/bagg // caller-supplied KeyValue would emit a key the classifier never sees. const tracingPkgPath = "github.com/azure/azure-dev/cli/azd/internal/tracing" +// internalCmdPkgPath is the import path of the error-mapping package whose +// MapError re-keys already-classified attributes under the error.* namespace by +// writing the Key field of an existing attribute.KeyValue (fields.ErrorKey on a +// key that a classified fields.* var produced). That field write is sanctioned +// plumbing, so it is exempt from the KeyValue Key-mutation rule below. +const internalCmdPkgPath = "github.com/azure/azure-dev/cli/azd/internal/cmd" + // isFieldsAttributeKeyType reports whether t is exactly the sanctioned // fields.AttributeKey named type. That wrapper is the only type the metadata // classifier discovers and reads Classification/Purpose/Endpoint from, so it is @@ -680,6 +687,17 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I return p == fieldsPkgPath || p == baggagePkgPath } + // isKeyValueKeyMutationPkgPath reports whether p may write the Key field of an + // existing attribute.KeyValue (kv.Key = ...). The KeyValue construction rule + // forbids building a raw struct outside the plumbing packages, but a value can + // also be assembled field by field; a raw kv.Key = attribute.Key("x") write is + // the same bypass. Only the sanctioned plumbing packages plus internal/cmd + // (MapError re-keys classified attributes under error.*) legitimately mutate a + // KeyValue's key. + isKeyValueKeyMutationPkgPath := func(p string) bool { + return isKeyValuePlumbingPkgPath(p) || p == internalCmdPkgPath + } + var violations []string ast.Inspect(file, func(n ast.Node) bool { switch node := n.(type) { @@ -747,6 +765,38 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I "fields.AttributeKey instead)", rel, pos.Line, node.Name)) } return true + case *ast.AssignStmt: + // Writing the embedded Key field defeats the type-based exemptions the + // two rules above rely on. A copied or zero-valued classified key whose + // Key is overwritten (k := fields.ServiceNameKey; k.Key = + // attribute.Key("raw"); k.String(v)) keeps the fields.AttributeKey type, + // so the method branch would accept its emission even though the key is + // unclassified; and a field-by-field attribute.KeyValue assembled the same + // way sidesteps the KeyValue construction rule. Flag either mutation + // outside the packages sanctioned to perform it. Only plain assignment can + // target a selector; := cannot. + if node.Tok != token.ASSIGN { + return true + } + for _, lhs := range node.Lhs { + sel, ok := lhs.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Key" { + continue + } + baseType := info.TypeOf(sel.X) + pos := fset.Position(sel.Pos()) + if isFieldsAttributeKeyType(baseType) && pkgPath != fieldsPkgPath { + violations = append(violations, fmt.Sprintf( + " %s:%d: the Key of a fields.AttributeKey is mutated outside the fields "+ + "package (its emissions would be exempted by type while carrying an "+ + "unclassified key)", rel, pos.Line)) + } else if isRawAttributeKeyValueType(baseType) && !isKeyValueKeyMutationPkgPath(pkgPath) { + violations = append(violations, fmt.Sprintf( + " %s:%d: attribute.KeyValue.Key is mutated outside the telemetry plumbing "+ + "(assemble it from a fields.AttributeKey instead)", rel, pos.Line)) + } + } + return true } return true }) @@ -761,15 +811,20 @@ func scanFileForRawAttributes(fset *token.FileSet, file *ast.File, info *types.I // producing attribute.Key builders reached as a method value (through a // parameter, local, struct field, or function result) or as a method expression // (attribute.Key.String, including when captured as a function value), a -// fields.AttributeKey constructed outside the fields package, and any raw +// fields.AttributeKey constructed outside the fields package, any raw // attribute.KeyValue struct built outside the sanctioned plumbing packages // (whatever its key — a literal, a run-time attribute.Key(x) conversion, or a -// variable forwarding one) are all flagged; while the sanctioned promoted-method +// variable forwarding one), and a Key-field mutation that would smuggle an +// unclassified key through the type-based exemptions (overwriting the Key of a +// copied or zero-valued fields.AttributeKey, or assembling an attribute.KeyValue +// field by field) are all flagged; while the sanctioned promoted-method // call on the classified fields.AttributeKey, fields.ExtensionUsageAttribute, a -// bare attribute.Key(k) conversion (which does not build a KeyValue), and -// non-KeyValue uses of a key (e.g. a map lookup) are not. The in-package -// exemptions — the fields registry vars, ExtensionUsageAttribute, and the -// fields/baggage KeyValue plumbing — are keyed on package path and so are +// bare attribute.Key(k) conversion (which does not build a KeyValue), a write to +// an unrelated Key field, and non-KeyValue uses of a key (e.g. a map lookup) are +// not. The in-package +// exemptions — the fields registry vars, ExtensionUsageAttribute, the +// fields/baggage KeyValue plumbing, and internal/cmd's error.* re-keying — are +// keyed on package path and so are // exercised by the module walk rather than these package-p fixtures. Fixtures are // type-checked against the real attribute and fields packages via go/packages, so // the guard runs with the same type information it uses on the module. @@ -991,6 +1046,57 @@ func f(kv attribute.KeyValue) { _ = kv.Key.String("v") } `, wantViolation: true, }, + { + name: "mutated Key on a copied classified fields.AttributeKey", + src: `package p +import ( + "go.opentelemetry.io/otel/attribute" + "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +) +func f() { + k := fields.ServiceNameKey + k.Key = attribute.Key("raw.key") + _ = k.String("v") +} +`, + wantViolation: true, + }, + { + name: "mutated Key on a zero-value fields.AttributeKey", + src: `package p +import ( + "go.opentelemetry.io/otel/attribute" + "github.com/azure/azure-dev/cli/azd/internal/tracing/fields" +) +func f() { + var k fields.AttributeKey + k.Key = attribute.Key("raw.key") + _ = k.String("v") +} +`, + wantViolation: true, + }, + { + name: "field-by-field attribute.KeyValue construction with a raw key", + src: `package p +import "go.opentelemetry.io/otel/attribute" +func f() attribute.KeyValue { + var kv attribute.KeyValue + kv.Key = attribute.Key("raw.key") + kv.Value = attribute.StringValue("v") + return kv +} +`, + wantViolation: true, + }, + { + name: "assignment to unrelated Key field", + src: `package p +type config struct{ Key string } +func f(c *config) { c.Key = "x" } +`, + wantViolation: false, + }, { name: "bare key conversion without value", src: `package p