Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Add ILogger #1765

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Conversation

Waterball12
Copy link
Contributor

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) => ...

@quinchs quinchs added the Needs investigation Needs to be looked at by a maintainer label Nov 23, 2021
@quinchs
Copy link
Member

quinchs commented Nov 24, 2021

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

@FeroxFoxxo
Copy link
Contributor

FeroxFoxxo commented Dec 2, 2021

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.

@quinchs quinchs added version: major and removed Needs investigation Needs to be looked at by a maintainer labels Jan 11, 2022
@csmir
Copy link
Collaborator

csmir commented Jan 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To look at
Development

Successfully merging this pull request may close these issues.

4 participants