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

Added logging to auth providers #180

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

shweaver-MSFT
Copy link
Member

Fixes #179

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Currently, the logging in the auth providers is rather unhelpful when troubleshooting authentication configuration issues. In various cases exceptions are eaten which is particularly unhelpful.

What is the new behavior?

There are a few parts to the logging issue:

1. The logger interface and implementation

The ILogger represents a very basic logger that I think needs some representation in the toolkit. However, I don't think that the authentication packages are necessarily the correct place for this. Instead, 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

2. Consuming the logger

I'm a bit torn in my decision on how to consumer the logger. The easy way I think is to just create a new instance of DebugLogger and start logging messages, but that defeats the purpose of offering an interface to begin with. There needs to be somewhere to maintain the global ILogger instance so that all toolkit components can access it from anywhere:

I made the logger a member of BaseProvider, but in hindsight I wish it were in its own class. Something like a singleton LogManager with a getter/setter for LogManager.Logger which is an ILogger based implementation. As a note, this would require developers with a custom logger of their own to extend ILogger OR use a shim class instead to connect it to the LogManager:

// Manages the global logger instance
public class LogManager
{
    public bool IsLoggingEnabled { get; set; } = false;

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

// An example custom logger, if you don't want the default DebugLogger
public class CustomLogger : ILogger
{
    public void Log(string message)
    {
        // Call your custom logger here
    }
}

// Set somewhere in app startup code
LogManager.IsLoggingEnabled = true;
LogManager.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.Logger.Log(message));

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

This is a DRAFT, so some of the concepts described are not accurate to the actual code changes I made so far. More discussion needs to happen on where is the correct place for the logger to live. Is it the Diagnostics package or perhaps Common? Please continue this in the issue #179

@ghost
Copy link

ghost commented Jan 3, 2022

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Improve logging in providers
2 participants