Nlog: GetCurrentClassLogger is without namespace on coreclr

[aplex]

On .Net 4.5 , rule matches full class name that includes namespace, but on coreclr this rule does not log anything, unless namespace is removed and only class name is left.

Type : Bug
NLog version: 4.4.0-beta4
Platform: coreclr rc1 update 2 / OS X 10.11

Works on .Net 4.5, but not coreclr:

    <rules>
        <logger name="My.NameSpace.EventLogger" minlevel="Debug" writeTo="file" />     
    </rules>

Works on coreclr:

    <rules>
        <logger name="EventLogger" minlevel="Debug" writeTo="file" />     
    </rules>

[304NotModified]

Thanks for reporting. Are you using GetCurrentClassLogger? Can you test with logging ${logger}?

[aplex]

Yep, I'm using GetCurrentClassLogger. I use ${logger} in file target. On .net 4.5 it outputs class with namespace, on coreclr just class name, without namespace.

[304NotModified]

Thanks for testing. This is an issue after the limited stacktrace possibilities in coreclr.

But maybe we can fix it now.

[304NotModified]

To elaborate on this - this is a really big issue!

This is a bug but also a known issue as the needed StackTrace constructors / methods aren't implemented in .NET Core (see https://github.com/dotnet/corefx/issues/1420, https://github.com/dotnet/corefx/issues/1797).

To release NLog for .NET Core in a short time-frame, we have changed the implementation for the moment with using the CallerFilePathAttribute and striping of the file name (without file extension), with the hope this will be fixed when .NET Core RTM was released. This is not the case and GetCurrentClassLogger is now using the file name as "class name".

I'm really unhappy with this as this is a breaking change (behavior change) and limits NLog's capabilities.

We can hack creating the StackTrace/StackFrame with reflection, but AFAIK this isn't implemented in every platform so this will cause issues.

I tried to replace/re-implement the GetCurrentClassLogger on the following ways:

1. Reflection/Activator.CreateInstance - problematic as it's unclear on which platform it will work.
2. Using another CallerAttribute - the three possible CallerAttributes won't help us. Maybe in the future, see https://github.com/dotnet/roslyn/issues/351
3. Change the call (not backwards compatible). I think this would be acceptable as long as we can do it with a replace all. Was experimenting with this.GetCurrentClassLogger(), which works, but not in a static context (can't use this) which is recommend and sometimes needed!
4. Using code weaver, for now checked Fody but Fody isn't supporting .NET Core (and unclear if it would work), see https://github.com/Fody/Fody/issues/206
5. Use the DNX code weaving:

When you place a class implemeting ICompileModule in the folder compiler/preprocess, then it will be picked up by DNX during compilation. Here you can manipulate the CompilationUnit before compilation.
For testing purpose I implemented a NotNullCompileModule, which automatically inserts null checks for all parameters that have the NotNullAttribute. Worked like a charm and just took a hour to implement. But usability wise it was not very nice, because when the exception occurs you end up in the IL code (since there does not exist any corresponding source code).
https://github.com/aspnet/dnx/blob/7ac7929aa575e17b3c271e4a7a0c164418de0395/src/Microsoft.Dnx.Compilation.CSharp.Abstractions/ICompileModule.cs
https://github.com/aspnet/dnx/blob/7ac7929aa575e17b3c271e4a7a0c164418de0395/src/Microsoft.Dnx.Compilation.CSharp.Abstractions/BeforeCompileContext.cs
https://www.reddit.com/r/csharp/comments/3qodbr/code_weaving_with_fody_good_or_a_mistake/cwh8ikx

but unclear if that is still working with CLI. (and we have to find some docs first). Also unclear if this works from a library.

1. Remove GetCurrentClassLogger is also another option.

I'm not sure what to do. I don't think we should release a RTM without this being fixed. I have the feeling that we should cancel the .NET Core support in NLog 4.4 and reintroduce it in 4.5, 4.6 or maybe 5.0.

Any thoughts on this would be appreciated!

---

Vote is also possible on Twitter: https://twitter.com/NLogOfficial/status/758390118310875136

[phenixdotnet]

@304NotModified Agree with you about postpone the .NET Core CLR support in NLog 5.0. Anyway its a major change and the versioning should reflect it.

About the GetCurrentClassLogger as far I know the Activator trick will work on all platform except UWP. The discussion is open with the coreclr team to add support of the default constructor in coreclr. Others possibilities seem to be most tricky than this hack

[304NotModified]

Thanks @phenixdotnet for your feedback!

Others possibilities seem to be most tricky than this hack

Could you elaborate on this?

About the GetCurrentClassLogger as far I know the Activator trick will work on all platform except UWP.

AFAIK the aot implementation is missing, and I think the issue will be broader then UWP?

Agree with you about postpone the .NET Core CLR support in NLog 5.0. Anyway its a major change and the versioning should reflect it.

If you don't break stuff, I preferred 4.x, as I take semver very serious. But the fact is now the .NET core stuff is holding new releases, which is bad IMO.

It would be great if the stacktrace API was back, or another backwardscompatible trick. The real question if we should wait for it, even if we don't know if it get fixed or when.

[phenixdotnet]

Sure :)

1. I'm fine with this one
2. As you're saying yourself it doesn't work
3. It's a breaking change and a big one. As far I remember create a static readonly ILogger instance is the best practice. Changing that will have another down side as every time a class with logger will be created a new instance of the logger will be too. Bad perf, bad memory.
4. I don't like tool like Postsharp or Fody when working on a lib. We will must inject another build step in the user build process and this will error prone.
5. Same than 4
6. Removing this functionality "just" because we can't find a better solution doesn't seem acceptable ^^'. Just from my experience, I'm using GetCurrentClassLogger everytime.

About versioning, adding new platform is a breaking change or not ? ;) And the other question is do we deprecate other platforms support in favor of netstandard or do we maintains all version ? For example we can drop support for PCL because PCL can use netstandard. Same for .NET 4.5 / .NET 4.6

You can continue to release new version, .NET Core stuff is on its own branch we will change the version and push an other alpha/beta version with it. Dropping support of .NET Core in NLog 4.4 is ok IMO.

[mniak]

I vote for postpone the .NET core release.

[304NotModified]

@phenixdotnet

I think it would be something like this:

• Move .NET Core and netplatform (currently 4.4 beta) to 5.0 beta. This will give us some more time to fix things.
• Remove GetCurrentClassLogger for netplatform as it doens't work well, and move it to a separate package. If you use .NET Core, maybe you don't even need GetCurrentClassLogger because of the DI.
• If we got GetCurrentClassLogger fully working, then I will move it back to the core package.

PS: I tried implementing GetCurrentClassLogger with the Activator, and it's tricky as there are also some stackframes added for the Activator itself. It's unclear if the Activator is behaves the same across every platform.

[304NotModified]

Combined with the other feedback (THANKS), I will move .NET Core to NLog 5. This will delay the RTM of .NET Core, but hopefully not too much.

I'm doubting if we should create an extra nuget package for getCurrentClassLogger for netstandard, please share your thoughts!

PS: I have some nice features in mind for NLog 4.4

[snakefoot]

@304NotModified Created PR #2146 to resolve this issue

[304NotModified]

Resolved in https://github.com/NLog/NLog/pull/2146

Will be released in NLog 5 beta 8.

Thanks @snakefoot and @stil !!