-
Notifications
You must be signed in to change notification settings - Fork 337
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
Add more logging and small enhacement to an int test #2680
Conversation
//await RunOnBehalfOfTestAsync(aadUser2, false).ConfigureAwait(false); | ||
//await RunOnBehalfOfTestAsync(adfsUser, true).ConfigureAwait(false); | ||
//await RunOnBehalfOfTestAsync(aadUser2, true).ConfigureAwait(false); | ||
//await RunOnBehalfOfTestAsync(aadUser2, false, true).ConfigureAwait(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for commenting these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, reverting.
private void LogReturnedToken(AuthenticationResult result) | ||
{ | ||
if (result.AccessToken != null && | ||
AuthenticationRequestParameters.RequestContext.Logger.IsLoggingEnabled(LogLevel.Info)) | ||
{ | ||
int appHashCode = AuthenticationRequestParameters.AppConfig.GetHashCode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Maybe rename here and in log below to appConfigHashCode?
- In which scenario would this hash code be useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the hash code is useful to know the instance of confidential client being used to track how many CCA are created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just as Neha stated, it's used to differentiate between different CCA objects. It's to be able to tell if ppl are using 1 or more instances of CCA object, as it has cache implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea.
LGTM
Fixes - no work item, need more logging for investigation.
Changes proposed in this request
Testing
Performance impact