Reduce CLI diagnostic log noise and improve process logging#15956
Reduce CLI diagnostic log noise and improve process logging#15956
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15956Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15956" |
There was a problem hiding this comment.
Pull request overview
This PR reduces diagnostic log noise in the Aspire CLI while improving the clarity and consistency of process execution logging. It also adds a startup log of all known feature flag states and avoids redundant feature-flag evaluation in NuGet package filtering.
Changes:
- Centralize “suppress logging” behavior in
ProcessExecutionFactoryby usingNullLoggerand simplifyProcessExecutionlogging branches. - Add
IFeatures.LogFeatureState()and call it at CLI startup to emit feature flag state for diagnostics. - Reduce log verbosity for cache/feature default behavior and avoid repeated
IsFeatureEnabledcalls inside per-package filter lambdas.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/Program.cs | Calls LogFeatureState() during startup to surface feature configuration in logs. |
| src/Aspire.Cli/NuGet/NuGetPackageCache.cs | Hoists ShowDeprecatedPackages evaluation outside the per-package filter lambda. |
| src/Aspire.Cli/NuGet/BundleNuGetPackageCache.cs | Hoists ShowDeprecatedPackages evaluation outside the per-package filter lambda. |
| src/Aspire.Cli/DotNet/ProcessExecutionFactory.cs | Uses NullLogger when suppressed and improves “running process” log context. |
| src/Aspire.Cli/DotNet/ProcessExecution.cs | Removes per-callsite suppression checks; adjusts process start/wait and stream forwarding logging. |
| src/Aspire.Cli/Configuration/IFeatures.cs | Adds new LogFeatureState() API to the internal feature service contract. |
| src/Aspire.Cli/Configuration/Features.cs | Downgrades default-value log to Trace and implements LogFeatureState(). |
| src/Aspire.Cli/Caching/DiskCache.cs | Downgrades cache hit/miss/store/delete logs from Debug to Trace. |
| public void LogFeatureState() | ||
| { | ||
| foreach (var metadata in KnownFeatures.GetAllFeatureMetadata()) | ||
| { | ||
| var value = IsFeatureEnabled(metadata.Name, metadata.DefaultValue); | ||
| logger.LogDebug("Feature {Feature} = {Value} (default: {DefaultValue})", metadata.Name, value, metadata.DefaultValue); | ||
| } |
There was a problem hiding this comment.
Features.LogFeatureState() references KnownFeatures, but KnownFeatures is declared in the Aspire.Cli namespace (src/Aspire.Cli/KnownFeatures.cs). This file is in Aspire.Cli.Configuration and currently lacks a using/fully-qualified reference, so this will not compile. Add using Aspire.Cli; (or fully-qualify Aspire.Cli.KnownFeatures).
| internal interface IFeatures | ||
| { | ||
| bool IsFeatureEnabled(string featureFlag, bool defaultValue); | ||
| void LogFeatureState(); |
There was a problem hiding this comment.
Adding LogFeatureState() to IFeatures is a breaking change for all IFeatures implementations (including numerous test fakes in tests/Aspire.Cli.Tests) and will cause compilation failures until they implement the new member. Consider either updating those implementations in this PR, or making this a non-interface extension method/default interface implementation to avoid widespread breakage.
| void LogFeatureState(); | |
| void LogFeatureState() { } |
| var effectiveLogger = options.SuppressLogging ? (ILogger)NullLogger.Instance : logger; | ||
|
|
||
| if (!suppressLogging) | ||
| { | ||
| logger.LogDebug("Running {FullName} with args: {Args}", workingDirectory.FullName, string.Join(" ", args)); | ||
| effectiveLogger.LogDebug("Running {FileName} in {WorkingDirectory} with args: {Args}", fileName, workingDirectory.FullName, string.Join(" ", args)); | ||
|
|
||
| if (env is not null) | ||
| if (env is not null) | ||
| { | ||
| foreach (var envKvp in env) | ||
| { | ||
| foreach (var envKvp in env) | ||
| { | ||
| logger.LogDebug("Running {FullName} with env: {EnvKey}={EnvValue}", workingDirectory.FullName, envKvp.Key, envKvp.Value); | ||
| } | ||
| effectiveLogger.LogDebug("{FileName} env: {EnvKey}={EnvValue}", fileName, envKvp.Key, envKvp.Value); | ||
| } | ||
| } |
There was a problem hiding this comment.
With the NullLogger approach, expensive argument formatting and env enumeration still happens when logging is suppressed because string.Join and the env foreach run unconditionally. Guard these with effectiveLogger.IsEnabled(LogLevel.Debug) (and only build the args string / iterate env when enabled) so SuppressLogging avoids the work as well as the output.
| string? line; | ||
| while ((line = await reader.ReadLineAsync()) is not null) | ||
| { | ||
| if (!suppressLogging) | ||
| { | ||
| _logger.LogTrace( | ||
| "{FileName}({ProcessId}) {Identifier}: {Line}", | ||
| FileName, | ||
| _process.Id, | ||
| identifier, | ||
| line | ||
| ); | ||
| } | ||
| _logger.LogTrace( | ||
| "({ProcessId}) {Identifier}: {Line}", | ||
| _process.Id, | ||
| identifier, | ||
| line | ||
| ); |
There was a problem hiding this comment.
Per-line process output trace logs no longer include the process FileName, which makes it harder to correlate output when multiple processes are running (previously it logged "{FileName}({ProcessId})..."). Consider including FileName (and optionally WorkingDirectory) in the trace template for better diagnostics.
|
🎬 CLI E2E Test Recordings — 56 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #24123652916 |
Description
Reduces diagnostic log noise in the Aspire CLI and improves process execution logging clarity.
Changes:
SuppressLoggingat every log call site, the factory now passesNullLogger.Instancewhen logging is suppressed. This simplifiesProcessExecutionby removing all conditional logging branches.DebugtoTraceto reduce noise. AddedLogFeatureState()method that logs all feature flag states at startup.LogFeatureState()at startup so feature configuration is visible in diagnostic logs.IsFeatureEnabledcall out of the per-package filter lambda to avoid redundant evaluations.Checklist