-
Notifications
You must be signed in to change notification settings - Fork 715
Clean up deployment logs to reduce verbosity and centralize stack trace filtering with DI-based configuration #12298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ducing verbosity Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
|
@copilot The detection of debug level vs not should be centralized in @dotnet/aspire/files/src/Aspire.Hosting/Pipelines/PipelineLoggerProvider.cs . We should not have if statements all over the code around stack traces vs not. |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Done! Moved the stack trace filtering logic to |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12298Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12298" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reduces verbosity in deployment logs by moving command execution details to Debug level and centralizing exception stack trace filtering, making production logs cleaner while preserving detailed information for troubleshooting.
Key changes:
- Command execution logging moved from Information to Debug level across container runtime classes
- Exception stack trace filtering centralized in
PipelineLoggerProviderto automatically exclude traces at Information level and above - Improved user experience with actionable error messages by default, full diagnostics available with
--verbosity debug
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| DockerContainerRuntime.cs | Changed Docker CLI command logging from Information to Debug level |
| ContainerRuntimeBase.cs | Changed container runtime command logging from Information to Debug level in two methods |
| PipelineLoggerProvider.cs | Added centralized exception filtering logic to exclude stack traces above Debug level |
|
@copilot Lets do more clean up of the azure logging: Remove this block logging about DefaultAzureCredential: We can log the azure credential type here @dotnet/aspire/files/src/Aspire.Hosting.Azure/Provisioning/Internal/DefaultTokenCredentialProvider.cs |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Done! Cleaned up Azure logging:
All Azure credential types now log consistently: "Using {credentialType} for provisioning." (commit 36d0aab) |
|
@copilot The PipelineLoggerProvider needs to get the configured log level from configuration see @dotnet/aspire/files/src/Aspire.Hosting/DistributedApplicationBuilder.cs and use that to filter if we show stack traces for exceptions or not. Let's create an options object called PipelineLoggingOptions that encapsulates this logic and is part of DI. class PipelineLoggingOptions The changes to DistributedApplicationBuilder should look like: _innerBuilder.Services.Configure(o => Then PipelineLoggerProvider should inject IOptions and should use IncludeExceptionDetails to determine if the stack should be included. |
…il configuration Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Done! Created |
…tions dependency and simplify logger initialization
|
@copilot Fix the tests D:\a\aspire\aspire\tests\Aspire.Hosting.Tests\Pipelines\PipelineLoggerProviderTests.cs(20,20): error CS1729: 'PipelineLoggerProvider' does not contain a constructor that takes 1 arguments [D:\a\aspire\aspire\tests\Aspire.Hosting.Tests\Aspire.Hosting.Tests.csproj] |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Fixed! Updated PipelineLoggerProviderTests to use the parameterless constructor that matches the current implementation. Removed the obsolete IOptions parameter and unused using statement. (commit c6e1435) |
tests/Aspire.Hosting.Tests/Pipelines/PipelineLoggerProviderTests.cs
Outdated
Show resolved
Hide resolved
tests/Aspire.Hosting.Tests/Pipelines/PipelineLoggerProviderTests.cs
Outdated
Show resolved
Hide resolved
tests/Aspire.Hosting.Tests/Pipelines/PipelineLoggerProviderTests.cs
Outdated
Show resolved
Hide resolved
…ce filtering with DI-based configuration (#12298) * Initial plan * Clean up deployment logs by conditionally showing stack traces and reducing verbosity Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Centralize stack trace filtering logic in PipelineLoggerProvider Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Update src/Aspire.Hosting/Pipelines/PipelineLoggerProvider.cs * Clean up Azure credential logging and remove verbose messages Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Use PipelineLoggingOptions to centralize log level and exception detail configuration Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Fix logging configuration to use IOptions for PipelineLoggingOptions * Update src/Aspire.Hosting/Pipelines/PipelineLoggerProvider.cs * Refactor PipelineLoggerProvider and PipelineStepContext to remove IOptions dependency and simplify logger initialization * Change log level from Information to Debug for deployment state save confirmation * Fix PipelineLoggerProviderTests to use parameterless constructor Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Apply suggestions from code review --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Co-authored-by: David Fowler <davidfowl@gmail.com> Co-authored-by: Ankit Jain <radical@gmail.com>
Clean up deployment logs
Based on the issue analysis and feedback, here are the changes made:
Centralize stack trace filtering in PipelineLoggerProvider
Change command execution logs from LogInformation to LogDebug
Fix PublishModeProvisioningContextProvider cancellation handling
Clean up Azure credential logging
Fix test compilation errors
Implementation Details
PipelineLoggerProvider now uses a simple approach without IOptions:
DistributedApplicationBuilder configures logging through standard filters:
Publishing:LogLevelconfigurationThis eliminates the need for conditional logging checks throughout the codebase while achieving clean production logs and detailed debug logs when needed.
Benefits
--verbosity debugTesting
Original prompt
This section details on the original issue you should resolve
<issue_title>Clean up deployment logs now that we have them</issue_title>
<issue_description> General guidance:
Specific fixes
.aspire\deployments\10F260DCF4A8CD0C5B329F00A1A16931049274AABA8201AF2A19FB8638DD20F5\production.json" we already show "13:45:49 (analyze-model) ✓ Deployment state will be loaded from:". We can make this debug.