Make terminal-host telemetry opt-in - #20005
Conversation
Disable helper telemetry unless ShowTerminalHost is enabled, and preserve per-helper OTLP service identity for diagnostic runs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c676f42-11fe-4ca3-9dba-8beeeca95fca
|
🚀 Dogfood this PR with:
curl -fsSL https://github.com/@raw/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20005Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20005" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The reviewed changes have focused test coverage and no unresolved issues.
Review tier: Balanced
Findings: None
What changed in this PR
Makes terminal-host telemetry opt-in through ShowTerminalHost, preventing hidden helpers from exporting inherited telemetry.
Changes:
- Gates helper telemetry and OTLP configuration behind explicit opt-in.
- Preserves helper service and replica identities.
- Adds focused telemetry and environment tests.
| File | Description |
|---|---|
tests/Aspire.TerminalHost.Tests/TerminalHostTelemetryTests.cs |
Tests telemetry gating, providers, and identity. |
tests/Aspire.Hosting.Tests/WithTerminalTests.cs |
Tests helper and parent environment behavior. |
src/Shared/KnownConfigNames.cs |
Defines the telemetry opt-in variable. |
src/Aspire.TerminalHost/TerminalHostTelemetry.cs |
Documents opt-in behavior. |
src/Aspire.TerminalHost/TerminalHostApp.cs |
Conditionally configures telemetry. |
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs |
Configures helper telemetry based on visibility. |
src/Aspire.Hosting/ApplicationModel/TerminalAnnotation.cs |
Documents the expanded option behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing ReportPR Information
Artifact Version Verification
Changes Analyzed
Categories: Hosting, shared terminal-host runtime, documentation, and tests. No dashboard implementation, templates, client integrations, VS Code extension, or CI infrastructure changes. Test SetupUsed an isolated local install and three fresh projects generated with the PR template hive, not the earlier source-build fixtures. curl -fsSL https://github.com/@raw/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh \
| bash -s -- 20005 --install-path "$testDir/install" --skip-path --skip-extensionTemplate creation explicitly selected the PR hive and version, C#, The experimental "$cli" config set features.terminalCommandsEnabled true --non-interactiveTerminal input was exercised through Hex1b; dashboard assertions and screenshots used headless Edge. No global CLI settings or repository files were changed. Test Scenarios Executed1. Default helpers, inherited settings, and parent telemetryCoverage: Happy path and negative configuration case. Status: Passed. Added default
Evidence: 2. Opted-in diagnostic replicasCoverage: Happy path and replica identity. Status: Passed. Created a real console project with two replicas and: .WithReplicas(2)
.WithTerminal(options => options.ShowTerminalHost = true)Both producers were alive, both helpers appeared in the resource list, and the workload responded to terminal input with Exported log resource attributes exactly matched DCP's injected identities:
The dashboard Traces and Metrics selectors contained the two helper names. Live terminal-host metric instruments were present, and the accessible metric table showed Evidence: 3. Replica restart and terminal recoveryCoverage: Lifecycle/recovery. Status: Restart and reattachment passed; continuous attachment did not. Restarted both concrete project instances using An exploratory assertion that the existing CLI attachment would continue accepting input failed: the attachment disconnected during restart. A fresh attachment successfully processed This run establishes recovery by reattachment, not transparent reconnection. It did not compare attachment continuity against the base branch, so this observation is not classified as a regression introduced by this PR. Evidence: 4. No dashboard and missing workloadCoverage: Boundary and expected failure. Status: Passed. A fresh AppHost disabled the dashboard and cleared its OTLP endpoint configuration. A default terminal still started and round-tripped input. A second resource referenced a deliberately nonexistent executable. It visibly entered Evidence: 5. Standalone helper opted in without an endpointCoverage: Telemetry initialization boundary. Status: Passed. Launched the actual packaged Evidence: Coverage LimitsThe helpers produced logs and metrics, not lifecycle activities. Therefore this run verifies their Traces resource-list behavior, but does not claim to have observed exported helper spans. Tracer-provider gating and identity are covered by the focused source tests added in this PR; those earlier local unit results are not represented as new artifact-test runs here. CI ObservationsThe PR's TerminalHost tests passed on Linux and Windows. Two other jobs initially failed and subsequently passed on retry:
At 07:19:41 UTC the final check snapshot was 309 passed, 7 skipped, none pending or failing. No code changes, quarantines, issue creation, or manual CI reruns were performed during this testing task. Summary
Overall ResultTargeted telemetry behavior verified against the matching PR artifacts. The restart attachment observation prevents claiming uninterrupted terminal continuity. Earlier broader CI failures passed on retry, and the final CI snapshot had no pending or failing checks. Artifacts and CleanupSanitized evidence is retained with the local testing report: five dashboard PNG screenshots, four terminal SVG screenshots, runtime JSON, assertion output, fixture source, installer logs, and CI logs. Dashboard login URLs, API keys, and raw environment exports are not included in this report. All test AppHosts, terminal viewers, the standalone helper, and headless browser sessions were stopped. The temporary install/project workspace was removed. The repository remains unchanged and the PR remains a draft. |
James Newton-King (JamesNK)
left a comment
There was a problem hiding this comment.
Approved. I left one non-blocking robustness comment about failing closed when the telemetry opt-in environment variable is malformed. Focused tests and direct OTLP proof otherwise confirmed the intended hidden/visible behavior and per-replica resource identity.
Use bool.TryParse so invalid diagnostic configuration leaves telemetry disabled without terminating the terminal host. Cover empty, whitespace, malformed, and numeric values. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c676f42-11fe-4ca3-9dba-8beeeca95fca
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Link IConfigurationExtensions into TerminalHost using the existing non-hosting attribute shim pattern, and use GetBool with a false default. Cover zero and nonzero numeric opt-in values alongside malformed input. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c676f42-11fe-4ca3-9dba-8beeeca95fca
Tests selector56 / 99 PR test projects · 4 PR jobs · 2 advisory-only targets, from 9 changed files. Selected PR test projects (56 / 99)
Selected PR jobs (4)
Advisory workflow impact (2)
How these were chosen — grouped by what changed
🔧 show 39
🔧 🔧 🧪 📦 affected project 📦 affected project 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Dashboard-disabled applications currently receive a synthetic OTLP endpoint and start unwanted exporter attempts.
Review tier: Balanced
Findings: None
Previously missed findings (1)
In code that hasn't changed since last review
src/Aspire.Hosting/TerminalResourceBuilderExtensions.cs:229
AddOtlpEnvironmentdoes not require an available endpoint: when the dashboard is disabled and neither dashboard OTLP URL is configured,OtlpEndpointResolverfalls back tohttp://localhost:18889. As a result,ShowTerminalHost = truestill injects that URL,CreateTelemetryHostBuilderstarts telemetry, and every helper attempts to export to a nonexistent collector, contrary to the documented “when an endpoint is available” behavior. Use the optional/skip-if-unavailable registration path (including dynamic dashboard-resource resolution) and cover the dashboard-disabled, no-endpoint case.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Pull request created: #1651
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1651 targeting Updated the
Note This draft PR needs human review before merging. |
|
The CI build failed due to test failure(s) that appear unrelated to the PR changes. These may be flaky tests. Suspected flaky test(s):
Suggested actions:
You can re-run the failed jobs from the workflow run page. |
Description
WithTerminal()currently exports telemetry from its hidden terminal-host helpers whenever an OTLP endpoint is available. Their logs registerAspire.TerminalHostin the dashboard's telemetry resource list, including the Traces filter, even when no spans are emitted.Terminal-host logs, metrics, and traces are now disabled by default. The existing
ShowTerminalHost = trueoption both reveals the helper resources and enables their diagnostic telemetry when an OTLP endpoint is available. Parent-resource telemetry and terminal interaction are unchanged.User-facing usage
Use
.WithTerminal()normally without exporting helper telemetry. To diagnose a terminal host, opt in with:Validation
ShowTerminalHost = true. All producers were alive, terminal input round-tripped, and only the two opted-in helpers exported logs. Exported service names and instance IDs matched DCP's resource identities despite ambient telemetry settings.The existing
DownstreamPrimaryResizeIsForwardedUpstreamAsRawResizeFrametest timed out in an isolated run waiting for a resize frame and was excluded from the successful TerminalHost selection. It calls the unchanged instanceRunAsyncpath rather than the modified telemetry entry point; it was not changed or quarantined.Fixes # (issue)
Checklist
<remarks />and<code />elements on your triple slash comments?