Skip to content

Commit 7afd271

Browse files
evgenyfedorov2evgenyfedorov2
committed
Address API Review feedback
1 parent 4f524eb commit 7afd271

29 files changed

+379
-414
lines changed

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

+16-22
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ namespace Microsoft.AspNetCore.Diagnostics.Buffering;
1616

1717
internal sealed class HttpRequestBuffer : ILoggingBuffer
1818
{
19-
private readonly IOptionsMonitor<HttpRequestBufferOptions> _options;
20-
private readonly IOptionsMonitor<GlobalBufferOptions> _globalOptions;
19+
private readonly IOptionsMonitor<HttpRequestLogBufferingOptions> _options;
20+
private readonly IOptionsMonitor<GlobalLogBufferingOptions> _globalOptions;
2121
private readonly ConcurrentQueue<SerializedLogRecord> _buffer;
2222
private readonly TimeProvider _timeProvider = TimeProvider.System;
2323
private readonly IBufferedLogger _bufferedLogger;
@@ -26,47 +26,41 @@ internal sealed class HttpRequestBuffer : ILoggingBuffer
2626
private int _bufferSize;
2727

2828
public HttpRequestBuffer(IBufferedLogger bufferedLogger,
29-
IOptionsMonitor<HttpRequestBufferOptions> options,
30-
IOptionsMonitor<GlobalBufferOptions> globalOptions)
29+
IOptionsMonitor<HttpRequestLogBufferingOptions> options,
30+
IOptionsMonitor<GlobalLogBufferingOptions> globalOptions)
3131
{
3232
_options = options;
3333
_globalOptions = globalOptions;
3434
_bufferedLogger = bufferedLogger;
3535
_buffer = new ConcurrentQueue<SerializedLogRecord>();
3636
}
3737

38-
public bool TryEnqueue<TState>(
39-
LogLevel logLevel,
40-
string category,
41-
EventId eventId,
42-
TState state,
43-
Exception? exception,
44-
Func<TState, Exception?, string> formatter)
38+
public bool TryEnqueue<TState>(LogEntry<TState> logEntry)
4539
{
4640
SerializedLogRecord serializedLogRecord = default;
47-
if (state is ModernTagJoiner modernTagJoiner)
41+
if (logEntry.State is ModernTagJoiner modernTagJoiner)
4842
{
49-
if (!IsEnabled(category, logLevel, eventId, modernTagJoiner))
43+
if (!IsEnabled(logEntry.Category, logEntry.LogLevel, logEntry.EventId, modernTagJoiner))
5044
{
5145
return false;
5246
}
5347

54-
serializedLogRecord = new SerializedLogRecord(logLevel, eventId, _timeProvider.GetUtcNow(), modernTagJoiner, exception,
55-
((Func<ModernTagJoiner, Exception?, string>)(object)formatter)(modernTagJoiner, exception));
48+
serializedLogRecord = new SerializedLogRecord(logEntry.LogLevel, logEntry.EventId, _timeProvider.GetUtcNow(), modernTagJoiner, logEntry.Exception,
49+
((Func<ModernTagJoiner, Exception?, string>)(object)logEntry.Formatter)(modernTagJoiner, logEntry.Exception));
5650
}
57-
else if (state is LegacyTagJoiner legacyTagJoiner)
51+
else if (logEntry.State is LegacyTagJoiner legacyTagJoiner)
5852
{
59-
if (!IsEnabled(category, logLevel, eventId, legacyTagJoiner))
53+
if (!IsEnabled(logEntry.Category, logEntry.LogLevel, logEntry.EventId, legacyTagJoiner))
6054
{
6155
return false;
6256
}
6357

64-
serializedLogRecord = new SerializedLogRecord(logLevel, eventId, _timeProvider.GetUtcNow(), legacyTagJoiner, exception,
65-
((Func<LegacyTagJoiner, Exception?, string>)(object)formatter)(legacyTagJoiner, exception));
58+
serializedLogRecord = new SerializedLogRecord(logEntry.LogLevel, logEntry.EventId, _timeProvider.GetUtcNow(), legacyTagJoiner, logEntry.Exception,
59+
((Func<LegacyTagJoiner, Exception?, string>)(object)logEntry.Formatter)(legacyTagJoiner, logEntry.Exception));
6660
}
6761
else
6862
{
69-
Throw.ArgumentException(nameof(state), $"Unsupported type of the log state object detected: {typeof(TState)}");
63+
Throw.InvalidOperationException($"Unsupported type of the log state object detected: {typeof(TState)}");
7064
}
7165

7266
if (serializedLogRecord.SizeInBytes > _globalOptions.CurrentValue.MaxLogRecordSizeInBytes)
@@ -113,14 +107,14 @@ public bool IsEnabled(string category, LogLevel logLevel, EventId eventId, IRead
113107
return false;
114108
}
115109

116-
BufferFilterRuleSelector.Select(_options.CurrentValue.Rules, category, logLevel, eventId, attributes, out BufferFilterRule? rule);
110+
LogBufferingFilterRuleSelector.Select(_options.CurrentValue.Rules, category, logLevel, eventId, attributes, out LogBufferingFilterRule? rule);
117111

118112
return rule is not null;
119113
}
120114

121115
private void Trim()
122116
{
123-
while (_bufferSize > _options.CurrentValue.PerRequestBufferSizeInBytes && _buffer.TryDequeue(out var item))
117+
while (_bufferSize > _options.CurrentValue.MaxPerRequestBufferSizeInBytes && _buffer.TryDequeue(out var item))
124118
{
125119
_ = Interlocked.Add(ref _bufferSize, -item.SizeInBytes);
126120
}

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

+15-24
Original file line numberDiff line numberDiff line change
@@ -1,65 +1,56 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System;
54
using Microsoft.AspNetCore.Http;
65
using Microsoft.Extensions.DependencyInjection;
76
using Microsoft.Extensions.Diagnostics.Buffering;
8-
using Microsoft.Extensions.Logging;
97
using Microsoft.Extensions.Logging.Abstractions;
108
using Microsoft.Extensions.Options;
119

1210
namespace Microsoft.AspNetCore.Diagnostics.Buffering;
1311

14-
internal sealed class HttpRequestBufferManager : IHttpRequestBufferManager
12+
internal sealed class HttpRequestBufferManager : HttpRequestLogBuffer
1513
{
16-
private readonly IGlobalBufferManager _globalBufferManager;
14+
private readonly LogBuffer _globalBuffer;
1715
private readonly IHttpContextAccessor _httpContextAccessor;
18-
private readonly IOptionsMonitor<HttpRequestBufferOptions> _requestOptions;
19-
private readonly IOptionsMonitor<GlobalBufferOptions> _globalOptions;
16+
private readonly IOptionsMonitor<HttpRequestLogBufferingOptions> _requestOptions;
17+
private readonly IOptionsMonitor<GlobalLogBufferingOptions> _globalOptions;
2018

2119
public HttpRequestBufferManager(
22-
IGlobalBufferManager globalBufferManager,
20+
LogBuffer globalBuffer,
2321
IHttpContextAccessor httpContextAccessor,
24-
IOptionsMonitor<HttpRequestBufferOptions> requestOptions,
25-
IOptionsMonitor<GlobalBufferOptions> globalOptions)
22+
IOptionsMonitor<HttpRequestLogBufferingOptions> requestOptions,
23+
IOptionsMonitor<GlobalLogBufferingOptions> globalOptions)
2624
{
27-
_globalBufferManager = globalBufferManager;
25+
_globalBuffer = globalBuffer;
2826
_httpContextAccessor = httpContextAccessor;
2927
_requestOptions = requestOptions;
3028
_globalOptions = globalOptions;
3129
}
3230

33-
public void FlushNonRequestLogs() => _globalBufferManager.Flush();
31+
public override void Flush() => _globalBuffer.Flush();
3432

35-
public void FlushCurrentRequestLogs()
33+
public override void FlushCurrentRequestLogs()
3634
{
3735
_httpContextAccessor.HttpContext?.RequestServices.GetService<HttpRequestBufferHolder>()?.Flush();
3836
}
3937

40-
public bool TryEnqueue<TState>(
41-
IBufferedLogger bufferedLogger,
42-
LogLevel logLevel,
43-
string category,
44-
EventId eventId,
45-
TState state,
46-
Exception? exception,
47-
Func<TState, Exception?, string> formatter)
38+
public override bool TryEnqueue<TState>(IBufferedLogger bufferedLogger, in LogEntry<TState> logEntry)
4839
{
4940
HttpContext? httpContext = _httpContextAccessor.HttpContext;
5041
if (httpContext is null)
5142
{
52-
return _globalBufferManager.TryEnqueue(bufferedLogger, logLevel, category, eventId, state, exception, formatter);
43+
return _globalBuffer.TryEnqueue(bufferedLogger, logEntry);
5344
}
5445

5546
HttpRequestBufferHolder? bufferHolder = httpContext.RequestServices.GetService<HttpRequestBufferHolder>();
56-
ILoggingBuffer? buffer = bufferHolder?.GetOrAdd(category, _ => new HttpRequestBuffer(bufferedLogger, _requestOptions, _globalOptions)!);
47+
ILoggingBuffer? buffer = bufferHolder?.GetOrAdd(logEntry.Category, _ => new HttpRequestBuffer(bufferedLogger, _requestOptions, _globalOptions)!);
5748

5849
if (buffer is null)
5950
{
60-
return _globalBufferManager.TryEnqueue(bufferedLogger, logLevel, category, eventId, state, exception, formatter);
51+
return _globalBuffer.TryEnqueue(bufferedLogger, logEntry);
6152
}
6253

63-
return buffer.TryEnqueue(logLevel, category, eventId, state, exception, formatter);
54+
return buffer.TryEnqueue(logEntry);
6455
}
6556
}

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

+42-35
Original file line numberDiff line numberDiff line change
@@ -17,83 +17,90 @@
1717
namespace Microsoft.Extensions.Logging;
1818

1919
/// <summary>
20-
/// Lets you register log buffers in a dependency injection container.
20+
/// Lets you register HTTP request log buffering in a dependency injection container.
2121
/// </summary>
2222
[Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
23-
public static class HttpRequestBufferLoggerBuilderExtensions
23+
public static class HttpRequestBufferingLoggingBuilderExtensions
2424
{
2525
/// <summary>
26-
/// Adds HTTP request-aware buffer to the logging infrastructure. Matched logs will be buffered in
27-
/// a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime./>.
26+
/// Adds HTTP request log buffering to the logging infrastructure.
2827
/// </summary>
2928
/// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
3029
/// <param name="configuration">The <see cref="IConfiguration" /> to add.</param>
3130
/// <returns>The value of <paramref name="builder"/>.</returns>
3231
/// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
32+
/// <remarks>
33+
/// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
34+
/// </remarks>
3335
public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, IConfiguration configuration)
3436
{
3537
_ = Throw.IfNull(builder);
3638
_ = Throw.IfNull(configuration);
3739

40+
_ = builder.Services.AddSingleton<IConfigureOptions<HttpRequestLogBufferingOptions>>(new HttpRequestLogBufferingConfigureOptions(configuration));
41+
3842
return builder
39-
.AddHttpRequestBufferConfiguration(configuration)
4043
.AddHttpRequestBufferManager()
41-
.AddGlobalBuffer(configuration);
44+
.AddGlobalBuffering(configuration);
4245
}
4346

4447
/// <summary>
45-
/// Adds HTTP request-aware buffering to the logging infrastructure. Matched logs will be buffered in
46-
/// a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime./>.
48+
/// Adds HTTP request log buffering to the logging infrastructure.
4749
/// </summary>
4850
/// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
49-
/// <param name="level">The log level (and below) to apply the buffer to.</param>
50-
/// <param name="configure">The buffer configuration options.</param>
51+
/// <param name="configure">The buffer configuration delegate.</param>
5152
/// <returns>The value of <paramref name="builder"/>.</returns>
5253
/// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
53-
public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, LogLevel? level = null, Action<HttpRequestBufferOptions>? configure = null)
54+
/// <remarks>
55+
/// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
56+
/// </remarks>
57+
public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, Action<HttpRequestLogBufferingOptions> configure)
5458
{
5559
_ = Throw.IfNull(builder);
60+
_ = Throw.IfNull(configure);
61+
62+
_ = builder.Services.Configure(configure);
5663

57-
_ = builder.Services
58-
.Configure<HttpRequestBufferOptions>(options => options.Rules.Add(new BufferFilterRule(null, level, null, null)))
59-
.Configure(configure ?? new Action<HttpRequestBufferOptions>(_ => { }));
64+
HttpRequestLogBufferingOptions options = new HttpRequestLogBufferingOptions();
65+
configure(options);
6066

6167
return builder
6268
.AddHttpRequestBufferManager()
63-
.AddGlobalBuffer(level);
69+
.AddGlobalBuffering(opts => opts.Rules = options.Rules);
6470
}
6571

6672
/// <summary>
67-
/// Adds HTTP request buffer provider to the logging infrastructure.
73+
/// Adds HTTP request log buffering to the logging infrastructure.
6874
/// </summary>
6975
/// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
70-
/// <returns>The <see cref="ILoggingBuilder"/> so that additional calls can be chained.</returns>
76+
/// <param name="level">The log level (and below) to apply the buffer to.</param>
77+
/// <returns>The value of <paramref name="builder"/>.</returns>
7178
/// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
72-
internal static ILoggingBuilder AddHttpRequestBufferManager(this ILoggingBuilder builder)
79+
/// <remarks>
80+
/// Matched logs will be buffered in a buffer specific to each HTTP request and can optionally be flushed and emitted during the request lifetime.
81+
/// </remarks>
82+
public static ILoggingBuilder AddHttpRequestBuffering(this ILoggingBuilder builder, LogLevel? level = null)
7383
{
7484
_ = Throw.IfNull(builder);
7585

76-
builder.Services.TryAddScoped<HttpRequestBufferHolder>();
77-
builder.Services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
78-
builder.Services.TryAddSingleton<HttpRequestBufferManager>();
79-
builder.Services.TryAddSingleton<IBufferManager>(static sp => sp.GetRequiredService<HttpRequestBufferManager>());
80-
builder.Services.TryAddSingleton<IHttpRequestBufferManager>(static sp => sp.GetRequiredService<HttpRequestBufferManager>());
86+
_ = builder.Services.Configure<HttpRequestLogBufferingOptions>(options => options.Rules.Add(new LogBufferingFilterRule { LogLevel = level }));
8187

82-
return builder;
88+
return builder
89+
.AddHttpRequestBufferManager()
90+
.AddGlobalBuffering(level);
8391
}
8492

85-
/// <summary>
86-
/// Configures <see cref="HttpRequestBufferOptions" /> from an instance of <see cref="IConfiguration" />.
87-
/// </summary>
88-
/// <param name="builder">The <see cref="ILoggingBuilder" />.</param>
89-
/// <param name="configuration">The <see cref="IConfiguration" /> to add.</param>
90-
/// <returns>The value of <paramref name="builder"/>.</returns>
91-
/// <exception cref="ArgumentNullException"><paramref name="builder"/> is <see langword="null"/>.</exception>
92-
internal static ILoggingBuilder AddHttpRequestBufferConfiguration(this ILoggingBuilder builder, IConfiguration configuration)
93+
internal static ILoggingBuilder AddHttpRequestBufferManager(this ILoggingBuilder builder)
9394
{
94-
_ = Throw.IfNull(builder);
95-
96-
_ = builder.Services.AddSingleton<IConfigureOptions<HttpRequestBufferOptions>>(new HttpRequestBufferConfigureOptions(configuration));
95+
builder.Services.TryAddScoped<HttpRequestBufferHolder>();
96+
builder.Services.TryAddSingleton<IHttpContextAccessor, HttpContextAccessor>();
97+
builder.Services.TryAddSingleton(sp =>
98+
{
99+
var globalBufferManager = sp.GetRequiredService<GlobalBufferManager>();
100+
return ActivatorUtilities.CreateInstance<HttpRequestBufferManager>(sp, globalBufferManager);
101+
});
102+
builder.Services.TryAddSingleton<LogBuffer>(sp => sp.GetRequiredService<HttpRequestBufferManager>());
103+
builder.Services.TryAddSingleton<HttpRequestLogBuffer>(sp => sp.GetRequiredService<HttpRequestBufferManager>());
97104

98105
return builder;
99106
}

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

+4-9
Original file line numberDiff line numberDiff line change
@@ -8,18 +8,13 @@
88
namespace Microsoft.AspNetCore.Diagnostics.Buffering;
99

1010
/// <summary>
11-
/// Interface for an HTTP request buffer manager.
11+
/// Buffers HTTP request logs into circular buffers and drops them after some time if not flushed.
1212
/// </summary>
1313
[Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
14-
public interface IHttpRequestBufferManager : IBufferManager
14+
public abstract class HttpRequestLogBuffer : LogBuffer
1515
{
1616
/// <summary>
17-
/// Flushes the buffer and emits non-request logs.
17+
/// Flushes buffers and emits buffered logs for the current HTTP request.
1818
/// </summary>
19-
void FlushNonRequestLogs();
20-
21-
/// <summary>
22-
/// Flushes the buffer and emits buffered logs for the current request.
23-
/// </summary>
24-
void FlushCurrentRequestLogs();
19+
public abstract void FlushCurrentRequestLogs();
2520
}

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

+8-6
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,22 @@
11
// Licensed to the .NET Foundation under one or more agreements.
22
// The .NET Foundation licenses this file to you under the MIT license.
33

4-
using System.Collections.Generic;
54
using Microsoft.Extensions.Configuration;
65
using Microsoft.Extensions.Options;
76

87
namespace Microsoft.AspNetCore.Diagnostics.Buffering;
98

10-
internal sealed class HttpRequestBufferConfigureOptions : IConfigureOptions<HttpRequestBufferOptions>
9+
internal sealed class HttpRequestLogBufferingConfigureOptions : IConfigureOptions<HttpRequestLogBufferingOptions>
1110
{
1211
private const string BufferingKey = "Buffering";
1312
private readonly IConfiguration _configuration;
1413

15-
public HttpRequestBufferConfigureOptions(IConfiguration configuration)
14+
public HttpRequestLogBufferingConfigureOptions(IConfiguration configuration)
1615
{
1716
_configuration = configuration;
1817
}
1918

20-
public void Configure(HttpRequestBufferOptions options)
19+
public void Configure(HttpRequestLogBufferingOptions options)
2120
{
2221
if (_configuration == null)
2322
{
@@ -30,12 +29,15 @@ public void Configure(HttpRequestBufferOptions options)
3029
return;
3130
}
3231

33-
var parsedOptions = section.Get<HttpRequestBufferOptions>();
32+
var parsedOptions = section.Get<HttpRequestLogBufferingOptions>();
3433
if (parsedOptions is null)
3534
{
3635
return;
3736
}
3837

39-
options.Rules.AddRange(parsedOptions.Rules);
38+
foreach (var rule in parsedOptions.Rules)
39+
{
40+
options.Rules.Add(rule);
41+
}
4042
}
4143
}

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

+7-7
Original file line numberDiff line numberDiff line change
@@ -9,21 +9,21 @@
99
namespace Microsoft.AspNetCore.Diagnostics.Buffering;
1010

1111
/// <summary>
12-
/// The options for LoggerBuffer.
12+
/// The options for HTTP request log buffering.
1313
/// </summary>
1414
[Experimental(diagnosticId: DiagnosticIds.Experiments.Telemetry, UrlFormat = DiagnosticIds.UrlFormat)]
15-
public class HttpRequestBufferOptions
15+
public class HttpRequestLogBufferingOptions
1616
{
1717
/// <summary>
1818
/// 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.
1919
/// </summary>
2020
/// TO DO: add validation.
21-
public int PerRequestBufferSizeInBytes { get; set; } = 5_000_000;
21+
public int MaxPerRequestBufferSizeInBytes { get; set; } = 5_000_000;
2222

23-
#pragma warning disable CA1002 // Do not expose generic lists - List is necessary to be able to call .AddRange()
23+
#pragma warning disable CA2227 // Collection properties should be read only - setter is necessary for options pattern
2424
/// <summary>
25-
/// Gets the collection of <see cref="BufferFilterRule"/> used for filtering log messages for the purpose of further buffering.
25+
/// Gets or sets the collection of <see cref="LogBufferingFilterRule"/> used for filtering log messages for the purpose of further buffering.
2626
/// </summary>
27-
public List<BufferFilterRule> Rules { get; } = [];
28-
#pragma warning restore CA1002 // Do not expose generic lists
27+
public IList<LogBufferingFilterRule> Rules { get; set; } = [];
28+
#pragma warning restore CA2227
2929
}

0 commit comments

Comments
 (0)