Classify raw OpenTelemetry attributes so GDPR metadata is applied - #9598
Draft
Marina He (hemarina) wants to merge 9 commits into
Draft
Classify raw OpenTelemetry attributes so GDPR metadata is applied#9598Marina He (hemarina) wants to merge 9 commits into
Marina He (hemarina) wants to merge 9 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{}{}
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:657
- azd-code-reviewer: This fixture codifies a false negative. Any local struct embedding
attribute.Keyis accepted, although onlyfields.AttributeKeycarries classifier metadata and is discoverable. Resolve the receiver to the exactfields.AttributeKeynamed type and treat arbitrary wrappers as violations.
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,
cli/azd/cmd/telemetry_test.go:314
- azd-code-reviewer: Excluding all nested modules leaves first-party extension product code outside this guard, contrary to the stated prevention goal. For example,
extensions/azure.ai.projects/internal/provisioning/foundry_provisioning_provider.go:1049still emitsattribute.String("provision.network_mode", ...). Add equivalent module-level checks (or a shared scanner used by each module) so raw fields cannot continue entering through extensions.
// - Nested modules (extensions/*, test/evals, test data samples) have their own
// go.mod and are not matched by the "./..." pattern.
cli/azd/internal/tracing/fields/fields.go:372
- These new
AttributeKeydefinitions require a privacy review underdocs/specs/metrics-audit/privacy-review-checklist.md:8-21, andcli/azd/AGENTS.md:329-332requires that review for new fields or classification changes. The provided PR description contains no privacy checklist or review record; complete that process before merging.
AuthCacheClearFailedKey = AttributeKey{
Key: attribute.Key("auth.cache_clear_failed"),
Classification: SystemMetadata,
Purpose: PerformanceAndHealth,
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
|
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)ContainerPublishRemoteBuildKey(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) thatAST-scans product code for raw
attribute.<Constructor>("literal", ...)usage andfails if a telemetry attribute bypasses the classified
fieldskeys.fields.
Testing
go test ./cmd/ -run TestNoRawTelemetryAttributes -count=1— passes