feat: add host-validated extension telemetry gRPC service#9174
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
4c63574 to
d1f3de6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a2bc94c to
5644075
Compare
5644075 to
38fd61a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
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.mdand new cross-cutting emission paths infeature-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-packageartifact construction path. Both metadata assignments inservice_graph.goandpublish.gocould be removed while this still passes. Add behavior tests for both paths that inspect the resulting package artifact and assertazd.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) |
This comment was marked as off-topic.
This comment was marked as off-topic.
jongio
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
Dismissing my earlier change request — the redesign (host-validated telemetry gRPC service) addresses the coupling concern. I'll do another pass.
vhvb1989
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
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.agentsextension. 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.modein a follow-up PR, after this ships in a core release.TelemetryServicewith a singleAddCommandUsageAttribute(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.deployorcmd.up) as a de-duplicated string slice.agent.deploy.modeasSystemMetadata/FeatureInsightwith the fixed valuescode,container,byo_image.service-target-providercapability may report, and only for allowed keys/values. Invalid input is rejected and never echoed into errors or logs.Unimplementedand the extension ignores it.azd.fromPackageartifact provenance marker (payload provenance, not telemetry) so a future producer can tell a user-supplied--from-packagepayload from one azd built.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.
Follow-up (not in this PR)
Tracked in #9230:
agent.deploy.modeat the earliest authoritative decision, on a released core SDK.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
fields/fields.gowith correct classification (SystemMetadata) and purpose (FeatureInsight)docs/specs/metrics-audit/telemetry-schema.mdcode,container,byo_image)