Open shweaver-MSFT opened 2 years ago
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 🙌
Fixes #179
PR Type
What kind of change does this PR introduce?
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 theILogger
interface put in theCommunityToolkit.Diagnostics
package (link). This enables us to offer the simpleDebugLogger
based onSystem.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 globalILogger
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 singletonLogManager
with a getter/setter forLogManager.Logger
which is anILogger
based implementation. As a note, this would require developers with a custom logger of their own to extendILogger
OR use a shim class instead to connect it to the LogManager: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 thePublicClientApplication
object from MSAL for .NET which has a mechanism for passing in a method to handle the log output. This is enabled by using theWithLogging
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:
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