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

More flexible logging #160

Open
wants to merge 6 commits into
base: debug
Choose a base branch
from
Open

More flexible logging #160

wants to merge 6 commits into from

Conversation

loop8ack
Copy link

@loop8ack loop8ack commented Dec 9, 2023

Implemented a custom logger interface in Photino.NET to provide flexible logging solutions. This extension should be backward compatible.

I plan to develop an integration of Microsoft.Extensions.Logging within the photino.Blazor project:

internal sealed class MicrosoftPhotinoLogger : IPhotinoLogger
{
    private readonly ILogger _logger;

    public MicrosoftPhotinoLogger(ILoggerFactory loggerFactory)
    {
        _logger = loggerFactory.CreateLogger("Photino.NET");
    }

    public void Log(int verbosity, Exception exception, string message)
    {
        LogLevel logLevel;

        if (verbosity <= LogVerbosity.Critical)
            logLevel = LogLevel.Error;
        else if (verbosity <= LogVerbosity.Warning)
            logLevel = LogLevel.Warning;
        else if (verbosity <= LogVerbosity.Verbose)
            logLevel = LogLevel.Information;
        else if (verbosity <= LogVerbosity.Debug)
            logLevel = LogLevel.Debug;
        else
            logLevel = LogLevel.Trace;

        if (_logger.IsEnabled(logLevel))
            _logger.Log(logLevel, exception, message);
    }
}

Suggestion for Future Consideration

As an alternative improvement, integrating Microsoft.Extensions.Logging.Abstractions could be considered for future enhancements. This integration would necessitate breaking changes, such as the removal of the LogVerbosity property and SetLogVerbosity() method. It's also important to note that this integration would introduce an additional dependency on Microsoft.Extensions.DependencyInjection.Abstractions.
Despite this, it offers a more unified and standardized approach to logging.

If an integration of "Microsoft.Extensions.Logging" is under consideration, then please feel free to view this pull request as an invitation for discussion.

@MikeYeager MikeYeager changed the base branch from master to debug December 14, 2023 16:57
@MikeYeager MikeYeager added enhancement New feature or request help wanted Extra attention is needed All OS labels Dec 14, 2023
@MikeYeager
Copy link
Collaborator

@loop8ack Thank you for the suggestion. Agreed that the internal logging on the .NET side was thrown in hastily. We would like to implement Microsoft.Extensions.Logging and Microsoft.Extensions.DependencyInjection.Abstractions, but it's not at the top of our priority list. Since all logging calls within Photino.NET are private, and likely should remain private, we shouldn't have to worry about breaking changes. We'll just have to make sure that if no logger is specified, we default to a console logger. Thoughts?

@loop8ack
Copy link
Author

@MikeYeager The concern about BreakingChanges relates to the "LogVerbosity" property, as this is public and follows a concept that differs from the standard. The same applies to the "SetLogVerbosity" method.

If I use Microsoft.Extensions.Logging, the old LogVerbosity concept would be dropped, which leads to problems if someone has customized the LogVerbosity.

I guess that wouldn't be very bad, but I still wanted to get feedback first.

@loop8ack
Copy link
Author

Is there any news on this?

If necessary, I can also create a new pull request with Microsoft.Extensions.Logging.
That would probably be better, but would also be a breaking change.

@ottodobretsberger
Copy link
Contributor

We are currently working on updating and bugfixing our Samples projects. When we get to incorporating new features, we will circle back to this and incorporate it on the next major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
All OS enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants