microsoft
Guard ILogger logging with IsEnabled checks
Motivation and Context
Reduce overhead when logging disabled.
Description
When there's only a constant message, having a guard is less impactful. But if there are any arguments, without a guard at a minimum there's going to be a params array allocation, any value type arguments are going to be boxed, and in general any other work required to produce the arguments will be incurred, even in the common case of logging being disabled (or logging at that level being disabled). I added guards in all cases just for consistency (though I didn't bother with tests / samples).
Contribution Checklist
- [x] The code builds clean without any errors or warnings
- [x] The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
- [x] The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with
dotnet format - [x] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone :smile:
the gain in performance has a pretty big impact on code readability. Is this approach common when using ILogger?
Is this approach common when using ILogger?
Yes.
If you prefer to put things on one line, we could do:
if (this.Log.IsEnabled(LogLevel.Trace)) this.Log.LogTrace("HTTP response: {0} {1}", (int)response.StatusCode, response.StatusCode.ToString("G"));
and think of it as a little prefix, instead of:
if (this.Log.IsEnabled(LogLevel.Trace))
{
this.Log.LogTrace("HTTP response: {0} {1}", (int)response.StatusCode, response.StatusCode.ToString("G"));
}
but otherwise, in this example without a guard:
this.Log.LogTrace("HTTP response: {0} {1}", (int)response.StatusCode, response.StatusCode.ToString("G"));
we're allocating a box, ToString'ing an enum, and allocating a params array, even when tracing is disabled or the log level isnt for tracing.
Also wondering if we could wrap in extension methods, to keep things looking clean.
Also wondering if we could wrap in extension methods, to keep things looking clean.
It depends.
If the only thing we're concerned about is the params array and the boxing, then generic extension methods can be defined for all arities in use. That, however, won't help with the cases where the call site does other work, like string interpolation or ToString on values.
String interpolation could be helped by using custom interpolated string handlers, but that's more complicated and would require more polyfills to work when targeting netstandard2.0.
There's also the logging source generator, which helps with the boilerplate, but it also means you're defining a logging method for each thing to be logged, though this helps provide structure as well around logging rather than it being more haphazard.
To be revisited later. Need to look at options with extension methods, code generation, measure overhead.