Emit installed extensions in telemetry for all commands#6918
Conversation
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 2 |
| Medium | 4 |
| Low | 1 |
| Total | 7 |
Key Findings
- Tests don't verify span attributes — the
WithInstalledExtensionstest only checksListInstalled()works, not that span attributes were actually emitted with correct sorted values. - Potential nil pointer dereference — if config contains a null extension entry,
installed[id].Versionwill panic. Telemetry code must never crash. - Use
slices.Sorted(maps.Keys())— the manual key extraction + sort is a 5-line pattern with a 1-line equivalent used 30+ times in this codebase. - Namespace mismatch — new keys use
extensions.*(plural) while existing keys useextension.*(singular).
What Looks Good ✅
- PII is safe: Only extension IDs (regex-validated package identifiers) and semver versions are emitted — no
Path, no user data. ClassificationSystemMetadata/FeatureInsightis correct. - No performance impact:
ListInstalled()operates on in-memory cached config. Slice ops on 0-5 elements. Telemetry upload is async via background goroutines. - Good defensive coding: Nil guard on
extensionManager, early return on error/empty, pre-allocated slices. - No concurrency concerns: CLI is single-command-per-process.
Overall Assessment: Approve with suggestions
d9966b7 to
c3b79fc
Compare
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Add new telemetry fields `extensions.installed.ids` and `extensions.installed.versions` to capture the list of installed extensions on every command span. This enables tracking which extensions users have installed, even for non-extension commands like `azd up` or `azd deploy`. Co-authored-by: JeffreyCA <9157833+JeffreyCA@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c3b79fc to
442fdc4
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #6917 by adding telemetry for installed extensions on all azd commands, not just extension commands. Previously, extension.id and extension.version were only emitted for ext.run span events. Now, every command span includes a sorted extension.installed string-slice attribute with id@version entries for each installed extension.
Changes:
- New
ExtensionsInstalledattribute key (extension.installed) added to thefieldspackage, consistent with existingExtensionId/ExtensionVersionkeys. - New
setInstalledExtensionsAttributeshelper onTelemetryMiddlewarethat callsListInstalled, builds sortedid@versionentries, and emits them; guarded for nil manager and empty/nil results. - New tests covering extensions present, no extensions installed, and nil manager cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cli/azd/internal/tracing/fields/fields.go |
Adds ExtensionsInstalled AttributeKey under the existing extension.* namespace |
cli/azd/cmd/middleware/telemetry.go |
Adds setInstalledExtensionsAttributes method and calls it from Run for all commands |
cli/azd/cmd/middleware/telemetry_test.go |
Adds three sub-tests for the new attribute emission, using mocktracing.Span |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
wbreza
left a comment
There was a problem hiding this comment.
Code Review: PR #6918 — Emit installed extensions in telemetry
Prior Review Resolution
| Prior Finding | Status |
|---|---|
| [High] Tests don't verify span attributes (wbreza) | ✅ Resolved |
| [Medium] Nil pointer dereference (wbreza) | ✅ Resolved |
[Medium] Use slices.Sorted(maps.Keys()) (wbreza) |
✅ N/A (refactored) |
| [Medium] Namespace mismatch (wbreza) | ✅ Resolved |
| [Medium] Missing edge case tests (wbreza) | |
| [Medium] Empty entries guard (JeffreyCA) | ✅ Addressed (intentional design) |
Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low | 4 |
| Total | 5 |
✅ What Looks Good
- PII-safe telemetry: Only extension IDs and semver versions emitted.
SystemMetadata/FeatureInsightclassification is correct. - Good defensive coding: Nil guards on
extensionManagerandext, early returns on error, pre-allocated slices. - Intentional empty-slice semantics: Emitting
[]vs. no attribute distinguishes "no extensions installed" from "couldn't check" — good telemetry practice. - Clean namespace:
extension.installedproperly follows the singularextension.*convention.
Overall Assessment: Approve with suggestions — all prior Critical/High findings resolved. Remaining items are minor quality improvements.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JeffreyCA
left a comment
There was a problem hiding this comment.
Addressed all four outstanding comments:
Missing ListInstalled error path test — Added WithListInstalledError subtest using malformed config ("not-a-map") to exercise the err != nil guard.
Swallowed error — Added log.Printf before the early return, consistent with existing usage on line 66.
Fragile index-based attribute lookup — Extracted findAttribute helper; all subtests now use key-based lookup consistently.
slices.Sorted(maps.Keys()) — Adopted the established codebase pattern, replacing manual append+sort.
|
/azp run azure-dev - cli |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/check-enforcer override |
--------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: JeffreyCA <9157833+JeffreyCA@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Fixes #6917
Extension telemetry (id, version) is currently only emitted for extension commands (
ext.run). For standard commands likeazd uporazd deploy, we have no visibility into what extensions the user has installed—relevant when extensions hook into core command lifecycles.Changes
Extensions are emitted as a sorted string slice of
id@versionpairs. OpenTelemetry attributes don't support nested objects, so a flat list is a natural fit. Theid@versionformat keeps each entry self-contained, is trivial to split in queries, and mirrors conventions in package managers like npm and Go modules.Example span output for
azd upwith two extensions installed:{ "Name": "cmd.up", "Attributes": [ { "Key": "extension.installed", "Type": "STRINGSLICE", "Value": ["microsoft.azd.ai@1.2.0", "microsoft.azd.demo@0.5.0"] } ] }When no extensions are installed, an empty slice is emitted:
{ "Name": "cmd.up", "Attributes": [ { "Key": "extension.installed", "Type": "STRINGSLICE", "Value": [] } ] }Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.