Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add logging buffering #5635
base: main
Are you sure you want to change the base?
Add logging buffering #5635
Changes from 1 commit
988c709
2f1a335
2d2412e
f7eaab1
1f464df
a371d9c
d7661a6
9d13ab0
fe00658
5fc421c
70cfc7c
e96277f
a79fcbf
8a91c15
4f524eb
b2b6e56
f3a6b85
1370225
393ce26
20b5a4c
f5ce71e
ce2f7fb
6ffbacd
1370ccb
56257ca
73a8678
8a42385
7ff16fc
35223f3
bf0b59d
715ce25
ccdfaeb
4e1566b
5171413
58f7b20
cc0d9b7
5ca502b
82eeb33
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You and Martin should decide if you care about emitting the events in any particular order. As-is it looks like the sort order is first by an arbitrary category ordering and secondly by approximate time-order. If users see buffered logs dumped on the console that way they might be confused by it but I imagine most logging backends won't care at all. I'm fine with it as-is but I wanted to raise the question to ensure everyone else also is comfortable with it.
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.
I have a logging provider that writes the events to files and I have been planning to support log buffering in that after it is upgraded to .NET 10 LTS. There though, I'd very much like to keep the events ordered by time, rather than by category. So if this PR implements per-category buffers, then I'd need to implement a separate step (in-process or out of process) that sorts the events after all per-category buffers have been flushed. More likely, I'd avoid the Microsoft.Extensions.Telemetry package, implement the buffering myself, and keep the events in chronological order. The API in dotnet/runtime at least makes that possible.
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.
This comment relates to the IBufferedLogger interface and to log buffering in general, not to any code in this pull request.
I've been working on a private implementation of a buffering logger factory that keeps the events in chronological order and supports .NET Framework too. It passes tests but I'll still need to work on:
In my implementation, there is typically one ILogBuffer per incoming request and possibly one global ILogBuffer as well; there are no per-category buffers. The per-category ILogger created by the ILoggerFactory locates the current ILogBuffer and passes to it both a LogEntry<TState> value and a "broadcasting" per-category IBufferedLogger reference. The ILogBuffer saves a queue entry that contains both the buffered form of the log entry and the IBufferedLogger reference. When the ILogBuffer is eventually flushed, it is expected to call IBufferedLogger.LogRecords(IEnumerable<BufferedLogRecord>), which then broadcasts the log records to the per-category, per-provider IBufferedLogger implementations if the logger filter rules allow. If consecutive queue entries have the same IBufferedLogger reference, then the log buffer can flush them in the same IBufferedLogger.LogRecords call.
In each of the following cases, the factory-created ILogger forwards the log entry to the per-provider ILogger immediately without buffering:
If the logger configuration is changed or ILoggerFactory.AddProvider is called, then the broadcasting IBufferedLogger instances are replaced with new ones that use the new configuration, and these new instances will be used in future queue entries, but any queue entries already in a log buffer keep referring to the old IBufferedLogger instances that use the old configuration.
A potential performance problem is that, if consecutive queue entries in a log buffer are for different log categories, then they have different IBufferedLogger references and cannot be included in the same IBufferedLogger.LogRecords(IEnumerable<BufferedLogRecord>) call. Thus, if the log categories keep alternating, then each IEnumerable<BufferedLogRecord> will have fewer elements, and flushing the entire buffer will require a greater number of IBufferedLogger.LogRecords calls. I'll need to measure the cost of this. It might be possible to replace IBufferedLogger with some new IMultiCategoryBufferedLogger interface that natively supports multiple log categories in the same IEnumerable<T>, but to take full advantage of it, the new interface would need to be implemented in logger provider packages as well.
Another performance risk is that my implementation currently has quite a few generic methods parameterised on <TState>. I recall reading that those are particularly slow in interface method calls; and that with value types, the native code is not shared. I did it this way to minimise allocations, but I should measure the real effect on performance.
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.
@KalleOlaviNiemitalo - I'll leave it to @evgenyfedorov2 to mull this over but I just wanted to give a big thanks for all the thoughtful feedback and context you've added. Its much appreciated!