fedarovich / isle

ISLE (Interpolated String Logging Extensions) is a library that allows developers to perform structured logging using interpolated strings in C# 10 or later.
MIT License
45 stars 1 forks source link

Logging calls with expressions don't work #2

Open cremor opened 2 years ago

cremor commented 2 years ago

Example code:

var now = DateTime.Now;
_logger.LogInformation($"property: {now.Day}");
var day = now.Day;
_logger.LogInformation($"simple: {day}");

Expected output:

property: 24
simple: 24

Actual output:

property: {now.Day}
simple: 24

The property access expressions could also be for more levels, that should also be supported. The last step could also be a method call instead of a property access.

fedarovich commented 2 years ago

This behavior depends on the underlying logging provider. I guess you use Serilog which requires the names to be valid C# identifiers.

The issues is that on one hand the expression can be anything, and on the other hand the expression can be transformed in different ways. For example, we can transform now.Day into Day or nowDay or even now_Day. That's why ISLE does not convert them by default, but allows the user to provide a custom delegate for the name conversion (see the end of the section Custom Argument Names).

At the moment, there are no conversions provided out-of-the-box, but I might provide some in the future versions.

cremor commented 2 years ago

Yes, I'm using Serilog. Thanks for the explanation. I've fixed it in my code by using

config.WithNameConverter(name => name.Replace(".", String.Empty, StringComparison.Ordinal));

I strongly suggest to add this as a default behavior so that popular logging frameworks work out of the box. Because that's a really annoying bug if you only use such expressions rarely and then only notice the problem when you already need the logged values 😄

fedarovich commented 2 years ago

I've added this functionality in version 1.2 as a part of Isle.Converters.ValueNameConverters class. However, it is not enabled by default, so you still need to add the converter manually:

IsleConfiguration.Configure(b => b.WithNameConverter(ValueNameConverters.SerilogCompatible()));
cremor commented 2 years ago

Works fine, thanks.

I suggest to add this to the readme if it won't be enabled by default.