vscode telemetry: emit Aspire wire names without losing VS Code safeguards - #18562
Conversation
…e names
Replace the implicit `microsoft-aspire.aspire-vscode/` extension-id prefix
that VS Code's `TelemetryLogger` adds to every telemetry event with an
explicit, dashboard-native namespace:
* Extension-emitted events → `aspire/vscode/<area>/<event>`
* Dashboard passthrough → `aspire/dashboard/<route>`
Route every send through `reporter.sendDangerousTelemetryEvent` /
`sendDangerousTelemetryErrorEvent`. Those bypass the prefix-applying
`TelemetryLogger` but also bypass the reporter's built-in opt-in gate, so
gating is enforced explicitly via `reporter.telemetryLevel`:
* `'all'` → regular and error events emit
* `'error'` → only error events emit
* `'crash'`/`'off'` → nothing emits
Renames cover the registry (`extension/src/utils/telemetryRegistry.ts`),
all 22 callsites, the data classification catalog
(`extension/telemetry.json`), and every test fake. Existing event
schemas / property unions are unchanged; only the wire names move.
Adds proof tests in `extension/src/test/telemetry.test.ts` pinning:
* `sendTelemetryEvent` routes through `sendDangerousTelemetryEvent`
with the registry-declared name and no extension-id prefix
* `sendTelemetryErrorEvent` routes through the dangerous error channel
* dashboard-namespaced events emit verbatim
* `'off'` / `'crash'` suppress everything; `'error'` allows only error
events
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 18562Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 18562" |
There was a problem hiding this comment.
Pull request overview
This PR removes the implicit microsoft-aspire.aspire-vscode/ extension‑id prefix that VS Code's TelemetryLogger prepends to every Aspire VS Code extension telemetry event, replacing it with two explicit dashboard‑native namespaces: extension events become aspire/vscode/<area>/<event> and dashboard passthrough events become aspire/dashboard/<route>. This aligns the extension's wire names with what other Aspire dashboard hosts (Visual Studio, C# Dev Kit) emit. The prefix is bypassed by routing all sends through sendDangerousTelemetryEvent / sendDangerousTelemetryErrorEvent, and because those "dangerous" APIs skip the reporter's built-in opt-in gate, the PR re-implements the gate explicitly via reporter.telemetryLevel.
Changes:
- Route all telemetry through the dangerous send APIs and add explicit telemetry-level gating (
'all'for usage events;'all'/'error'for error events; suppress at'crash'/'off'). - Rename every registry event name, every production call site, and every
telemetry.jsonclassification key to the newaspire/vscode/...andaspire/dashboard/...wire names. - Update all test fakes to expose
telemetryLeveland record dangerous sends, and add pinning tests proving names emit unprefixed via the dangerous channel plus level-gating behavior.
Show a summary per file
| File | Description |
|---|---|
| extension/src/utils/telemetry.ts | Adds dangerous-send routing, getCurrentTelemetryLevel(), and explicit usage/error gating |
| extension/src/utils/telemetryRegistry.ts | Renames event schema keys to final wire names; documents naming convention and known dashboard names |
| extension/telemetry.json | Renames all 22 classification entity keys to the new wire names |
| extension/src/extension.ts | Updates extension/activated call site to aspire/vscode/... |
| extension/src/utils/workspace.ts | Updates cli/availability call site |
| extension/src/utils/meaningfulEngagement.ts | Updates engagement/active call site |
| extension/src/utils/appHostDiscovery.ts | Updates apphost/discovery/result call site |
| extension/src/services/AppHostLaunchService.ts | Updates apphost/launch/result call sites and typed properties |
| extension/src/server/interactionService.ts | Updates dashboard launch resolved/migration call sites |
| extension/src/views/AppHostsViewTelemetry.ts | Updates runningapphostsview/shown call site |
| extension/src/debugger/AspireDebugSession.ts | Updates debug/apphost/start and debug/apphost/end call sites |
| extension/src/dcp/AspireDcpServer.ts | Updates debug/runsession/start / end call sites |
| extension/src/dcp/DashboardTelemetryPassthrough.ts | Updates dashboard passthrough event names and typed properties to aspire/dashboard/... |
| extension/src/test/telemetry.test.ts | Adds dangerous-channel and level-gating pinning tests; reworks fake reporter |
| extension/src/test/telemetryInventory.test.ts | Sets the entity prefix to empty to match unprefixed wire names |
| extension/src/test/dashboardTelemetryRoutes.test.ts | Updates fake reporter and route name assertions |
| extension/src/test/aspireDebugSession.test.ts | Updates fake reporter and debug event name assertions |
| extension/src/test/appHostsViewTelemetry.test.ts | Updates fake reporter and event name assertion |
| extension/src/test/appHostLaunchService.test.ts | Updates fake reporter and launch result assertions |
| extension/src/test/appHostDiscovery.test.ts | Updates fake reporter and discovery result assertions |
| extension/src/test/meaningfulEngagement.test.ts | Updates fake reporter and engagement event assertion |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 0
- Review effort level: Medium
Fail test telemetry immediately if the regular sendTelemetryEvent path is used, proving the extension bypasses VS Code's extension-id prefix channel. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add explicit property value sanitization and restore common telemetry dimensions before using the dangerous telemetry send path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep dashboard telemetry enabled for VS Code errors-only telemetry so dashboard faults can still reach the error channel, and expand dangerous-path redaction for URI-prefixed and space-containing home paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Merged main in — extension suite green (1224 passing, lint clean). Note for whoever reviews this: #18602 currently contains all 21 files from this PR plus 9 more. I'd rather land this one first and then rebase #18602 down to just the attach-debugger change, so the telemetry prefix rename gets reviewed on its own. |
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (1)
extension/src/utils/telemetry.ts:190
- This replacement sanitizer leaves quoted credential assignments unchanged: for example,
--token="secret-value"andpassword='secret value'do not match because the value character class excludes quotes at its first position. Since the dangerous path no longer runsTelemetryLogger.cleanData(), those common forms are sent verbatim. Handle optional quoted values (including spaces) and add regression coverage for both quote styles.
.replace(/\b(password|passwd|pwd|token|secret|api[_-]?key|key)(\s*[:=]\s*)[^&\s"',;}]+/gi, '$1$2<redacted>');
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Medium
…gate, error-envelope wording Implements unresolved code-review feedback shared with PR microsoft#18602: - Restore URL and generic filesystem-path redaction in the manual sanitizer that bypasses TelemetryLogger.cleanData(), so private hosts (https://storage.example/...) and workspace paths (/mnt/customer/...) no longer leak across the JSON boundary. - Preserve VS Code's logging-only telemetry mode before the dangerous direct-send path via a behavioral probe (no public API exposes it), with a fake-reporter exemption for test-injected reporters. - Correct the "error envelope" wording: both dangerous APIs emit EventData/customEvent in @vscode/extension-telemetry 1.5.1; only the opt-in gate and event name differ, not an ExceptionData envelope. Shared telemetry files are kept byte-identical with PR microsoft#18602. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5a388bf9-6873-459a-953b-386dc7157dc2
…r-envelope wording, assembly-name handling Implements unresolved code-review feedback shared with PR microsoft#18562: - Restore URL and generic filesystem-path redaction in the manual sanitizer that bypasses TelemetryLogger.cleanData(), so private hosts (https://storage.example/...) and workspace paths (/mnt/customer/...) no longer leak across the JSON boundary. - Preserve VS Code's logging-only telemetry mode before the dangerous direct-send path via a behavioral probe (no public API exposes it), with a fake-reporter exemption for test-injected reporters. - Correct the "error envelope" wording: both dangerous APIs emit EventData/customEvent in @vscode/extension-telemetry 1.5.1; only the opt-in gate and event name differ, not an ExceptionData envelope. - AssemblyName attach hint (18602): accept valid names containing spaces (e.g. "My Service"), and reframe the raw-.csproj read as a best-effort hint, not a source of truth. Confirmed the backchannel exposes no evaluated AssemblyName / child PID (only the dotnet run launcher pid, dotnet path, and project.path), so an out-of-file AssemblyName from Directory.Build.props/imports cannot be resolved here; documented the runtime-confirmed limitation and added a regression test pinning the filename fallback. When the project file DOES customize AssemblyName to a value we cannot evaluate (e.g. an MSBuild expression), fail explicitly with an actionable message instead of silently attaching to a wrong process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5a388bf9-6873-459a-953b-386dc7157dc2
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
extension/src/utils/telemetry.ts:395
- The replacement sanitizer still leaks the common whitespace form of CLI credentials. For example,
--password secretand--token secretdo not match this expression because it requires:or=, so they reach the dangerous sender unchanged; the bypassed VS CodecleanData()generic-secret rule redacted these values. Treat whitespace as a credential separator too (and add a regression case) before relying on this path as the cleaner replacement.
.replace(/\b(password|passwd|pwd|token|secret|sig|api[_-]?key|client[_-]?secret|account[_-]?key|shared[_-]?access[_-]?key|sharedaccesskey|connection[_-]?string|connectionstring|key)(\s*[:=]\s*)(?:(["'])([^"']*)\3|([^&\s"',;}]+))/gi, (_match: string, key: string, separator: string, quote: string | undefined) => `${key}${separator}${quote ?? ''}<redacted>${quote ?? ''}`)
extension/src/utils/telemetry.ts:480
- This intentionally exempts every relative slash-separated value, but the bypassed
cleanData()file-path matcher also removed relative paths. A dashboard bundle value such ascustomer/project/file.csnow leaves through the dangerous sender unchanged, even though the passthrough treats its input as a hostile boundary and promises not to report workspace content. Preserve only the known structural route fields (such asdashboard_event_name) and redact relative paths in free-form values.
// POSIX absolute paths outside the user's home tree. Require at least two
// segments so a bare `/` or a lone `/word` (often a route or flag, not a path)
// is left intact, and require a leading delimiter so relative event names are
// not matched. `/Users/...` and `/home/...` are already redacted above.
.replace(/(^|[\s"'=(:,|])\/(?!Users\/|home\/)[^/\s"'<>:;,|]+(?:\/[^/\s"'<>:;,|]*)+/g, (_match: string, prefix: string) => `${prefix}<path>`);
extension/src/utils/telemetry.ts:360
- Re-serializing after replacement can expand a dashboard bundle beyond
MAX_BUNDLE_CHARS, whose limit was enforced before this sanitizer ran. Repeated short values such astoken=xexpand substantially totoken=<redacted>; a bundle near 8192 characters can therefore exceed the telemetry property limit and be truncated/rejected downstream, makingdashboard_propertiesinvalid JSON. Sanitize beforeserializeBundle, or reapply the bundle budget while preserving a valid JSON envelope.
return JSON.stringify(sanitizeJsonNode(parsed, preserveGuids));
- Files reviewed: 21/21 changed files
- Comments generated: 1
- Review effort level: Medium
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
…ot quotes The manual sanitizer stopped URL and `authorization: bearer` matches at the first quote, which truncated the match and leaked the tail: https://private.example/?q="customer"&account=alice -> https://<redacted>"customer"&account=alice (&account=alice leaked) authorization: bearer abc"def -> authorization: bearer <redacted>"def (def leaked) The quote-stop was justified as protecting JSON string boundaries, but JSON bundles are already sanitized structurally upstream (trySanitizeJsonBundle), so by the time a value reaches these passes a quote is a literal character in free-form text. Redact through the next whitespace, matching VS Code's cleanData(). `\S` remains a superset of the old class, so it still absorbs an already-inserted `<redacted>` placeholder. The filesystem-path passes are unaffected (their global regexes re-match after a quote: `<path>"<path>`). Adds regression tests for the URL and bearer cases plus a JSON-bundle URL that must still round-trip as valid JSON. Updates two existing quoted-secret tests whose URL tail is now fully redacted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ec1c2b63-332f-406a-b654-de83229b10c9
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (6)
extension/src/utils/telemetry.ts:395
- The replacement sanitizer still leaks common space-separated credentials. For example,
--token secret,password secret, andsignature=secretdo not match this expression, while the bypassed VS CodecleanData()generic-secret check redacts them. A dashboard value such asAspire.Dashboard.UserAgent: "--token secret"therefore reaches the dangerous sender unchanged. Extend this pass to cover whitespace-delimited assignments and the omitted generic-secret keywords, with regression tests for those forms.
.replace(/\b(password|passwd|pwd|token|secret|sig|api[_-]?key|client[_-]?secret|account[_-]?key|shared[_-]?access[_-]?key|sharedaccesskey|connection[_-]?string|connectionstring|key)(\s*[:=]\s*)(?:(["'])([^"']*)\3|([^&\s"',;}]+))/gi, (_match: string, key: string, separator: string, quote: string | undefined) => `${key}${separator}${quote ?? ''}<redacted>${quote ?? ''}`)
extension/src/test/telemetry.test.ts:227
- Remove the
Item 1:label. It refers to an internal review checklist rather than durable code context; keep the current-state rationale that follows it.
// Item 1: VS Code's `TelemetryLogger.cleanData()` (bypassed on the dangerous send path)
extension/src/test/telemetry.test.ts:512
- Remove the
Item 2:label. It refers to an internal review checklist rather than durable code context; keep the current-state rationale that follows it.
// Item 2: the `sendDangerous*` APIs bypass `TelemetryLogger`, which is what normally enforces
extension/src/test/telemetry.test.ts:163
- Remove the
Item 1:label. It refers to an internal review checklist rather than durable code context; keep the current-state rationale that follows it.
This issue also appears on line 227 of the same file.
// Item 1: the manual sanitizer replaces the `TelemetryLogger.cleanData()` that the dangerous
extension/src/test/telemetry.test.ts:262
- Remove the
Item 2:label. It refers to an internal review checklist rather than durable code context; keep the current-state rationale that follows it.
This issue also appears on line 512 of the same file.
// Item 2: `JSON.stringify({ x: 'token="secret"' })` is `{"x":"token=\"secret\""}`. A text-level
extension/src/test/telemetry.test.ts:278
- Remove the
Item 4:label. It refers to an internal review checklist rather than durable code context; keep the current-state rationale that follows it.
// Item 4: VS Code's `cleanData()` treats UNC paths (`\\server\share\...`) as absolute paths.
- Files reviewed: 21/21 changed files
- Comments generated: 0 new
- Review effort level: Medium
…onto the attach PR microsoft#18602 sits logically on top of microsoft#18562, but the branches diverged after the shared telemetry commits, so the dangerous-send sanitizer here was still the weaker pre-hardening variant. Bring it to parity with microsoft#18562's final, privacy-safe behavior while keeping the attach-debugger work intact. - Redact standalone credential tokens (Google API keys, JWT/Entra tokens, Slack tokens, GitHub classic and fine-grained PATs) that are neither `key=value` assignments nor `Bearer` values, mirroring VS Code's `userDataRegexes`. Without this the dangerous path emitted them verbatim. - Sanitize JSON bundle values structurally (parse, sanitize leaf strings, re-stringify) instead of regexing raw JSON text, which corrupted the JSON and leaked secrets nested inside JSON-escaped quotes. - Redact UNC network paths (`\\server\share\...`), including the doubled separators of a JSON-encoded bundle. The drive-letter-only Windows rule let private server/share/workspace names through. - Redact URLs and `authorization: bearer` values through the next whitespace rather than stopping at a quote, which previously truncated the match and left the tail (`&account=alice`) in the payload. Safe now that JSON bundles are sanitized structurally. - Drop `aspire/dashboard/aiassistant/feedback` from the fallback event-name list; `TelemetryEventKeys.cs` declares only five names. Kept from this branch: the structural-correlation-ID test proving `operation_id` / `dashboard_correlated_with` round-trip verbatim while a GUID in a free-form value is still redacted, and the checklist-free test comments. Also tightened the non-zero-exit comment in `AspireDcpServer.ts` to match the verified `@vscode/extension-telemetry` 1.5.1 contract: the dangerous error API calls the same `sendEventData`, so the distinction is the error-level opt-in gate and event name, not a different App Insights envelope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c73cc7c-e7e3-4b37-8a3f-245b16148b2e
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c73cc7c-e7e3-4b37-8a3f-245b16148b2e
aspire/vscode/* and aspire/dashboard/* wire names (bypass extension-id prefix)There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
extension/src/utils/telemetryRegistry.ts:227
- This weakens the registry's privacy boundary: every event property now accepts
TelemetryTrustedValue, so any producer can bypass VS Code's PII/path cleaning for arbitrary fields. Only the allowlisteddashboard_event_nameneeds that escape hatch; keep all other event/common properties string-only so accidental trusted wrapping is rejected by the type checker.
Partial<Record<TelemetryEventSchema[E]['properties'] | CommonTelemetryProperty, TelemetryPropertyValue>>;
extension/src/dcp/DashboardTelemetryPassthrough.ts:1055
- This contradicts the PR's stated dashboard contract: instead of preserving and bounding the raw
dashboard_event_name, every valid dashboard event outside this hard-coded set becomesother. That drops the native signal whenever a supported older/newer dashboard emits another event and requires an extension update for each addition. Either preserve a sanitized/clampedaspire/dashboard/...value as described, or explicitly change the contract and keep the allowlist/documentation synchronized.
function dashboardEventNameProperty(value: unknown): TelemetryPropertyValue {
const eventName = typeof value === 'string' ? value : '';
return KNOWN_DASHBOARD_EVENT_NAMES.has(eventName)
? new vscode.TelemetryTrustedValue(eventName)
: 'other';
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Medium
Ella Hathaway (ellahathaway)
left a comment
There was a problem hiding this comment.
Looks good overall. The telemetry bridge preserves VS Code's safeguards while emitting the intended Aspire wire names. I left one non-blocking comment to align the registry guidance with the implemented dashboard event allowlist.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12028902-b78a-47d6-9c57-2f31289f5b66
There was a problem hiding this comment.
Review details
Suppressed comments (1)
extension/src/dcp/DashboardTelemetryPassthrough.ts:1028
- This pre-check misses a secret keyword at the end of a leaf. After serialization, VS Code tests the whole JSON string, so the closing quote supplies the required non-alphanumeric delimiter; for example, a value of
monkeydoes not match here but the final"monkey"matches VS Code'skey[^A-Za-z0-9]pattern. VS Code then replaces the entiredashboard_propertiesvalue with a redaction marker, so it is no longer parseable JSON. Include the serialized closing delimiter in this probe and add a regression case for a trailing keyword.
/(key|token|sig|secret|signature|password|passwd|pwd|android:value)[^A-Za-z0-9]/i.test(boundedValue) ||
- Files reviewed: 22/22 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
636b708
into
microsoft:main
Summary
aspire/vscode/*and dashboard passthrough events as fixedaspire/dashboard/*wire names.TelemetryLoggerpayload into the existing reporter transport.other, including near misses.telemetry.json.The inner reporter no longer owns extension-host exceptions; the outer VS Code logger does. This avoids duplicate exception events while preserving the existing automatic exception path.
Testing
corepack yarn run lintcorepack yarn run packageThe focused coverage includes exact wire names, telemetry-level gating, common-property forwarding, one-time automatic exceptions, structural string arrays, older shipped dashboard event names, and every VS Code 1.98 secret-pattern category.
Fixes #17844