Repository navigation
Reduce AppHost log noise for CLI connections and readiness checks - #20542
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 20542Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 20542" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The targeted logging changes match the stated intent and have focused regression coverage.
Review effort: Balanced
Findings: None
What changed in this PR
Reduces routine auxiliary backchannel logging noise while preserving errors for unexpected failures.
Changes:
- Moves expected connection lifecycle logs from Debug to Trace.
- Omits exception details for expected connection resets.
- Adds focused logging assertions.
| File | Description |
|---|---|
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelService.cs |
Lowers expected lifecycle logging verbosity. |
tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelTests.cs |
Verifies Trace-level cancellation and reset logs. |
💡 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. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
James Newton-King (JamesNK)
changed the title
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Move routine client lifecycle events to Trace and avoid logging stack traces for expected connection resets. Assert the shutdown and disconnect log levels in focused tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An abrupt socket close can complete StreamJsonRpc normally or surface as a connection reset. Check both disconnect paths stay at Trace without exceptions while ensuring no error is logged. 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 concise Debug messages when AppHost connections close during ps info and dashboard lookups, while retaining exception details for unexpected failures.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ignore expected AppHost disconnects while continuing to watch remaining AppHosts. Keep Debug exceptions for unexpected RPC failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Propagate connection loss from legacy resource log streaming so follow commands report a stopped AppHost instead of treating disconnect as a successful end of logs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the existing GetResourceLogsAsync fallback behavior and remove its dedicated propagation test. Retain the shared backchannel disconnect classifier. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Share socket connection-reset classification between CLI telemetry followers and the Hosting auxiliary backchannel. Preserve error handling for invalid telemetry and unexpected transport failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
22decaf to
445f9f8
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The documented telemetry-follow shutdown contract lacks coverage through the real CLI and HTTP transport path.
Review effort: Balanced
Findings: 1
Files not reviewed (1)
- src/Aspire.Cli/Resources/TelemetryCommandStrings.Designer.cs: Generated file
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Mitch Denny (mitchdenny)
left a comment
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
Approved, a few non blocking comments.
Add an optional error-handler overload and a disconnect-aware wrapper, update telemetry follow commands and tests, and remove unnecessary README detail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Tests selector51 / 98 PR test projects · 4 PR jobs, from 40 changed files. Selected PR test projects (51 / 98)
Selected PR jobs (4)
How these were chosen — grouped by what changed
🔧 show 45
🧪 📦 affected project 🧪 🧪 🧪 🧪 🧪 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
Source review found no blocking issues, and focused regression coverage addresses the changed behavior.
Review effort: Balanced
Findings: None
Resolved since last review (1)
Files not reviewed (1)
- src/Aspire.Cli/Resources/TelemetryCommandStrings.Designer.cs: Generated file
|
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: #1819
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1819 targeting Triggered signals: Note This draft PR needs human review before merging. |
Description
Reduce routine AppHost diagnostic noise produced by CLI connections and repeated dashboard readiness checks:
WatchAsyncsnapshot replay inside itstry/finallyso early disposal unsubscribes.Microsoft.Extensions.Http.DefaultHttpClientFactorylogs in all AppHost logging providers, including the CLI backchannel. Information, Warning, and Error logs remain visible to those providers and are still forwarded to the CLI.No new dependencies or public API changes.
Related: #20552 tracks the separately investigated accumulation of
aspire ps --followprocesses and their dashboard URL requests; this PR reduces log noise but does not reduce those calls.Validation
ResourceNotificationTestsandDistributedApplicationBuilderTests: 137 passed, one Unix-only test skipped on Windows. The HTTP-client test verifies that Debug entries are filtered globally while Information and higher remain available to both the backchannel and another logging provider.aspire describecalls produced four dashboard-URL RPC entries and no resource-readiness wait/finished entries.Checklist
<remarks />and<code />elements on your triple slash comments?