Address API Review feedback

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBuffer.cs

@@ -16,8 +16,8 @@ namespace Microsoft.AspNetCore.Diagnostics.Buffering;
 
 internal sealed class HttpRequestBuffer : ILoggingBuffer
 {
-    private readonly IOptionsMonitor<HttpRequestBufferOptions> _options;
-    private readonly IOptionsMonitor<GlobalBufferOptions> _globalOptions;
+    private readonly IOptionsMonitor<HttpRequestLogBufferingOptions> _options;
+    private readonly IOptionsMonitor<GlobalLogBufferingOptions> _globalOptions;
     private readonly ConcurrentQueue<SerializedLogRecord> _buffer;
     private readonly TimeProvider _timeProvider = TimeProvider.System;
     private readonly IBufferedLogger _bufferedLogger;
@@ -26,47 +26,41 @@ internal sealed class HttpRequestBuffer : ILoggingBuffer
     private int _bufferSize;
 
     public HttpRequestBuffer(IBufferedLogger bufferedLogger,
-        IOptionsMonitor<HttpRequestBufferOptions> options,
-        IOptionsMonitor<GlobalBufferOptions> globalOptions)
+        IOptionsMonitor<HttpRequestLogBufferingOptions> options,
+        IOptionsMonitor<GlobalLogBufferingOptions> globalOptions)
     {
         _options = options;
         _globalOptions = globalOptions;
         _bufferedLogger = bufferedLogger;
         _buffer = new ConcurrentQueue<SerializedLogRecord>();
     }
 
-    public bool TryEnqueue<TState>(
-        LogLevel logLevel,
-        string category,
-        EventId eventId,
-        TState state,
-        Exception? exception,
-        Func<TState, Exception?, string> formatter)
+    public bool TryEnqueue<TState>(LogEntry<TState> logEntry)
     {
         SerializedLogRecord serializedLogRecord = default;
-        if (state is ModernTagJoiner modernTagJoiner)
+        if (logEntry.State is ModernTagJoiner modernTagJoiner)
         {
-            if (!IsEnabled(category, logLevel, eventId, modernTagJoiner))
+            if (!IsEnabled(logEntry.Category, logEntry.LogLevel, logEntry.EventId, modernTagJoiner))
             {
                 return false;
             }
 
-            serializedLogRecord = new SerializedLogRecord(logLevel, eventId, _timeProvider.GetUtcNow(), modernTagJoiner, exception,
-                ((Func<ModernTagJoiner, Exception?, string>)(object)formatter)(modernTagJoiner, exception));
+            serializedLogRecord = new SerializedLogRecord(logEntry.LogLevel, logEntry.EventId, _timeProvider.GetUtcNow(), modernTagJoiner, logEntry.Exception,
+                ((Func<ModernTagJoiner, Exception?, string>)(object)logEntry.Formatter)(modernTagJoiner, logEntry.Exception));
         }
-        else if (state is LegacyTagJoiner legacyTagJoiner)
+        else if (logEntry.State is LegacyTagJoiner legacyTagJoiner)
         {
-            if (!IsEnabled(category, logLevel, eventId, legacyTagJoiner))
+            if (!IsEnabled(logEntry.Category, logEntry.LogLevel, logEntry.EventId, legacyTagJoiner))
             {
                 return false;
             }
 
-            serializedLogRecord = new SerializedLogRecord(logLevel, eventId, _timeProvider.GetUtcNow(), legacyTagJoiner, exception,
-                ((Func<LegacyTagJoiner, Exception?, string>)(object)formatter)(legacyTagJoiner, exception));
+            serializedLogRecord = new SerializedLogRecord(logEntry.LogLevel, logEntry.EventId, _timeProvider.GetUtcNow(), legacyTagJoiner, logEntry.Exception,
+                ((Func<LegacyTagJoiner, Exception?, string>)(object)logEntry.Formatter)(legacyTagJoiner, logEntry.Exception));
         }
         else
         {
-            Throw.ArgumentException(nameof(state), $"Unsupported type of the log state object detected: {typeof(TState)}");
+            Throw.InvalidOperationException($"Unsupported type of the log state object detected: {typeof(TState)}");
         }
 
         if (serializedLogRecord.SizeInBytes > _globalOptions.CurrentValue.MaxLogRecordSizeInBytes)
@@ -113,14 +107,14 @@ public bool IsEnabled(string category, LogLevel logLevel, EventId eventId, IRead
             return false;
         }
 
-        BufferFilterRuleSelector.Select(_options.CurrentValue.Rules, category, logLevel, eventId, attributes, out BufferFilterRule? rule);
+        LogBufferingFilterRuleSelector.Select(_options.CurrentValue.Rules, category, logLevel, eventId, attributes, out LogBufferingFilterRule? rule);
 
         return rule is not null;
     }
 
     private void Trim()
     {
-        while (_bufferSize > _options.CurrentValue.PerRequestBufferSizeInBytes && _buffer.TryDequeue(out var item))
+        while (_bufferSize > _options.CurrentValue.MaxPerRequestBufferSizeInBytes && _buffer.TryDequeue(out var item))
         {
             _ = Interlocked.Add(ref _bufferSize, -item.SizeInBytes);
         }

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBufferManager.cs

@@ -1,65 +1,56 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System;
 using Microsoft.AspNetCore.Http;
 using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Diagnostics.Buffering;
-using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 
 namespace Microsoft.AspNetCore.Diagnostics.Buffering;
 
-internal sealed class HttpRequestBufferManager : IHttpRequestBufferManager
+internal sealed class HttpRequestBufferManager : HttpRequestLogBuffer
 {
-    private readonly IGlobalBufferManager _globalBufferManager;
+    private readonly LogBuffer _globalBuffer;
     private readonly IHttpContextAccessor _httpContextAccessor;
-    private readonly IOptionsMonitor<HttpRequestBufferOptions> _requestOptions;
-    private readonly IOptionsMonitor<GlobalBufferOptions> _globalOptions;
+    private readonly IOptionsMonitor<HttpRequestLogBufferingOptions> _requestOptions;
+    private readonly IOptionsMonitor<GlobalLogBufferingOptions> _globalOptions;
 
     public HttpRequestBufferManager(
-        IGlobalBufferManager globalBufferManager,
+        LogBuffer globalBuffer,
         IHttpContextAccessor httpContextAccessor,
-        IOptionsMonitor<HttpRequestBufferOptions> requestOptions,
-        IOptionsMonitor<GlobalBufferOptions> globalOptions)
+        IOptionsMonitor<HttpRequestLogBufferingOptions> requestOptions,
+        IOptionsMonitor<GlobalLogBufferingOptions> globalOptions)
     {
-        _globalBufferManager = globalBufferManager;
+        _globalBuffer = globalBuffer;
         _httpContextAccessor = httpContextAccessor;
         _requestOptions = requestOptions;
         _globalOptions = globalOptions;
     }
 
-    public void FlushNonRequestLogs() => _globalBufferManager.Flush();
+    public override void Flush() => _globalBuffer.Flush();
 
-    public void FlushCurrentRequestLogs()
+    public override void FlushCurrentRequestLogs()
     {
         _httpContextAccessor.HttpContext?.RequestServices.GetService<HttpRequestBufferHolder>()?.Flush();
     }
 
-    public bool TryEnqueue<TState>(
-        IBufferedLogger bufferedLogger,
-        LogLevel logLevel,
-        string category,
-        EventId eventId,
-        TState state,
-        Exception? exception,
-        Func<TState, Exception?, string> formatter)
+    public override bool TryEnqueue<TState>(IBufferedLogger bufferedLogger, in LogEntry<TState> logEntry)
     {
         HttpContext? httpContext = _httpContextAccessor.HttpContext;
         if (httpContext is null)
         {
-            return _globalBufferManager.TryEnqueue(bufferedLogger, logLevel, category, eventId, state, exception, formatter);
+            return _globalBuffer.TryEnqueue(bufferedLogger, logEntry);
         }
 
         HttpRequestBufferHolder? bufferHolder = httpContext.RequestServices.GetService<HttpRequestBufferHolder>();
-        ILoggingBuffer? buffer = bufferHolder?.GetOrAdd(category, _ => new HttpRequestBuffer(bufferedLogger, _requestOptions, _globalOptions)!);
+        ILoggingBuffer? buffer = bufferHolder?.GetOrAdd(logEntry.Category, _ => new HttpRequestBuffer(bufferedLogger, _requestOptions, _globalOptions)!);
 
         if (buffer is null)
         {
-            return _globalBufferManager.TryEnqueue(bufferedLogger, logLevel, category, eventId, state, exception, formatter);
+            return _globalBuffer.TryEnqueue(bufferedLogger, logEntry);
         }
 
-        return buffer.TryEnqueue(logLevel, category, eventId, state, exception, formatter);
+        return buffer.TryEnqueue(logEntry);
     }
 }

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBufferLoggerBuilderExtensions.cs → src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBufferingLoggingBuilderExtensions.cs

@@ -17,83 +17,90 @@
 namespace Microsoft.Extensions.Logging;
 
 /// <summary>
-/// Lets you register log buffers in a dependency injection container.
+/// Lets you register HTTP request log buffering in a dependency injection container.
 /// </summary>
 [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
-public static class HttpRequestBufferLoggerBuilderExtensions
+public static class HttpRequestBufferingLoggingBuilderExtensions
 {
     /// <summary>
-    /// Adds HTTP request-aware buffer to the logging infrastructure. Matched logs will be buffered in
-    /// a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime./>.
+    /// Adds HTTP request log buffering to the logging infrastructure. 
     /// </summary>
     /// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
     /// <param name="configuration">The <see cref="IConfiguration" /> to add.</param>
     /// <returns>The value of <paramref name="builder"/>.</returns>
     /// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
+    /// <remarks>
+    /// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
+    /// </remarks>
     public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, IConfiguration configuration)
     {
         _ = Throw.IfNull(builder);
         _ = Throw.IfNull(configuration);
 
+        _ = builder.Services.AddSingleton<IConfigureOptions<HttpRequestLogBufferingOptions>>(new HttpRequestLogBufferingConfigureOptions(configuration));
+
         return builder
-            .AddHttpRequestBufferConfiguration(configuration)
             .AddHttpRequestBufferManager()
-            .AddGlobalBuffer(configuration);
+            .AddGlobalBuffering(configuration);
     }
 
     /// <summary>
-    /// Adds HTTP request-aware buffering to the logging infrastructure. Matched logs will be buffered in
-    /// a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime./>.
+    /// Adds HTTP request log buffering to the logging infrastructure.
     /// </summary>
     /// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
-    /// <param name="level">The log level (and below) to apply the buffer to.</param>
-    /// <param name="configure">The buffer configuration options.</param>
+    /// <param name="configure">The buffer configuration delegate.</param>
     /// <returns>The value of <paramref name="builder"/>.</returns>
     /// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
-    public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, LogLevel? level = null, Action<HttpRequestBufferOptions>? configure = null)
+    /// <remarks>
+    /// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
+    /// </remarks>
+    public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, Action<HttpRequestLogBufferingOptions> configure)
     {
         _ = Throw.IfNull(builder);
+        _ = Throw.IfNull(configure);
+
+        _ = builder.Services.Configure(configure);
 
-        _ = builder.Services
-            .Configure<HttpRequestBufferOptions>(options => options.Rules.Add(new BufferFilterRule(null, level, null, null)))
-            .Configure(configure ?? new Action<HttpRequestBufferOptions>(_ => { }));
+        HttpRequestLogBufferingOptions options = new HttpRequestLogBufferingOptions();
+        configure(options);
 
         return builder
             .AddHttpRequestBufferManager()
-            .AddGlobalBuffer(level);
+            .AddGlobalBuffering(opts => opts.Rules = options.Rules);
     }
 
     /// <summary>
-    /// Adds HTTP request buffer provider to the logging infrastructure.
+    /// Adds HTTP request log buffering to the logging infrastructure.
     /// </summary>
     /// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
-    /// <returns>The <see cref="ILoggingBuilder"/> so that additional calls can be chained.</returns>
+    /// <param name="logLevel">The log level (and below) to apply the buffer to.</param>
+    /// <returns>The value of <paramref name="builder"/>.</returns>
     /// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
-    internal static ILoggingBuilder AddHttpRequestBufferManager(this ILoggingBuilder builder)
+    /// <remarks>
+    /// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
+    /// </remarks>
+    public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, LogLevel? logLevel = null)
     {
         _ = Throw.IfNull(builder);
 
-        builder.Services.TryAddScoped<HttpRequestBufferHolder>();
-        builder.Services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
-        builder.Services.TryAddSingleton<HttpRequestBufferManager>();
-        builder.Services.TryAddSingleton<IBufferManager>(static sp => sp.GetRequiredService<HttpRequestBufferManager>());
-        builder.Services.TryAddSingleton<IHttpRequestBufferManager>(static sp => sp.GetRequiredService<HttpRequestBufferManager>());
+        _ = builder.Services.Configure<HttpRequestLogBufferingOptions>(options => options.Rules.Add(new LogBufferingFilterRule(logLevel: logLevel)));
 
-        return builder;
+        return builder
+            .AddHttpRequestBufferManager()
+            .AddGlobalBuffering(logLevel);
     }
 
-    /// <summary>
-    /// Configures <see cref="HttpRequestBufferOptions" /> from an instance of <see cref="IConfiguration" />.
-    /// </summary>
-    /// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
-    /// <param name="configuration">The <see cref="IConfiguration" /> to add.</param>
-    /// <returns>The value of <paramref name="builder"/>.</returns>
-    /// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
-    internal static ILoggingBuilder AddHttpRequestBufferConfiguration(this ILoggingBuilder builder, IConfiguration configuration)
+    internal static ILoggingBuilder AddHttpRequestBufferManager(this ILoggingBuilder builder)
     {
-        _ = Throw.IfNull(builder);
-
-        _ = builder.Services.AddSingleton<IConfigureOptions<HttpRequestBufferOptions>>(new HttpRequestBufferConfigureOptions(configuration));
+        builder.Services.TryAddScoped<HttpRequestBufferHolder>();
+        builder.Services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
+        builder.Services.TryAddSingleton(sp =>
+        {
+            var globalBufferManager = sp.GetRequiredService<GlobalBufferManager>();
+            return ActivatorUtilities.CreateInstance<HttpRequestBufferManager>(sp, globalBufferManager);
+        });
+        builder.Services.TryAddSingleton<LogBuffer>(sp => sp.GetRequiredService<HttpRequestBufferManager>());
+        builder.Services.TryAddSingleton<HttpRequestLogBuffer>(sp => sp.GetRequiredService<HttpRequestBufferManager>());
 
         return builder;
     }

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/IHttpRequestBufferManager.cs → src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestLogBuffer.cs

@@ -8,18 +8,13 @@
 namespace Microsoft.AspNetCore.Diagnostics.Buffering;
 
 /// <summary>
-/// Interface for an HTTP request buffer manager.
+/// Buffers HTTP request logs into circular buffers and drops them after some time if not flushed.
 /// </summary>
 [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
-public interface IHttpRequestBufferManager : IBufferManager
+public abstract class HttpRequestLogBuffer : LogBuffer
 {
     /// <summary>
-    /// Flushes the buffer and emits non-request logs.
+    /// Flushes buffers and emits buffered logs for the current HTTP request.
     /// </summary>
-    void FlushNonRequestLogs();
-
-    /// <summary>
-    /// Flushes the buffer and emits buffered logs for the current request.
-    /// </summary>
-    void FlushCurrentRequestLogs();
+    public abstract void FlushCurrentRequestLogs();
 }

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBufferConfigureOptions.cs → src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestLogBufferingConfigureOptions.cs

@@ -1,23 +1,22 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
-using System.Collections.Generic;
 using Microsoft.Extensions.Configuration;
 using Microsoft.Extensions.Options;
 
 namespace Microsoft.AspNetCore.Diagnostics.Buffering;
 
-internal sealed class HttpRequestBufferConfigureOptions : IConfigureOptions<HttpRequestBufferOptions>
+internal sealed class HttpRequestLogBufferingConfigureOptions : IConfigureOptions<HttpRequestLogBufferingOptions>
 {
     private const string BufferingKey = "Buffering";
     private readonly IConfiguration _configuration;
 
-    public HttpRequestBufferConfigureOptions(IConfiguration configuration)
+    public HttpRequestLogBufferingConfigureOptions(IConfiguration configuration)
     {
         _configuration = configuration;
     }
 
-    public void Configure(HttpRequestBufferOptions options)
+    public void Configure(HttpRequestLogBufferingOptions options)
     {
         if (_configuration == null)
         {
@@ -30,12 +29,15 @@ public void Configure(HttpRequestBufferOptions options)
             return;
         }
 
-        var parsedOptions = section.Get<HttpRequestBufferOptions>();
+        var parsedOptions = section.Get<HttpRequestLogBufferingOptions>();
         if (parsedOptions is null)
         {
             return;
         }
 
-        options.Rules.AddRange(parsedOptions.Rules);
+        foreach (var rule in parsedOptions.Rules)
+        {
+            options.Rules.Add(rule);
+        }
     }
 }

src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestBufferOptions.cs → src/Libraries/Microsoft.AspNetCore.Diagnostics.Middleware/Buffering/HttpRequestLogBufferingOptions.cs

@@ -9,21 +9,21 @@
 namespace Microsoft.AspNetCore.Diagnostics.Buffering;
 
 /// <summary>
-/// The options for LoggerBuffer.
+/// The options for HTTP request log buffering.
 /// </summary>
 [Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
-public class HttpRequestBufferOptions
+public class HttpRequestLogBufferingOptions
 {
     /// <summary>
     /// Gets or sets the size in bytes of the buffer for a request. If the buffer size exceeds this limit, the oldest buffered log records will be dropped.
     /// </summary>
     /// TO DO: add validation.
-    public int PerRequestBufferSizeInBytes { get; set; } = 5_000_000;
+    public int MaxPerRequestBufferSizeInBytes { get; set; } = 5_000_000;
 
-#pragma warning disable CA1002 // Do not expose generic lists - List is necessary to be able to call .AddRange()
+#pragma warning disable CA2227 // Collection properties should be read only - setter is necessary for options pattern
     /// <summary>
-    /// Gets the collection of <see cref="BufferFilterRule"/> used for filtering log messages for the purpose of further buffering.
+    /// Gets or sets the collection of <see cref="LogBufferingFilterRule"/> used for filtering log messages for the purpose of further buffering.
     /// </summary>
-    public List<BufferFilterRule> Rules { get; } = [];
-#pragma warning restore CA1002 // Do not expose generic lists
+    public IList<LogBufferingFilterRule> Rules { get; set; } = [];
+#pragma warning restore CA2227
 }