Skip to content

feat: add host-validated extension telemetry gRPC service#9174

Open
huimiu wants to merge 4 commits into
mainfrom
hui/telemetry-deployment-mode
Open

feat: add host-validated extension telemetry gRPC service#9174
huimiu wants to merge 4 commits into
mainfrom
hui/telemetry-deployment-mode

Conversation

@huimiu

@huimiu huimiu commented Jul 16, 2026

Copy link
Copy Markdown
Member

Fixes #9255. Part of #9230 (the producer lands in a follow-up PR).

Why

We want to know which path a hosted agent deploy actually took: source code (code), a container azd built (container), or an existing image the user brought (byo_image). The manifest language alone can't tell these apart, so adoption and troubleshooting telemetry is ambiguous today.

An earlier version of this PR taught azd core to read that value out of deploy artifact metadata and hardcoded the azure.ai.agents extension. That was rejected: it pointed the dependency the wrong way (core knowing about one specific extension), shipped telemetry-only data through the deploy artifact contract, and would force a core change for every future extension.

What this PR does

This PR replaces that with a generic, host-controlled way for any authenticated azd extension to contribute a telemetry value, and registers the first field for it. It is core-only: the plumbing and the field are in, but nothing emits a value yet. The agents extension starts producing agent.deploy.mode in a follow-up PR, after this ships in a core release.

  • New gRPC TelemetryService with a single AddCommandUsageAttribute(key, value) call. An extension calls it; azd core validates the key/value against a host-owned allowlist and attaches the value to the current command's telemetry event (cmd.deploy or cmd.up) as a de-duplicated string slice.
  • Register agent.deploy.mode as SystemMetadata / FeatureInsight with the fixed values code, container, byo_image.
  • Command-scoped storage in core so a value lands only on the owning command event and never leaks onto synthetic child spans or sibling commands.
  • Authenticated and capability-gated: only a signed extension with the service-target-provider capability may report, and only for allowed keys/values. Invalid input is rejected and never echoed into errors or logs.
  • Best-effort and backward compatible: telemetry never affects deploy; an older host returns Unimplemented and the extension ignores it.
  • Add a generic azd.fromPackage artifact provenance marker (payload provenance, not telemetry) so a future producer can tell a user-supplied --from-package payload from one azd built.
  • Document the SDK API and the registered field (SDK reference and telemetry schema). The user-facing "emitted data" docs land with the producer in the follow-up PR, since nothing emits the field yet.

Why this approach

Core stays extension-agnostic: it owns the telemetry schema (key, allowed values, classification, purpose, eligible commands) and validates everything an extension sends. Extensions can't invent telemetry or push arbitrary data, which keeps azd's existing per-field privacy governance intact across the extension boundary. Values are collected per command and flushed onto the correct command span, so they can't leak onto synthetic package/provision spans or later commands.

Open question — the allowlist

Core keeps an allowlist: every key/value an extension can send is pre-registered in core with its classification and eligible commands.

  • Pro: preserves our strict privacy model — no extension can dump arbitrary data (PII, resource names, paths) into azd telemetry.
  • Con: a new telemetry field can't ship with the extension alone — it has to be added in core and picked up in a released azd, so extension telemetry is tied to the core release cadence (the same as all other azd telemetry today).
  • Decision needed: keep the strict allowlist, or loosen it (e.g. namespaced keys) and accept more privacy risk. Worth a quick check with the telemetry/privacy owner.

Follow-up (not in this PR)

Tracked in #9230:

  • Agents extension producer: report agent.deploy.mode at the earliest authoritative decision, on a released core SDK.
  • User-facing telemetry docs (docs/reference/telemetry-data.md, feature-telemetry-matrix.md) once the field actually emits.

Testing

go build, targeted unit and integration tests, gofmt, go vet, golangci-lint (0 issues), go fix -diff (clean), and cspell all pass. Race checks run in CI.

Telemetry Change Checklist

  • Field defined in fields/fields.go with correct classification (SystemMetadata) and purpose (FeatureInsight)
  • Field documented in docs/specs/metrics-audit/telemetry-schema.md
  • Enum values documented with the allowed value set (code, container, byo_image)
  • Hashing applied where required — N/A; only fixed enum values are accepted
  • Measurement OTel type — N/A; this is a string slice, not a measurement
  • No customer content or PII: only the three fixed enums can be accepted; unknown keys and values are rejected and never logged
  • Privacy review of the new field and the new extension-to-host telemetry boundary

@azure-pipelines

This comment was marked as outdated.

@github-actions github-actions Bot added the ext-agents azure.ai.agents extension label Jul 16, 2026
@huimiu
huimiu force-pushed the hui/telemetry-deployment-mode branch from 4c63574 to d1f3de6 Compare July 21, 2026 07:39
@huimiu
huimiu marked this pull request as ready for review July 21, 2026 10:18
Copilot AI review requested due to automatic review settings July 21, 2026 10:18
@azure-pipelines

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

This comment was marked as outdated.

jongio

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings July 21, 2026 13:13

This comment was marked as outdated.

jongio

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings July 21, 2026 15:35

This comment was marked as outdated.

jongio

This comment was marked as outdated.

jongio

This comment was marked as outdated.

@huimiu
huimiu marked this pull request as draft July 22, 2026 01:53
@huimiu
huimiu force-pushed the hui/telemetry-deployment-mode branch from a2bc94c to 5644075 Compare July 22, 2026 09:01
@github-actions github-actions Bot added the area/extensions Extensions (general) label Jul 22, 2026
@huimiu
huimiu force-pushed the hui/telemetry-deployment-mode branch from 5644075 to 38fd61a Compare July 22, 2026 09:40
@huimiu huimiu changed the title feat: record hosted agent deployment mode feat: add host-validated extension telemetry gRPC service Jul 22, 2026
@Azure Azure deleted a comment from azure-sdk-automation Bot Jul 22, 2026
@huimiu huimiu linked an issue Jul 22, 2026 that may be closed by this pull request
@huimiu
huimiu marked this pull request as ready for review July 22, 2026 10:17
Copilot AI review requested due to automatic review settings July 22, 2026 10:18
@azure-pipelines

This comment was marked as outdated.

This comment was marked as outdated.

Copilot AI review requested due to automatic review settings July 22, 2026 12:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • cli/azd/pkg/azdext/telemetry.pb.go: Generated file
  • cli/azd/pkg/azdext/telemetry_grpc.pb.go: Generated file
Comments suppressed due to low confidence (2)

docs/specs/metrics-audit/telemetry-schema.md:91

  • This field is emit-capable once this host ships, even if the first-party producer follows later. Repository telemetry guidance requires new fields to be reflected in docs/reference/telemetry-data.md and new cross-cutting emission paths in feature-telemetry-matrix.md (cli/azd/AGENTS.md:246-281); currently neither mentions this field/service. Add those entries now so the public reference and audit inventory stay aligned with the host behavior. [azd-code-reviewer]
| Agent deployment mode | `agent.deploy.mode` | SystemMetadata | FeatureInsight | `string[]`; per-command set of `code`/`container`/`byo_image` contributed by an authenticated extension through the telemetry service; fixed enum, not hashed; App Insights stores JSON text |

cli/azd/internal/cmd/from_package_test.go:20

  • This test only verifies that two constants are equal; it never exercises either changed --from-package artifact construction path. Both metadata assignments in service_graph.go and publish.go could be removed while this still passes. Add behavior tests for both paths that inspect the resulting package artifact and assert azd.fromPackage == "true". [azd-code-reviewer]
func TestFromPackageMetadataConstant(t *testing.T) {
	t.Parallel()
	require.Equal(t, "azd.fromPackage", project.MetadataKeyFromPackage)
	require.Equal(t, azdext.ArtifactMetadataKeyFromPackage, project.MetadataKeyFromPackage)

// service, which routes them to the current scope. Closing the scope below
// attaches those values to this command span only, so they never leak onto
// synthetic child spans or sibling commands.
usageScope := tracing.BeginCommandUsageScope(eventName)
@azure-sdk-automation

This comment was marked as off-topic.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generic host-validated telemetry service resolves the coupling concern from my earlier note. Core no longer hardcodes the agents extension id or reads its private metadata key: eligibility now runs through the signed capability check plus the host-owned key/value allowlist, and the from-package marker is a plain provenance boolean with the constant aliased from the SDK (MetadataKeyFromPackage = azdext.ArtifactMetadataKeyFromPackage) so core and extensions can't drift. That's the right direction, and it keeps Artifact.Metadata about payload rather than telemetry.

One thing to settle before merge, and it's your own open item rather than a new one: the privacy review of the new extension-to-host telemetry boundary is still unchecked. The allowlist keeps the strict model intact, but the choice to keep it strict versus loosening to namespaced keys should get a sign-off from the telemetry/privacy owner, since it changes who can put values into azd telemetry. Worth locking that down instead of carrying it as an open question into merge.

The test-coverage and docs gaps the automated reviewer flagged (publish not classified in TestCommandTelemetryCoverage, from_package_test only asserting constant equality) are worth a quick pass. Deferring the emitted-data docs to the producer PR makes sense given nothing emits the field yet.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coupling concern from the earlier design is resolved. Core no longer hardcodes an extension id or reads an extension's private metadata key. Extension telemetry now goes through the generic AddCommandUsageAttribute call, gated by the signed capability check plus the host-owned key and value allowlist, and Artifact.Metadata stays about payload with azd.fromPackage as plain provenance.

The command-scoped store attaches values only to the owning cmd.deploy or cmd.up span and drops anything reported while a synthetic child scope is on top, so a value can't leak onto a sibling or child span. The handler fails closed on missing claims, missing capability, unknown key, invalid policy, and disallowed value, and rejected caller text isn't echoed back into the returned error. Tests cover each of those paths plus concurrent reporting and duplicate collapsing.

One pre-merge gate is still open, and it's your own checklist item rather than a code change: the privacy review of the new extension-to-host telemetry boundary, along with the strict-allowlist versus namespaced-keys decision. That sign-off is worth locking down before merge since it's what opens the boundary. Deferring the emitted-data docs to the producer PR is reasonable while nothing emits the field yet.

@vhvb1989
vhvb1989 dismissed their stale review July 24, 2026 17:31

Dismissing my earlier change request — the redesign (host-validated telemetry gRPC service) addresses the coupling concern. I'll do another pass.

@vhvb1989 vhvb1989 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reworking this — the new host-validated telemetry gRPC service looks like the right direction and addresses the coupling concern I raised (no hardcoded extension id, Artifact.Metadata stays about payload). I've dismissed my earlier change request. I'll do another pass over the key parts and follow up.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 898b2f9. Checked the host-owned allowlist, the capability gate off signed claims, and the command-scoped storage. internal/tracing, internal/grpcserver, pkg/azdext/telemetry and cmd/middleware all pass locally.

One residual item, not blocking this PR. The doc comment on ResetCommandUsageForTest says tests relying on command usage state must not use t.Parallel(), but the indirect callers still do. TelemetryMiddleware.Run now begins and closes a scope on every invocation, and it's exercised from parallel tests in cmd/middleware: telemetry_test.go WithRootAction and WithChildAction, plus TestTelemetryMiddleware_Run_ResponseError_WrapsWithTraceId in middleware_test.go. That makes Begin(A), Begin(B), Close(A) reachable, where Close(A) hits the "not the current scope" branch, gets logged, and leaves A on the stack for the rest of the test binary run.

Nothing asserts extension-contributed values through the middleware today, so nothing fails, and production is one strictly nested command chain per process, so this is test-only right now. Worth either isolating scope ownership per invocation or dropping t.Parallel() on the middleware tests that call Run, so the first test that does assert those attributes doesn't come in flaky.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-verified at 898b2f9, unchanged since my last review. The host-validated telemetry service keeps core extension-agnostic: AddCommandUsageAttribute fails closed on missing claims, an unregistered key, a missing capability, an invalid policy, and a disallowed value, and it gates on signed capabilities instead of a hardcoded extension id. The command-scoped store attaches values only to the eligible top scope and never falls back to an ancestor, so nothing lands on a synthetic child or sibling span. azd.fromPackage stays plain payload provenance with the core constant aliased from the SDK, so core and extensions can't drift. Approving.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 898b2f9: the host-validated telemetry gRPC service, the command usage scope stack, and the from-package provenance marker. Validation fails closed on unknown keys, missing capabilities, and disallowed values; the scope store is mutex-guarded and rejects out-of-order closes without corrupting state; and the grpcserver, tracing, and middleware tests pass locally. Approving.

@jongio jongio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Re-checked the host-validated telemetry service, the command-scoped usage store, and the --from-package provenance marker against the current head. The allowlist validation fails closed, capability gating is enforced from signed claims, and command-scoped storage keeps values off sibling and synthetic child spans. No new concerns from me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/extensions Extensions (general) ext-agents azure.ai.agents extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a generic host-validated extension telemetry gRPC API

4 participants