feat: let extensions report named telemetry events with attributes - #9174
Open
Hui Miao (huimiu) wants to merge 4 commits into
Open
feat: let extensions report named telemetry events with attributes#9174Hui Miao (huimiu) wants to merge 4 commits into
Hui Miao (huimiu) wants to merge 4 commits into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Hui Miao (huimiu)
force-pushed
the
hui/telemetry-deployment-mode
branch
from
July 21, 2026 07:39
4c63574 to
d1f3de6
Compare
Hui Miao (huimiu)
marked this pull request as ready for review
July 21, 2026 10:18
Hui Miao (huimiu)
requested review from
JeffreyCA,
Rick Winter (RickWinter),
Glenn Harper (glharper),
Marina He (hemarina),
Zhijie Huang (hund030),
Richard Park (richardpark-msft),
tg-msft,
John Miller (therealjohn),
Travis Angevine (trangevi),
Travis Wilson (trrwilson) and
Victor Vazquez (vhvb1989)
as code owners
July 21, 2026 10:18
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
The mutable source-name gate and concurrent manager lookup leave privacy and race-condition blockers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Files not reviewed (2)
- cli/azd/pkg/azdext/telemetry.pb.go: Generated file
- cli/azd/pkg/azdext/telemetry_grpc.pb.go: Generated file
Suppressed comments (5)
cli/azd/internal/grpcserver/telemetry_service.go:130
- azd-code-reviewer: This does not prove an official-registry install.
SourceManager.Addreserves onlybundle(cli/azd/pkg/extensions/source_manager.go:89-111), so after removing the default source a user can register any URL under the nameazd; installs from it still persistSource == "azd"and can send unreviewed text into the shared pipeline. Bind eligibility to an immutable canonical registry identity or signed registry membership rather than the user-configurable source name.
if !strings.EqualFold(extension.Source, extensions.MainRegistryName) {
cli/azd/internal/grpcserver/telemetry_service.go:146
- azd-code-reviewer: The unofficial-source return occurs before this per-attribute validation, so local/dev installs get
Accepted: falseeven for empty or oversized keys and values. This contradicts the documentedInvalidArgumentcontract and means malformed integrations are only discovered after publishing. Validate the complete request before applying the source eligibility drop, and add an unofficial-source plus malformed-attribute test.
for key, value := range req.Attributes {
if key == "" || len(key) > maxUsageKeyLength {
cli/azd/internal/grpcserver/telemetry_service.go:153
- azd-code-reviewer: These limits are documented and reported as characters, but Go's
lencounts UTF-8 bytes. Since the API also imposes no charset restriction, an event name, key, or value within the advertised Unicode-character limit can still be rejected. Either count the exporter-defined character units or define the limits as bytes consistently across the proto, docs, errors, and boundary tests.
if len(value) > maxUsageValueLength {
return nil, status.Errorf(codes.InvalidArgument,
"attribute values must be at most %d characters", maxUsageValueLength)
docs/specs/metrics-audit/telemetry-schema.md:238
- azd-code-reviewer: This authoritative schema omits whether dynamic values are hashed and whether they are measurements. The repository telemetry rules (
cli/azd/AGENTS.md:277-279) require those properties for every new field; state them explicitly so privacy reviewers and downstream consumers do not have to infer them from other documents.
cli/azd/internal/tracing/fields/fields.go:1177 - The telemetry checklist requires every new field to be added to the central field-contract test. Add
ExtensionEventtoTestTelemetryFieldConstantsincli/azd/cmd/telemetry_test.go; the service test checks emitted behavior but does not independently protect the documented schema key from renaming.
ExtensionEvent = AttributeKey{
Key: attribute.Key("extension.event"),
Classification: SystemMetadata,
Purpose: FeatureInsight,
- Files reviewed: 34/36 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Jon Gallant (jongio)
left a comment
Jon Gallant (jongio)
left a comment
Member
There was a problem hiding this comment.
Still open from the 2026-07-31 round: the source alias check at telemetry_service.go:130. SourceManager.Add only reserves bundle (pkg/extensions/source_manager.go:93), so the default azd source can still be removed and re-added pointing at another registry, and an extension installed from it passes the gate.
Verified the two new commits against my earlier comments. traceContextPairs now round-trips both environment values through the propagator, so a malformed TRACEPARENT or TRACESTATE is dropped instead of reaching gRPC metadata. TraceparentKey and TracestateKey are the exact header names the W3C propagator reads (pkg/azdext/context.go:14-15), so extraction resolves, and unsampled traces still propagate because IsValid() doesn't depend on the sampled bit.
Both schema files are byte identical to the merge base again: extension.schema.json at 237 CRLF and 42 bare LF, registry.schema.json at 279 and 10, matching 69b43d1 on both ends.
Left a note on the concurrency thread.
Jon Gallant (jongio)
left a comment
Jon Gallant (jongio)
left a comment
Member
There was a problem hiding this comment.
Re-verified against the merged HEAD. The only new commit is a clean main merge with no change to the PR's own files, so my earlier findings carry over unchanged.
Still open from the 2026-07-31 round: the source alias check at telemetry_service.go:130. I re-read source_manager.go at this HEAD and it still holds. Add reserves only bundle (line 93) and Remove doesn't protect azd. List restores the default source only when the whole extension.sources key is absent, so once any other source exists, removing azd and re-adding that name pointed at another registry succeeds, and an extension installed from it passes the gate.
Four new items, all verified against the code at this HEAD:
- MEDIUM
telemetry_service.go:130: the source gate short-circuits per-attribute validation, so the documented local-development workflow can't surface attribute bugs - MEDIUM
extension-telemetry.md:124: the KQL example queries the wrong table and returns no rows - LOW
extension-telemetry.md:59: the bounds are byte counts, not character counts - LOW
fields.go:1175:extension.eventhas no literal-key assertion in the telemetry contract test
Hui Miao (huimiu)
force-pushed
the
hui/telemetry-deployment-mode
branch
from
August 4, 2026 05:25
4204517 to
3fe213c
Compare
Hui Miao (huimiu)
force-pushed
the
hui/telemetry-deployment-mode
branch
from
August 4, 2026 05:26
3fe213c to
1817e20
Compare
Jon Gallant (jongio)
approved these changes
Aug 4, 2026
Jon Gallant (jongio)
left a comment
Jon Gallant (jongio)
left a comment
Member
There was a problem hiding this comment.
A few things from this pass at 1817e20. None of them blocks merge.
MEDIUM
extensions/microsoft.azd.demo/internal/cmd/telemetry.go:46returns an error from the command whenReportUsagefails, and the guide that links to this file as the worked example says the opposite. Inline.docs/specs/metrics-audit/telemetry-schema.md:253still describes a capability and a stored declaration. Neither exists in this design. Inline.docs/extensions/extension-telemetry.md:124still queriesdependencies.SpanToEnvelopesends every span throughspanToRequestData(internal/telemetry/appinsights-exporter/span_to_envelope.go:19-33,51), and that is the only exporter in the tree, soext.usagelands inrequestsand this query returns no rows. Suggestion is on the open thread.- The bounds tables in
extension-telemetry.md:59andtelemetry-schema.mdstill say characters.telemetry_service.go:100,146,151uselen(), which counts UTF-8 bytes, so a 128-character non-ASCII event name is rejected at roughly a third of the documented limit. Either document them as byte limits or switch the three checks toutf8.RuneCountInString.
LOW
extensions/microsoft.azd.demo/internal/cmd/telemetry.go:42always reportsunder_1s. Inline.- Validation ordering is better this round: the event name and the attribute count are now checked before the source gate. The per-key and per-value bounds are still inside the loop below the gate, so a
--source devinstall sending a 200-character key still getsAccepted: falserather thanInvalidArgument. Moving the loop above the gate closes the remainder. pkg/extensions/validate_registry.go:267still takes anextensionIdit never reads.unparamisn't in the enabled linter set, so CI won't catch it.
The test-validity problem I raised earlier is gone. main_test.go installs one provider for the package, so the recorder is actually wired to tracing.Start, and RecordsNoSpanWhenRejected, RejectedCallDoesNotSpendBudget, and CapHoldsUnderConcurrentCalls now assert on real spans. The TRACEPARENT fallback round-trips through the propagator, so a malformed environment value gets dropped instead of failing every extension RPC. go build ./... is clean and go test ./internal/grpcserver/... ./pkg/azdext/... ./internal/tracing/... ./pkg/extensions/... passes locally.
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
Legacy source recovery, trace-context trust, and public telemetry semantics need correction before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (4)
cli/azd/pkg/extensions/source_manager.go:132
- azd-code-reviewer: This unconditional reservation leaves legacy
azdsource overrides with no CLI repair path. Previous releases allowed users to add or remove a custom source under this name, but after this change a polluted entry cannot be removed, adding the official entry returnsErrSourceExists, andCreateSourcerefuses to use the polluted entry. Allow removal/replacement when the storedazdentry is not official, or migrate existing entries to the official configuration.
cli/azd/internal/grpcserver/trace_context.go:91 - azd-code-reviewer: Only the trace ID is compared before the caller's entire
traceparentandtracestatereplace the signed values. An authenticated extension can therefore clear the sampled flag (making host spans non-recording whileReportUsagestill returnsAccepted: true) or inject caller-controlled tracestate into exported host spans. PreserveTraceFlagsandTraceStatefromtokenSpanContextand accept at most the caller's child span ID, or require those signed fields to match before using the metadata.
incomingContext := propagation.TraceContext{}.Extract(context.Background(), carrier)
incomingSpanContext := trace.SpanContextFromContext(incomingContext)
if incomingSpanContext.IsValid() &&
incomingSpanContext.TraceID() == tokenSpanContext.TraceID() {
return propagation.TraceContext{}.Extract(ctx, carrier)
cli/azd/grpc/proto/telemetry.proto:36
- azd-code-reviewer:
Accepteddoes not actually report whether an event was recorded. The service returnstrueafter only the source and budget checks, even when telemetry is disabled or initialization failed and the global tracer is non-recording; the demo then prints that anext.usagespan was recorded. Define this as “accepted by the host” throughout the proto, SDK docs, and demo output, or make the implementation returnfalsewhen no span can be recorded.
// Accepted reports whether the host recorded the event. A false value is
// not an error: the extension was not installed from the official azd
// registry, or the per-invocation event budget is already spent. Callers
// should not branch on it.
cli/azd/internal/tracing/events/events.go:38
- azd-code-reviewer: The contributor telemetry reference at
cli/azd/docs/tracing-in-azd.md:76-104still presents its table as the existing event taxonomy but omitsext.usage,extension.event, and dynamicext.*attributes, and still describesextension.sourceas upgrade-only. Add this event and its fields there so the code-level telemetry guide stays consistent with the new event and the other updated audit/reference documents.
// ExtensionUsageEvent carries one usage event an extension reported
// through the telemetry service. The host stamps the extension's
// identity and namespaces every attribute the extension supplied.
ExtensionUsageEvent = "ext.usage"
- Files reviewed: 48/48 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
Author
|
Copilot resolve the merge conflicts in this pull request |
Co-authored-by: huimiu <107838226+huimiu@users.noreply.github.com>
Contributor
Resolved the merge conflict by merging |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
cli/azd/internal/grpcserver/telemetry_service.go:116
- azd-code-reviewer: This creates a third
Accepted: falseoutcome: source-config load or parse failures. The proto and author guides sayfalseonly means an unofficial source or exhausted budget, while other host verification failures are gRPC errors. Either returncodes.Internalhere or document this additional drop reason everywhere the response contract is described.
if err != nil {
log.Printf(
"telemetry: failed to verify source %q for %s: %v",
extension.Source, extension.Id, err)
return &azdext.ReportUsageResponse{Accepted: false}, nil
cli/azd/internal/grpcserver/telemetry_service.go:149
- azd-code-reviewer: Every call after the budget is exhausted still emits this log entry. A buggy loop can therefore create unbounded debug-log I/O even though telemetry spans are capped, especially in long-lived MCP sessions. Log only the first transition past the limit and silently reject later calls.
if s.recorded.Add(1) > maxUsageEventsPerInvocation {
log.Printf(
"telemetry: dropping usage event from %s, limit of %d reached",
extension.Id, maxUsageEventsPerInvocation)
docs/specs/metrics-audit/telemetry-schema.md:231
- azd-code-reviewer: The authoritative Extensions field table still omits the new host-owned
extension.eventfield and still describesextension.sourceas upgrade-only. Referencing these fields in prose does not provide the required classification/purpose inventory. Addextension.eventto the table and update the source note forext.usage.
docs/specs/metrics-audit/telemetry-schema.md:246 - azd-code-reviewer: This overstates the provenance guarantee.
ExtensionClaimssigns only the subject ID;ReportUsagereads the version and source from the installed record. Describe the ID as bound by the signed subject and the version/source as local installed-record values so the authoritative privacy schema matches the implementation.
cli/azd/internal/tracing/fields/fields.go:1192 - This new telemetry field and event trigger the repository's privacy-review process, but the PR description does not include the required Telemetry Change Checklist from
docs/specs/metrics-audit/privacy-review-checklist.md(cli/azd/AGENTS.md:302-303). Add the completed checklist and privacy-review status to the PR description before merging.
ExtensionEvent = AttributeKey{
Key: attribute.Key("extension.event"),
Classification: SystemMetadata,
Purpose: FeatureInsight,
}
Comment on lines
+129
to
+132
| if strings.EqualFold(name, MainRegistryName) { | ||
| return fmt.Errorf( | ||
| "'%s' is reserved and cannot be removed, %w", | ||
| MainRegistryName, ErrSourceReserved) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cli/azd/pkg/extensions/source_manager.go:132
- azd-code-reviewer: This unconditional reservation leaves configurations created by older azd versions unrecoverable through the CLI. A user could previously point
azdat another URL; after upgrading,CreateSourcerejects that entry,Removealways refuses it, andAddcannot restore the official source because the name already exists. It also contradicts the current docs that say the source can be removed and re-added. Allow removal/reset when the storedazdentry is not official (while protecting the official entry), and update the source-management docs.
cli/azd/internal/grpcserver/trace_context.go:92 - azd-code-reviewer: The caller can keep the signed trace ID but change the
trace-flagsbyte to00; this condition accepts it, and the default parent-based sampler then makes host spans non-recording. That lets an extension suppressext.usageand other gRPC-server spans while still passing the signed-trace check. Preserve or require the signed claims' trace flags when accepting a child context, and add an unsampled-same-trace test.
if incomingSpanContext.IsValid() &&
incomingSpanContext.TraceID() == tokenSpanContext.TraceID() {
return propagation.TraceContext{}.Extract(ctx, carrier)
cli/azd/internal/grpcserver/telemetry_service.go:158
- azd-code-reviewer:
Acceptedis documented as meaning the host recorded the event, but this always returns true even when telemetry is disabled or initialization failed. In those casesmain.goleaves the global provider as a no-op,tracing.Startcreates a non-recording span, and the demo falsely prints that anext.usagespan was recorded. Checkspan.IsRecording()(without spending budget) and return false, or redefine the response and user-facing documentation to mean admission rather than recording.
_, span := tracing.Start(ctx, events.ExtensionUsageEvent)
span.SetAttributes(attributes...)
span.End()
return &azdext.ReportUsageResponse{Accepted: true}, nil
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.
Adds
TelemetryService.ReportUsageto the extension gRPC API so extensions can emit named usage events with bounded attributes throughazd's existing telemetry pipeline.Why this is needed
Extensions run as separate processes, so core telemetry currently shows that an extension ran but not which feature was used. Reusing the host pipeline avoids per-extension exporters and keeps consent, transport, identity, and privacy controls centralized.
Approach
ext.usagespan per call;event_namebecomesextension.eventand caller attributes becomeext.<key>.azdinvocation.Accepted: falsefor non-official-source installs or exhausted budget without failing the extension command.azdregistry source as the admission gate. This is an install-source control, not cryptographic first-party proof; extensions admitted to that registry are eligible by design.azd demo telemetryin demo extension v0.8.0, plus the extension guide, ADR, schema, telemetry reference, matrix, and privacy checklist updates.Validation
azd demo telemetry --no-promptwith the exact PR bundle mapped to an isolatedazdsourcePASS—Usage event recorded as an ext.usage span.azd demo telemetry --debug --no-promptwith bundle sourcePASS— exits 0; debug log confirms the event was dropped because source wasbundleazd x publish --registry ... --artifacts ... --no-promptPASS— publish succeeds and existing MCP config is preservedazd extension source validate ... --strict --no-promptPASS—Registry validation passed.All 11 recorded E2E scenarios passed. Telemetry upload was disabled with
AZURE_DEV_COLLECT_TELEMETRY=no, so this validates the host RPC, acceptance/drop behavior, and span path without sending data to the backend.