Add Support for Microsoft.Extensions.Logging

[SkyeHoefling]

Description of Problem

Logging in DNN is using log4net and I believe we are using a DNN specific fork of it. This works and I don't intend to change how this works at all for the .NET Framework version of DNN. As we get closer to .NET Core migrations, we need to consider how to decouple the platform from any hard dependencies on .NET Framework to make the transition easier.

DNN currently has an ILog interface that exists in the DotNetNuke.Instrumentation library. This is the interface that is used for logging throughout the platform and ecosystem.

public interface ILog
{
    bool IsDebugEnabled { get; }
    bool IsInfoEnabled { get; }
    bool IsTraceEnabled { get; }
    bool IsWarnEnabled { get; } 
    bool IsErrorEnabled { get; }
    bool IsFatalEnabled { get; }

    void Debug(object message);
    void Debug(object message, Exception exception);
    void DebugFormat(string format, params object[] args);
    void DebugFormat(IFormatProvider provider, string format, params object[] args);

    void Info(object message);
    void Info(object message, Exception exception);
    void InfoFormat(string format, params object[] args);
    void InfoFormat(IFormatProvider provider, string format, params object[] args);

    void Trace(object message);
    void Trace(object message, Exception exception);
    void TraceFormat(string format, params object[] args);
    void TraceFormat(IFormatProvider provider, string format, params object[] args);

    void Warn(object message);
    void Warn(object message, Exception exception);
    void WarnFormat(string format, params object[] args);
    void WarnFormat(IFormatProvider provider, string format, params object[] args);

    void Error(object message);
    void Error(object message, Exception exception);
    void ErrorFormat(string format, params object[] args);
    void ErrorFormat(IFormatProvider provider, string format, params object[] args);

    void Fatal(object message);
    void Fatal(object message, Exception exception);
    void FatalFormat(string format, params object[] args);
    void FatalFormat(IFormatProvider provider, string format, params object[] args);
}

This topic was discussed in some capacity in the following item:

• 2801 - The discuss was more focused on upgrading log4net

Proposed Solution Option 1

I propose we refactor our current implementation of the DotNetNuke.Instrumentation.ILog interface and our log4net implementation to use Microsoft.Extensions.Logging and deprecate our custom interface and current usage for v11. This will allow us to register the Microsoft.Extensions.Logging interface ILogger with the Dependency Injection Container and then anyone can resolve it anywhere in the platform or extensions.

As with a breaking change like this, the current usage of LoggerSource.Instance.GetLogger() should return the registered instance of ILogger but this creates more complexity to prevent any type of break since DotNetNuke.Instrumentation.ILog != Microsoft.Extensions.Logging.ILogger

How Will This Work?

I'm glad you asked!

1. Deprecate all usages of DotNetNuke.Instrumentation.ILog for removal in v11.0
2. Deprecate all methods in the LoggerSource that returns ILog for removal in v11. This will make it clear that we can still use this for building our ILogger but the DNN ILog interface is going to be removed.
3. Create an implementation of the current logger that implements Microsoft.Extensions.Logging.ILogger. This means our current logging implementation will not change
4. Update all usages of the DotNetNuke.Instrumentation.ILog in the platform to use the Microsoft.Extensions.Logging.ILogger via Dependency Injection and where not available allow it ot use the Service Locator pattern.

Should DotNetNuke.Instrumentation be .NET Standard?

I would really love for this project to become .NET Standard 2.0 compliant but I believe that will be a follow-up issue if we proceed with this proposal. The DotNetNuke.Instrumentation library has hard dependencies on System.Web that is used for logging some of the exceptions. After an analysis of some of the code we may be able to remove it, but I'm not 100% certain at this time.

In the below code snippet, the BuildManager object is a System.Web dependency. This is just 1 example of the System.Web dependency in the DotNetNuke.Instrumentation library https://github.com/dnnsoftware/Dnn.Platform/blob/e03f3b0e32a89c88178fdaad1f9d9c73d8f914fc/DNN%20Platform/DotNetNuke.Instrumentation/DnnLog.cs#L60-L64

How to Register ILogger

This will require new registration code in the DotNetNuke.Instrumentation project which will be using the new IDnnStartup interface. The LoggerSource or similar object will be responsible for creating a Microsoft.Extensions.Logging.ILogger Implementation and registering it to the container. At that point any part of the platform or extensions can resolve it using Dependency Injection

The new ILogger Interface

The ILogger is a much smaller interface than our current ILog but the same rules can still be applied.

https://github.com/aspnet/Extensions/blob/66007df6e5b68683de0cab0dc6a8552265318f20/src/Logging/Logging.Abstractions/src/ILogger.cs#L8-L39

copied from link above

/// <summary>
/// Represents a type used to perform logging.
/// </summary>
/// <remarks>Aggregates most logging patterns to a single method.</remarks>
public interface ILogger
{
    /// <summary>
    /// Writes a log entry.
    /// </summary>
    /// <param name="logLevel">Entry will be written on this level.</param>
    /// <param name="eventId">Id of the event.</param>
    /// <param name="state">The entry to be written. Can be also an object.</param>
    /// <param name="exception">The exception related to this entry.</param>
    /// <param name="formatter">Function to create a <see cref="string"/> message of the <paramref name="state"/> and <paramref name="exception"/>.</param>
    /// <typeparam name="TState">The type of the object to be written.</typeparam>
    void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter);

    /// <summary>
    /// Checks if the given <paramref name="logLevel"/> is enabled.
    /// </summary>
    /// <param name="logLevel">Level to be checked.</param>
    /// <returns><c>true</c> if enabled.</returns>
    bool IsEnabled(LogLevel logLevel);

    /// <summary>
    /// Begins a logical operation scope.
    /// </summary>
    /// <param name="state">The identifier for the scope.</param>
    /// <typeparam name="TState">The type of the state to begin scope for.</typeparam>
    /// <returns>An <see cref="IDisposable"/> that ends the logical operation scope on dispose.</returns>
    IDisposable BeginScope<TState>(TState state);
}

Deprecated ILog Implementation

As we migrate away from the ILog implementation we can configure it to use the new ILogger in the implementation details. This will create a clear migration strategy of what methods map to the new interface. As well this will create 1 code path for logging and limit logging side-effects between having 2 different implementations

Alternatives Researched

N/A

Affected version

I propose we target this change for 9.x so we can properly deprecate the ILog interface for v11.0

• [x] 10.0.0 nightly build
• [x] 9.4.1 nightly build
• [x] 9.4.0 latest supported release

[donker]

Don't take the following as wanting to tack something onto this (i.e. scope creep): I second this and I'd love to see this move "closer" to event logging. I.e. similar implementation. As a dev I find it confusing that we have the two (and currently completely different technical solutions). So the "front facing part" that developers see would be near each other and the "back end" decoupled as suggested here. Thoughts?

[valadas]

Hmm, I am not sure I understand but I think it is useful to have a level of logging that admins can view and some other level that is not visible by admins and is more reserved to only people with hosting access... If that was your meaning...

[SkyeHoefling]

What needs to be done to convert this into an issue and no longer as an RFC? Personally I think it is documented enough for development.

With it getting closer and closer to 10.0 is there another 9.x release going out where we can deprecate the NLog implementation. I remember when I last attempted this I got it about 80% of the way done but ran into some problems. If we deprecate NLog now, it will give us some time to finish that implementation in 10.x and remove the NLog implementations at 11.x along with many other APIs we are planning to remove.

[mitchelsellers]

@ahoefling I think this is a real issue at this point I removed the "RFC" discussion point to it.

We will for sure have 9.7.0 and a few other releases.

[stale[bot]]

We have detected this issue has not had any activity during the last 90 days. That could mean this issue is no longer relevant and/or nobody has found the necessary time to address the issue. We are trying to keep the list of open issues limited to those issues that are relevant to the majority and to close the ones that have become 'stale' (inactive). If no further activity is detected within the next 14 days, the issue will be closed automatically. If new comments are are posted and/or a solution (pull request) is submitted for review that references this issue, the issue will not be closed. Closed issues can be reopened at any time in the future. Please remember those participating in this open source project are volunteers trying to help others and creating a better DNN Platform for all. Thank you for your continued involvement and contributions!

[SkyeHoefling]

This is still current.

I have a fork that is 90% done, just needs a little polish before submitting a PR.

[stale[bot]]

This issue has been closed automatically due to inactivity (as mentioned 14 days ago). Feel free to re-open the issue if you believe it is still relevant.

[SkyeHoefling]

This is still current, can we get this marked as pinned @mitchelsellers

[valadas]

I am reopening the issue @david-poindexter what did we decide about tags that would be used to prevent stalebot from closing some issues ?

[valadas]

Oh, nevermind @david-poindexter I just could not type properly, Pinned is still there 😄 Need more coffee I guess