-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Draft] Semantic Logging Support #6394
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
[Draft] Semantic Logging Support #6394
Conversation
eaf69a2
to
941cc17
Compare
This PR isn't fully compiling yet and I've had to re-review my own code since I started writing this back in October, 2022, but I wanted to offer a high-level summary of these changes, their motivations, and goals. Goals & MotivationsLogging has occasionally reared its head as a significant bottleneck in user-designed actors, mostly as a result of excessive allocations, stringification, and subsequent GC pressure. Our current Our goals with this PR are to help improve performance and extensibility while retaining our asynchronous log-consumption design and its benefits. In addition to those - we also want these new APIs to be at least source-compatible with the ones that are currently in-use through Akka.NET's internals as well as what users are doing in the wild - this will require a bit of planning but no reflection magic or any hacks of the sort. Goals
Binary CompatibilityBinary compat may not be achievable due to how "greedy" the existing My present thinking is that if we can't have our cake and eat it too, we're better off taking some binary compat API risk and automatically upgrading the performance of everyone's existing infrastructure so long as they just compile their code with v1.5 installed, but I'm open to arguments to the contrary. DesignI'll need to put a longer draft together in a subsequent comment, but basically we're making the internal logging state and the formatting strongly typed without boxing. |
DesignIn this design, we do the following. All Logs Still Flow Through
|
Benchmarks so far, bearing in mind we haven't made any internal optimizations yet. It's going to be ugly.
|
Method | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen0 | Gen1 | Gen2 | Allocated | Alloc Ratio |
---|---|---|---|---|---|---|---|---|---|---|---|
LogInfoNoParams | 163.2 ns | 3.25 ns | 8.22 ns | 160.3 ns | 1.00 | 0.00 | 0.0110 | 0.0060 | 0.0010 | 64 B | 1.00 |
LogInfo1Params | 383.5 ns | 19.88 ns | 58.61 ns | 401.0 ns | 2.36 | 0.39 | 0.0250 | 0.0120 | - | 160 B | 2.50 |
LogInfo2Params | 401.8 ns | 9.62 ns | 28.21 ns | 410.3 ns | 2.47 | 0.22 | 0.0300 | 0.0150 | - | 192 B | 3.00 |
LogInfoWithException | 164.6 ns | 3.26 ns | 7.62 ns | 160.9 ns | 1.01 | 0.07 | 0.0100 | 0.0050 | - | 64 B | 1.00 |
LogInfoWithException1Params | 383.3 ns | 20.53 ns | 60.52 ns | 398.9 ns | 2.36 | 0.38 | 0.0260 | 0.0130 | 0.0010 | 160 B | 2.50 |
LogInfoWithException2Params | 408.3 ns | 9.78 ns | 28.82 ns | 416.9 ns | 2.51 | 0.22 | 0.0300 | 0.0150 | - | 192 B | 3.00 |
This PR
BenchmarkDotNet=v0.13.2, OS=Windows 10 (10.0.19044.2486/21H2/November2021Update)
AMD Ryzen 7 1700, 1 CPU, 16 logical and 8 physical cores
.NET SDK=7.0.100
[Host] : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2
Job-NPJPYK : .NET 7.0.0 (7.0.22.51805), X64 RyuJIT AVX2
InvocationCount=1 UnrollFactor=1
Method | Mean | Error | StdDev | Median | Ratio | RatioSD | Gen0 | Gen1 | Gen2 | Allocated | Alloc Ratio |
---|---|---|---|---|---|---|---|---|---|---|---|
LogInfoNoParams | 345.2 ns | 24.55 ns | 72.40 ns | 373.1 ns | 1.00 | 0.00 | 0.0180 | 0.0090 | 0.0010 | 112 B | 1.00 |
LogInfo1Params | 419.5 ns | 11.53 ns | 33.99 ns | 435.6 ns | 1.29 | 0.37 | 0.0260 | 0.0130 | - | 168 B | 1.50 |
LogInfo2Params | 579.0 ns | 51.58 ns | 152.09 ns | 589.1 ns | 1.85 | 0.83 | 0.0310 | 0.0150 | - | 200 B | 1.79 |
LogInfoWithException | 339.4 ns | 24.61 ns | 72.56 ns | 352.0 ns | 0.98 | 0.04 | 0.0180 | 0.0090 | 0.0010 | 112 B | 1.00 |
LogInfoWithException1Params | 420.3 ns | 11.60 ns | 34.19 ns | 434.2 ns | 1.29 | 0.36 | 0.0260 | 0.0130 | - | 168 B | 1.50 |
LogInfoWithException2Params | 568.0 ns | 45.92 ns | 135.41 ns | 582.7 ns | 1.81 | 0.77 | 0.0310 | 0.0150 | - | 200 B | 1.79 |
/// </summary> | ||
public class LogMessage | ||
/// <typeparam name="TState">The state for the specified log message.</typeparam> | ||
public readonly struct LogEntry<TState> : ILogContents |
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.
Any cast from struct to interface is a boxing, using struct here instead of class is a tradeoff?
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.
So this is something I'm debating - to make this whole system more performant, we have to either commit to a larger breaking change or backwards compatibility.
- More performance - use generics for log state all the way down and discard the old
LogMessage
types. We can make the impact on theILoggingAdapter
side of things minimal but all custom back-end logging infrastructure will be broken. Changes needed to fix will be minor but will require new source code. - Backwards compat - this is what I opted for in this approach so far: preserving backwards compat to the extent that it's possible. That's why there's boxing and other issues.
I'm going to go down the more radical route and see what needs to be done here.
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.
Also, it matters where the boxing happens - we 100% don't care if it happens in the consuming logger actor. We care a lot if it happens in the foreground.
Changes
Prototype semantic logging support and a major performance improvement to Akka.NET logging in general.
Designed to be source compatible with Akka.NET, but not binary compatible.
Work in progress
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
Latest
dev
BenchmarksInclude data from the relevant benchmark prior to this change here.
This PR's Benchmarks
Include data from after this change here.