Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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