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] Improve logging in providers #179

Open
shweaver-MSFT opened this issue Dec 8, 2021 · 0 comments · May be fixed by #180
Open

[Feature] Improve logging in providers #179

shweaver-MSFT opened this issue Dec 8, 2021 · 0 comments · May be fixed by #180

Comments

@shweaver-MSFT
Copy link
Member

shweaver-MSFT commented Dec 8, 2021

Describe the problem this feature would solve

When the authentication providers work, they are a delight. But when they don't work, they are rather difficult to troubleshoot.

Describe the solution

We need better logging to expose some insights into what the provider is doing, and what exceptions or failures are occurring. However, I feel like this is a great opportunity to handle logging in a more generic way across the whole toolkit.

I see a few moving parts:

  • ILogger interface - defines the basic functions of a logger. At a minimum, log a string message.
  • DebugLogger implementation - extends ILogger and uses System.Diagnostics.Debug.WriteLine to log messages. This is the default.
  • LogManager singleton - maintains active ILogger implementation used across the packages.

1. The logger interface and implementation

I'd like to see the ILogger interface put in the CommunityToolkit.Diagnostics package (link). This enables us to offer the simple DebugLogger based on System.Diagnostics.Debug.WriteLine

// Defines a basic logger
public interface ILogger
{
    // Turn logging on or off
    bool IsEnabled { get; set; }

    // Log a string message, if enabled
    void Log(string message);
}

// Default logger implementation
public class DebugLogger : ILogger
{
    // Turned off by default.
    public bool IsEnabled { get; set; } = false;

    public void Log(string message)
    {
        if (IsEnabled)
        {
            System.Diagnostics.Debug.WriteLine(message);
        }
    }
}

// Manages the global logger instance
public class LogManager
{
    public static LogManager Instance { get; } = new LogManager();

    public ILogger Logger { get; set; } = new DebugLogger();
}

2. Consuming the logger

Developers with a custom logger can extend ILogger OR use a shim class instead to connect it to the LogManager:

// Set somewhere in app startup code
LogManager.Instance.Logger.IsEnabled = true;

// OR

// An example custom logger shim, if you don't want the default DebugLogger:
public class CustomLogger : ILogger
{
    public bool IsEnabled { get; set; } = true;

    public void Log(string message)
    {
        // Call your custom logger here
    }
}

// Set somewhere in app startup code
LogManager.Instance.Logger = new CustomLogger();

3. Applying the logger to WindowsProvider

Pretty straight forward, ensure that every try/catch block sends any exceptions to the logger. Also look for opportunities to log other common events that could help with troubleshooting.

4. Applying the logger to MsalProvider

First ensure that every try/catch block sends any exceptions to the logger. But also, the MsalProvider leverages the PublicClientApplication object from MSAL for .NET which has a mechanism for passing in a method to handle the log output. This is enabled by using the WithLogging method on the client builder. This can be connected with the LogManager like so:

clientBuilder.WithLogging((level, message, containsPii) => LogManager.Instance.Logger.Log(message));

Additional context & Screenshots

Here is an example of a developers having issues with troubleshooting in the auth providers:
MicrosoftDocs/WindowsCommunityToolkitDocs#593

Open questions

  1. Should the auth providers handle logging internally (local to the auth packages)?
  2. OR is a logging system the right thing to do?
  3. Where should the core classes to support logging go? In the Diagnostics or Common packages?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant