Classify raw OpenTelemetry attributes so GDPR metadata is applied - #9598
Open
Marina He (hemarina) wants to merge 18 commits into
Open
Classify raw OpenTelemetry attributes so GDPR metadata is applied#9598Marina He (hemarina) wants to merge 18 commits into
Marina He (hemarina) wants to merge 18 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 20 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
There was a problem hiding this comment.
Pull request overview
Classifies raw OpenTelemetry attributes so GDPR metadata is discoverable and adds regression protection.
Changes:
- Adds classified telemetry keys and bounded AKS values.
- Replaces raw attribute emission.
- Updates telemetry documentation and adds an AST guard test.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
docs/specs/metrics-audit/telemetry-schema.md |
Documents classification contracts and fields. |
docs/specs/metrics-audit/privacy-review-checklist.md |
Expands privacy-review guidance. |
docs/specs/metrics-audit/feature-telemetry-matrix.md |
Updates telemetry inventory. |
docs/reference/telemetry-data.md |
Documents emitted fields. |
docs/guides/feature-telemetry.md |
Adds instrumentation guidance. |
cli/azd/pkg/project/service_target_aks.go |
Emits a bounded classified skip reason. |
cli/azd/pkg/project/container_helper.go |
Uses the classified remote-build key. |
cli/azd/internal/tracing/fields/fields.go |
Defines classified attribute keys. |
cli/azd/cmd/telemetry_test.go |
Adds field assertions and AST scanning. |
cli/azd/cmd/auth_login.go |
Classifies cache-clear failure telemetry. |
cli/azd/AGENTS.md |
Records telemetry implementation requirements. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/internal/tracing/fields/fields.go:1161
- [azd-code-reviewer] This emits
container.remotebuild, while the new schema/reference entries consistently documentcontainer.publish.remotebuild(telemetry-schema.md:193,feature-telemetry-matrix.md:164,telemetry-data.md:430, and the privacy checklist). As written, the catalog and downstream guidance point to a property azd never emits. Use one key consistently; preserving the existing raw key requires correcting those docs, while adopting the namespaced key requires changing this definition and its assertions.
Key: attribute.Key("container.remotebuild"),
cli/azd/cmd/telemetry_test.go:365
- [azd-code-reviewer] This directory-wide exemption also skips product emission code such as
internal/tracing/tracer.go, so a future raw constructor there would pass the new guard despite bypassingfields.AttributeKey. There are currently no matching constructors underinternal/tracing; remove this exemption (or allowlist only an exact implementation file if one later needs it) so the guard covers all core emission paths.
// 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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (4)
cli/azd/cmd/telemetry_test.go:402
- [azd-code-reviewer] This check only accepts selector bases that are the imported package identifier.
attribute.Key("raw.key").String(v)has aCallExprbase and therefore passes, even though it also bypassesfields.AttributeKey. Extend the scanner and add a negative fixture for the key-method form so the guard enforces its stated contract.
pkgIdent, ok := sel.X.(*ast.Ident)
if !ok || pkgIdent.Name != attrPkgName {
return true
}
docs/specs/metrics-audit/telemetry-schema.md:199
- [azd-code-reviewer] This new field's schema entry documents the enum but not its required hashing and measurement status. Mark it explicitly as not hashed and not a measurement.
| Skip reason | `skip.reason` | SystemMetadata | FeatureInsight | Bounded enum (`cluster_not_provisioned`); never raw error text. Emitted on `aks.postprovision.skip`. |
docs/specs/metrics-audit/telemetry-schema.md:327
- [azd-code-reviewer] This new field's schema entry omits the required hashing and measurement decisions. Explicitly record that the bounded enum is not hashed and is not a measurement.
| 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. |
docs/specs/metrics-audit/telemetry-schema.md:193
- [azd-code-reviewer] The authoritative schema must state hashing and measurement status for each new field, but this row leaves both implicit. Record that this boolean is not hashed and is not a measurement so the data-catalog review has complete metadata.
This issue also appears in the following locations of the same file:
- line 199
- line 327
| Publish remote build | `container.remotebuild` | SystemMetadata | FeatureInsight | Bool — whether the image was built remotely (ACR) rather than locally. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (7)
cli/azd/cmd/telemetry_test.go:502
- [azd-code-reviewer] This added fixture line exceeds the repository's 125-character Go limit (
cli/azd/AGENTS.md:115-126), so thellllint check will reject it.
src: `package p; import otelattr "go.opentelemetry.io/otel/attribute"; var _ = otelattr.Bool("raw.key", true)`,
cli/azd/cmd/telemetry_test.go:507
- [azd-code-reviewer] This added fixture line exceeds the repository's 125-character Go limit (
cli/azd/AGENTS.md:115-126), so thellllint check will reject it.
src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.IntSlice("raw.key", []int{1})`,
cli/azd/cmd/telemetry_test.go:512
- [azd-code-reviewer] This added fixture line exceeds the repository's 125-character Go limit (
cli/azd/AGENTS.md:115-126), so thellllint check will reject it.
src: `package p; import "go.opentelemetry.io/otel/attribute"; var _ = attribute.Key("raw.key").String("v")`,
docs/specs/metrics-audit/feature-telemetry-matrix.md:164
- [azd-code-reviewer] This matrix update is incomplete:
auth.cache_clear_failedis also added in this PR, but theauth loginrow at line 46 and the command-specific field table at line 118 still list onlyauth.method. Add the new field there so the command-to-telemetry inventory stays synchronized.
| **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 |
docs/specs/metrics-audit/privacy-review-checklist.md:204
- [azd-code-reviewer]
AttributeKeyexposes anEndpointmember, and the other updated docs use that exact name.EndpointIdTypedoes not exist in this codebase, so this checklist would direct contributors to document the wrong field.
- EndpointIdType (only when the value is a known endpoint identifier)
cli/azd/cmd/telemetry_test.go:523
- [azd-code-reviewer] This is not a sanctioned promoted-method case:
kis explicitly an OTelattribute.Key, not a classifiedfields.AttributeKey. Product code usingk.String(...)would emit an unregistered key, yet this fixture requires the guard to accept it. Make this a violation and use type information, or equivalent declaration tracking, to distinguish classified receivers.
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,
cli/azd/cmd/telemetry_test.go:497
- [azd-code-reviewer] This added fixture line exceeds the repository's 125-character Go limit (
cli/azd/AGENTS.md:115-126), so thellllint check will reject it.
This issue also appears in the following locations of the same file:
- line 502
- line 507
- line 512
src: `package p; import "go.opentelemetry.io/otel/attribute"; const k = "raw.key"; var _ = attribute.String(k, "v")`,
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cli/azd/cmd/telemetry_test.go:449
- [azd-code-reviewer] This name-only tracking makes the guard unsound across scopes: a raw
k attribute.Keyin one function causes a shadowed, classifiedk.String(...)elsewhere to be reported, while an inferred declaration such asvar k = attribute.Key("raw.key")is not recorded and bypasses the guard. Track declaration identity withast.Ident.Obj(orgo/types) and add fixtures for both inferred declarations and shadowing so the test reliably enforces its stated contract.
rawKeyIdents := map[string]struct{}{}
Detect direct attribute.KeyValue{Key: <const>, ...} 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
Contributor
Author
|
Addressed the latest review feedback in 175fa77:
Validation: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/azd/cmd/telemetry_test.go:531
- [azd-code-reviewer] Treating every non-constant key as a safe forward leaves a direct bypass:
attribute.KeyValue{Key: attribute.Key(runtimeKey), Value: ...}has nogo/typesconstant value and is accepted, even though it creates a new unclassified key. This contradicts the documented rule that dynamic keys are allowed only throughfields.ExtensionUsageAttribute. Track the key expression's origin and permit only registeredfields.AttributeKey/existing baggage forwarding, or explicitly allowlist the known plumbing sites.
// 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
cli/azd/pkg/project/container_helper.go:627
- [azd-code-reviewer] This records the configured preference before publishing, not the method that actually built the image. If
runRemoteBuildfails, lines 638-655 fall back topublishLocalImage, but this property remainstrue; that contradicts the new docs and PR description saying it identifies local versus remote build usage. Either update the attribute after the selected path completes (including fallback) or document/name it as “remote build requested.”
fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild),
cli/azd/cmd/telemetry_test.go:659
- [azd-code-reviewer] Calls through a constructor alias are not detected. For example,
builder := attribute.String; builder("raw.key", "v")resolves the call identifier to a*types.Var, while the selector used in the assignment has notypes.Selection, so neither branch reports it. Track package-function aliases (and add a fixture) so the guard actually rejects all raw constructor usage promised by this test.
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)))
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/azd/cmd/telemetry_test.go:620
- Exempting every
fields.AttributeKeycomposite literal inside thefieldspackage also permits function-local or unexported keys that the GDPR classifier cannot discover. Calls through those values are then accepted because their receiver type is sanctioned. Validate that static definitions in this package belong to exported package-level vars, with an explicit exception for the dynamicExtensionUsageAttributefactory. (azd-code-reviewer)
// 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) {
cli/azd/cmd/telemetry_test.go:532
- Treating every non-constant key expression as safe leaves a direct bypass: a package variable can be initialized with
attribute.Key("raw.key")and then used inattribute.KeyValue{Key: rawKey, ...}; both constructs pass this scanner while emitting an uncatalogued key. The guard needs to prove that a forwarded key originates from a classifiedfields.AttributeKey, or narrowly allowlist the existing baggage/rehydration plumbing instead of allowing arbitrary variables and function results. (azd-code-reviewer)
// 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.
cli/azd/pkg/project/container_helper.go:629
remoteBuildUsedis initialized totruebefore parsing options or running ACR. Any early return (for example, invalid publish options or a remote-build failure when no local runtime is available) therefore reports that the image was built remotely even though no build succeeded, contradicting the new field documentation. Initialize it as false and set it true only afterrunRemoteBuildsucceeds, or document this as the requested/attempted method instead. (azd-code-reviewer)
// 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
Comment on lines
+625
to
+626
| defer func() { span.EndWithStatus(err) }() | ||
| span.SetAttributes( | ||
| attribute.Bool("container.remotebuild", serviceConfig.Docker.RemoteBuild), | ||
| ) | ||
| span.SetAttributes(fields.ContainerRemoteBuildKey.Bool(serviceConfig.Docker.RemoteBuild)) |
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
cli/azd/pkg/project/container_helper.go:662
- [azd-code-reviewer] The field-constant test only proves that the key can build a boolean; it does not cover this new outcome-dependent assignment. A regression could report
truefor the existing local-fallback path (orfalseafter a successful remote build) while all added tests still pass. Add span-capture assertions for remote success, local fallback, and remote failure so the documented emitted semantics are pinned.
} else {
remoteBuildUsed = true
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/cmd/telemetry_test.go:538
azd-code-reviewer: I think this exception needs to be limited to the actual tracing-plumbing package. As written, any product package can accept anattribute.KeyValue(including one returned by external code) and callkv.Key.String(...); the scanner suppresses it even though the receiver is not a classifiedfields.AttributeKey. The fixture at line 874 explicitly locks in this false negative. Gate this branch by the sanctioned package path and make the package-pfixture expect a violation.
if kind == types.MethodVal && isReemittedKeyValueKey(sel.X) {
return false
}
cli/azd/cmd/telemetry_test.go:333
azd-code-reviewer: This module scan only loads files selected for the test runner's currentGOOS/build tags. For example, Linux CI omits every*_windows.gofile, so a raw attribute added to Windows-only product code would pass this guard. Run the scan for each supported target (linux,windows, anddarwin) or add equivalent per-platform CI coverage so the repository-wide invariant applies to all shipped code.
pkgs, err := packages.Load(cfg, "./...")
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
| // 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) { |
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
Contributor
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cli/azd/cmd/telemetry_test.go:347
- This manually collects and sorts map keys despite the repository's Go 1.26 convention to use
slices.Sorted(maps.Keys(...))(cli/azd/AGENTS.md:338-343, 387-389). Replace this block withtargets := slices.Sorted(maps.Keys(goosSet))and useslices.Sort(violations)below so the imports remain consistent.
targets := make([]string, 0, len(goosSet))
for goos := range goosSet {
targets = append(targets, goos)
}
sort.Strings(targets)
cli/azd/cmd/telemetry_test.go:341
- The guard does not cover the tagged Windows build it cites. This loop invokes
packages.Loadwithout-tags=oneauth, so Windows selectspkg/oneauth/oneauth.go(!(oneauth && windows)) and never scansoneauth_windows.go,c_funcs.go, orutils.go;cli/azd/ci-build.ps1:131-136can build that variant with theoneauthtag. A raw attribute added there would therefore pass this test. Add a build-tag-aware scan target (or a syntax fallback/dedicated Windows scan for the cgo-backed variant) so every shipped source variant is checked. (azd-code-reviewer)
This issue also appears on line 343 of the same file.
goosSet := map[string]bool{"linux": true, "windows": true, "darwin": true}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix https://github.com/Azure/azure-dev-pr/issues/1803
Summary
Several telemetry properties were emitted as raw OpenTelemetry attribute literals
(e.g.
attribute.String("skip.reason", ...)) instead of the classifiedfields.AttributeKeyvariables defined ininternal/tracing/fields. Raw literalscarry no data-classification or purpose metadata, so the metadata classifier never
sees them and the properties land unclassified.
This PR routes those attributes through classified
fields.*keys and adds a guardtest so new raw attributes can't creep back in.
Changes
AttributeKeydefinitions ininternal/tracing/fields/fields.go:AksSkipReasonKey(skip.reason)ContainerRemoteBuildKey(container.remotebuild)AuthCacheClearFailedKey(auth.cache_clear_failed)attribute.String/Bool(...)calls with the classified keys in:pkg/project/service_target_aks.gopkg/project/container_helper.gocmd/auth_login.goerrortext to a bounded compile-timestring enum (
cluster_not_provisioned), so the telemetry value stays lowcardinality and never carries user-derived text.
TestNoRawTelemetryAttributesguard test (cmd/telemetry_test.go). Theguard is type-aware (uses
go/typesviagolang.org/x/tools/go/packages): itresolves each telemetry call's types and fails when a
KeyValue-producingattribute constructor is used on anything other than the classified
fields.AttributeKey— catching rawattribute.<Constructor>("literal", ...),bare
attribute.Keyvalues, and arbitrary wrapper structs, including throughimport aliases, dot imports, struct fields, and function results.
fields.
Testing
go test ./cmd/ -run 'TestNoRawTelemetryAttributes|TestRawTelemetryAttributeScanner' -count=1— passesgo vet,golangci-lint,gofmtcleanTelemetry Change Checklist
New Fields
fields/fields.go(AksSkipReasonKey,ContainerRemoteBuildKey,AuthCacheClearFailedKey)SystemMetadata;FeatureInsightfor skip/remotebuild,PerformanceAndHealthfor auth cache-clear)docs/specs/metrics-audit/telemetry-schema.mdskip.reason=cluster_not_provisioned;auth.cache_clear_failed=auth/subscriptions;container.remotebuild= bool)ext.*fields added)Privacy
SystemMetadata)CustomerContentemitted in telemetryerrortext is replaced by a bounded enumTesting
Documentation
docs/specs/metrics-audit/telemetry-schema.md)