-
Notifications
You must be signed in to change notification settings - Fork 101
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
Update to Serilog 4.x, remove some allocations #247
base: dev
Are you sure you want to change the base?
Conversation
…ry<K,V> or (K,V).
{ | ||
messageTemplate = value; | ||
} | ||
else if (property.Key.StartsWith("@")) | ||
{ | ||
if (_logger.BindProperty(GetKeyWithoutFirstSymbol(DestructureDictionary, property.Key), property.Value, true, out var destructured)) | ||
properties.Add(destructured); | ||
properties.Add(destructured.Name, destructured.Value); |
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 should be [destructured.Name] = destructured.Value
to avoid exceptions on duplicate keys.
@@ -36,7 +36,8 @@ public static LogEventLevel ToSerilogLevel(LogLevel logLevel) | |||
{ | |||
return logLevel switch | |||
{ | |||
LogLevel.None or LogLevel.Critical => LogEventLevel.Fatal, | |||
LogLevel.None => LevelAlias.Off, |
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.
bugfix?
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 don't think it'll have any effect in the way it's used, here, since events should never carry None
- just makes the code clearer.
if (_logger.BindProperty(property.Key, property.Value, false, out var bound)) | ||
properties.Add(bound); | ||
// Simple micro-optimization for the most common and reliably scalar values; could go further here. | ||
if (property.Value is null or string or int or long && LogEventProperty.IsValidName(property.Key)) |
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 would prefer to use ScalarValue.Null
for property.Value is null
check.
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.
👍 will take a look
Updates to Serilog 4.x to elminate a dictionary construction using
LogEvent.UnstableAssembleFromParts()
.This gets the old
LogInformation
benchmark down from (a whopping) 1.26 KB/iteration to (a whopping) 1.17 KB/iteration.But! The
LogInformation
benchmark is flawed, constructing a number of unrelated objects and doing some structure capturing, making it not representative of the overhead imposed by this library.So, I've renamed it to
Capturing
, and its companion toCapturingScoped
, and added some new benchmarks that should be closer to real-world average usage conditions.Using Serilog.Extensions.Logging still halves throughput and more than doubles allocations compared with just using Serilog directly, but in real-world terms both setups will have pretty negligible effects on latency or GC load.
Before and After
Before (
main
)After
Updated benchmarks
The new benchmarks here compare various MEL + Serilog.Extensions.Logging scenarios to a plain
_log.Information("Hello!")
Serilog call.