-
-
Notifications
You must be signed in to change notification settings - Fork 108
feat: Extensible log sink architecture #4493
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
Design for #4478 - enables real-time log streaming to IDEs during test execution via a plugin-based ILogSink system inspired by ASP.NET Core.
Add sink routing to the console interceptors so that Console.WriteLine calls stream in real-time to registered log sinks. - Add abstract SinkLogLevel property to OptimizedConsoleInterceptor - StandardOutConsoleInterceptor uses LogLevel.Information - StandardErrorConsoleInterceptor uses LogLevel.Error - Route all WriteLine methods (sync and async) to LogSinkRouter - Use both sync RouteToSinks and async RouteToSinksAsync as appropriate
Add built-in log sink that streams log messages to IDEs (Rider, VS, etc.) via Microsoft Testing Platform's IOutputDevice.DisplayAsync(). Features: - Implements ILogSink interface from TUnit.Core.Logging - Implements IOutputDeviceDataProducer for MTP integration - Color-coded output by log level (Red=Error, Yellow=Warning, Gray=Debug) - Configurable minimum log level (default: Information) - Fire-and-forget sync logging with async implementation - Exception message formatting appended to log message
Add comprehensive tests covering: - AddSink registers sink and makes it available via GetSinks - AddSink<T> instantiates and registers generic sink types - GetSinks returns empty list when no sinks registered - GetSinks returns snapshot (not live reference) - Clear removes all sinks without disposing them - DisposeAllAsync disposes IAsyncDisposable sinks - DisposeAllAsync disposes IDisposable sinks - DisposeAllAsync prefers IAsyncDisposable over IDisposable - DisposeAllAsync clears sinks list after disposal - DisposeAllAsync continues disposing remaining sinks on error Tests use [NotInParallel] to ensure isolation since TUnitLoggerFactory has static state.
Add end-to-end tests verifying that messages logged through DefaultLogger are properly routed to registered sinks via LogSinkRouter. Tests cover: - Basic message routing to registered sink - Multiple message routing - Correct log level propagation - Log level prefixes in messages - Context passing to sinks - Async logging support - Multiple sinks receiving broadcast messages - Disabled sink filtering - Level-filtered sink behavior
SummaryAdds a plugin-based log sink system for real-time IDE streaming during test execution. Critical Issues1. Performance Regression in OptimizedConsoleInterceptor (BLOCKING)Location: TUnit.Engine/Logging/OptimizedConsoleInterceptor.cs:499-547 The PR replaces optimized WriteLineFormatted calls with string.Format, which defeats the purpose of the BufferedTextWriter optimization. Before: _originalOutBuffer?.WriteLineFormatted(format, arg0, arg1); Problem: WriteLineFormatted uses StringBuilder.AppendFormat to avoid allocating intermediate strings. The new code calls string.Format upfront, creating an allocation on every formatted console write. This is a hot path during test execution. Fix: Either keep using WriteLineFormatted and route the format string directly to sinks, OR if you need the formatted string, cache it once and reuse it for both destinations. 2. Lock Type Not Available in netstandard2.0 (BLOCKING)Location: TUnit.Core/Logging/TUnitLoggerFactory.cs:177 The Lock type was introduced in .NET 9. TUnit targets netstandard2.0;net8.0;net9.0;net10.0. This will fail to compile on netstandard2.0 and net8.0. Fix: Replace with object _lock or use conditional compilation. 3. Breaking Change - Removal of OriginalConsole Writes (BLOCKING)Location: TUnit.Core/Logging/DefaultLogger.cs:25-31, 48-54 The PR removes writes to GlobalContext.Current.OriginalConsoleOut/Error from DefaultLogger. Was this intentional? If users relied on this for debugging, this is a breaking change. Verify that OutputDeviceLogSink provides equivalent coverage. Suggestions
Location: TUnit.Core/Logging/LogSinkRouter.cs:108, 136 GetSinks() creates a new array on every log call. If log volume is high, consider caching the sink array and invalidating on changes. VerdictREQUEST CHANGES - Critical issues 1, 2, and 3 must be addressed. |
Skip IDE streaming when running in console environment since console output is already visible. Uses IClientInfo to detect console clients.
SummaryImplements a plugin-based log sink system for real-time log streaming to IDEs. Critical IssuesPerformance concern in hot path: LogSinkRouter.RouteToSinks() allocates a new array on EVERY console write via TUnitLoggerFactory.GetSinks().ToArray(). This executes on every WriteLine() in OptimizedConsoleInterceptor. Impact: Tests that write frequently to console will cause significant GC pressure. Recommendation: Cache the sinks array and invalidate on Add/Clear to avoid hot path allocations. This violates TUnit Performance First rule (CLAUDE.md rule 4). Suggestions
VerdictREQUEST CHANGES - Critical perf issue needs fixing before merge |
IDEs display test output from StandardOutputProperty on TestNodeUpdateMessage, not from IOutputDevice.DisplayAsync. Changed approach to: - Add OutputUpdate method to ITUnitMessageBus for real-time output updates - Create IdeOutputLogSink that sends TestNodeUpdateMessage with current output - Modify TestExtensions.ToTestNode to support output during InProgress state - Remove unused OutputDeviceLogSink (IOutputDevice approach didn't work for IDEs) This sends TestNodeUpdateMessage updates with InProgressTestNodeStateProperty AND StandardOutputProperty, which IDEs pick up for real-time test output display.
SummaryImplements a plugin-based log sink system that enables real-time log streaming to IDEs during test execution. Critical Issues1. CRITICAL: Breaking behavioral change in DefaultLogger (TUnit.Core/Logging/DefaultLogger.cs:125-158) The removal of
Now it only goes to:
Impact: Users who rely on seeing logger output in the console when running tests from the command line will see nothing unless a sink is registered. The IdeOutputLogSink is only registered for IDE clients, not console clients. Fix: Restore the OriginalConsoleOut/Error writes OR ensure a console sink is registered for console clients in TUnitServiceProvider.cs. 2. CRITICAL: Performance - Hot path allocations (TUnit.Engine/Logging/OptimizedConsoleInterceptor.cs) From the diff, every WriteLine call now routes to sinks via
Fix: Consider caching the sink list or using a lock-free approach. The current implementation in LogSinkRouter: var sinks = TUnitLoggerFactory.GetSinks(); // lock + ToArray() every callShould either:
Suggestions1. ILogSink.Log() fire-and-forget pattern (TUnit.Engine/Logging/IdeOutputLogSink.cs:23-25) public void Log(LogLevel level, string message, Exception? exception, Context? context)
{
// Fire and forget - the async call will complete in background
_ = LogAsync(level, message, exception, context);
}This is dangerous - exceptions in the async continuation will be swallowed and not logged anywhere. The LogSinkRouter already catches and logs exceptions, but only for the awaited path. Suggestion: Document this behavior in ILogSink interface comments, or consider making Log() fully synchronous for sinks that need it (and have IdeOutputLogSink use a background queue instead). 2. Missing ConfigureAwait(false) in one location TUnit.Engine/Logging/IdeOutputLogSink.cs:41 calls But should verify all async paths in the routing use ConfigureAwait(false) consistently to avoid context capture overhead. 3. Lock type compatibility (TUnit.Core/Logging/TUnitLoggerFactory.cs:9) private static readonly Lock _lock = new();This uses the new .NET 9 VerdictBlocking:
Non-blocking but recommended: |
Summary
Introduces an extensible log sink architecture for routing test output to multiple destinations. This provides a foundation for custom logging integrations and future streaming capabilities.
Related: #4478 (real-time IDE streaming deferred to #4495)
Key Changes
New log sink system - Extensible architecture for routing log output to multiple destinations
ILogSinkinterface for custom log destinationsTUnitLoggerFactoryfor registering sinks at startupLogSinkRouterfor dispatching to all registered sinksBuilt-in sinks
TestOutputSink- Accumulates output toTestContext.OutputWriterfor test resultsConsoleOutputSink- Real-time console output when--output Detailedis enabledConsole interceptor integration -
OptimizedConsoleInterceptorroutesConsole.WriteLinecalls to registered sinksVerbosityService improvements - Centralized control over output behavior
IsDetailedOutputflag for--output DetailedmodeArchitecture
New Public APIs
Example: Custom Sink
Cleanup Included
LogLevelProvider._logLevelwas static, causing session pollutionNullLogger<T>toMtpNullLogger<T>for clarityAsyncConsoleWriterclassProcessFlushQueue()methodTest Plan
TUnitLoggerFactoryLogSinkRouter--output Detailedfrom console--output Normalstill works (buffered)Future Work
Real-time IDE streaming (#4495) will build on this sink architecture in a future PR.