Classify raw OpenTelemetry attributes so GDPR metadata is applied

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 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` 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
+  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**
 

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)
 			}
 		}

cli/azd/cmd/telemetry_test.go

@@ -4,6 +4,14 @@
 package cmd
 
 import (
+	"fmt"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -26,7 +34,8 @@
 // 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 +56,15 @@
 			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,9 +272,280 @@
 			require.NotEmpty(t, kv.Value.AsString())
 		}
 	})
+
+	// Container publish telemetry fields
+	t.Run("ContainerFields", func(t *testing.T) {
+		t.Parallel()
+		kv := fields.ContainerRemoteBuildKey.Bool(true)
+		require.Equal(t, "container.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(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
+// issue #1803).
+//
+// Legitimately excluded from the scan:
+//   - *_test.go files (test fixtures build raw attributes on purpose).
+//   - extensions/... — independent extension modules with their own schema.
+func TestNoRawTelemetryAttributes(t *testing.T) {
+	t.Parallel()
+
+	// 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)
+
+		fset := token.NewFileSet()
+		file, parseErr := parser.ParseFile(fset, path, nil, 0)
+		if parseErr != nil {
+			return nil // skip unparseable files
+		}
+
+		violations = append(violations, scanGoFileForRawAttributes(fset, file, rel)...)
+		return nil
+	})
+	require.NoError(t, err)
+
+	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"),
+		)
+	}
+}
+
+// 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
 }
 
-// TestCommandTelemetryCoverage ensures every user-facing command is explicitly categorized
+// 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.