Discord.Net icon indicating copy to clipboard operation
Discord.Net copied to clipboard

Feature: Add ILogger

Open Waterball12 opened this issue 4 years ago • 3 comments

Description

This pull request adds support for the ILogger interface from the Microsoft.Extensions.Logging package. It replaces LogManager and the other components with ILogger, ILoggerFactory, and ILoggerProvider.

Changes

I have deleted Logger, LogManager, LogMessage and LogSeverity from Discord.Net.Core and instead replaced them with a DefaultLogger, DefaultLoggerFactory and DefaultLoggerProvider. Also, I have added Microsoft.Extensions.Logging.Abstractions v5.0.0 to the Discord.Net.Core package. Finally, I have changed all the Logger implementation to ILogger so the code has changed like this

--- await _gatewayLogger.DebugAsync("...").ConfigureAwait(false);
+++ _gatewayLogger.LogDebug("...");

Motivation

The ILogger implementation has more flexibility and allows the use of different logging provider (Serilog, Nlog) and just pass a LoggerFactory instead of having to use a Log event

Breaking changes

I have removed the LogLevel Property in DiscordConfig not sure if I should have instead kept the property and just add the Obsolete attribute

I have removed the Log event property from the client again not sure if I should have instead kept the property and just add the Obsolete attribute

The CommandService is affected as well, I have removed the LogLevel property and added LoggerFactory not sure if I should have instead kept the property and just add the Obsolete attribute

Now to get logs from the DiscordSocketClient you can do something like this

var client = _client = new DiscordShardedClient(new DiscordSocketConfig
{
---    LogLevel = LogSeverity.Info,
+++    LoggerFactory = yourLoggerFactory
});

--- client.Log += (msg) => ...

Waterball12 avatar Feb 01 '21 22:02 Waterball12

I don't think this would benefit a lot of people, if you really needed to you could just proxy the discord log event to your own implementation of ILogger. I've never had issues with using a log event or needed to implement my own type of logs so it could just be my own bias here.

The main issues I see is everyone coming from 2.x to 3.x and there old code breaking because of logging changes, I would try to avoid this as much as possible

quinchs avatar Nov 24 '21 17:11 quinchs

Personally, I think this pull would be useful for the next version of DNet as ILogger<> is used in a wide range of other libraries already, and would make it easier to integrate as you wouldn't need to write your own conversion methods between the two types. As this class is what is preferred by many in the C# community, broadly, to use to log, and the differences between the DNet and Microsoft loggers are minute, the only major issue with this is the breaking changes that would be caused, a sacrifice that would be a (relatively?) easy fix for many developers.

As far as I know, DSharp also uses ILogger, as there's no particular need for a custom implementation for a logger on either libraries, while this class exists. For my own use case, before migrating to DNet, this made integration with ASP.Net, which a lot of particularly larger bots use for creating RESTful APIs, very simple and straight forward as they used the same logging class (ILogger).

Just my two cents though! I completely agree that breaking changes should be avoided where possible, though it makes more sense to be using the default logger interface that Microsoft provides for this better integration than a custom one that does essentially the same thing. Using ILogger would be more versatile than the current logger that DNet supports.

FeroxFoxxo avatar Dec 02 '21 06:12 FeroxFoxxo

It is unfortunate that ILogger is not supported by the default System libraries like IServiceProvider. It would be elegant and easy if we did not need to depend on an exterior service (Microsoft.Extensions.Logging) to add the feature. For me personally this is enough reason not to implement it, and keep the library self dependent for the most part.

csmir avatar Jan 21 '23 11:01 csmir