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.

Sign up for GitHub

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

Jump to bottom

Adding support for MSAL telemetry cache data. #2256

Merged
merged 14 commits into from
Aug 24, 2023
Merged

Adding support for MSAL telemetry cache data. #2256

merged 14 commits into from
Aug 24, 2023
+272 −60

Conversation

trwalke
Copy link
Member

@trwalke trwalke commented May 24, 2023 •
edited
Loading

Fix for #1900

Here is the implementation of the telemetry cache details for TokenCacheNotificationArgs.
It appears to me that the MsalDistributedTokenCacheAdapter seems to be the correct place to make these changes but I want to confirm this with you @jmprieur and @jennyf19.

  • Are there any other locations where I need to implement this logic?
  • Are there any expectations for the customer to implement their own versions of MsalAbstractTokenCacheProvider or MsalDistributedTokenCacheAdapter
  • If the above is correct, do you know of anyway to gather this information from the customers implementation? I am trying to avoid adding to much additional api service.

@trwalke trwalke requested review from jmprieur and jennyf19 May 24, 2023 08:19
@trwalke
Copy link
Member Author

trwalke commented May 24, 2023

Cant build until after MSAL release.

@trwalke trwalke mentioned this pull request May 24, 2023
Closed
jmprieur
jmprieur reviewed May 24, 2023
View reviewed changes
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @trwalke
I left a question.

src/Microsoft.Identity.Web.TokenCache/MsalAbstractTokenCacheProvider.cs Outdated Show resolved Hide resolved
@trwalke trwalke marked this pull request as ready for review June 2, 2023 05:33
bgavrilMS
bgavrilMS requested changes Jun 2, 2023
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

trwalke added 2 commits June 6, 2023 13:11
Adding more tests
9649acf 
adding implementation for in memory cache
Test update
1b2492a
bgavrilMS
bgavrilMS approved these changes Jun 12, 2023
View reviewed changes
Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving code changes, but please consider the PR blocked until we figure out the path forward with telemetry.

jennyf19
jennyf19 requested changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marking as blocked until the telemetry questions are sorted out.

@bgavrilMS bgavrilMS requested a review from jennyf19 June 13, 2023 12:03
@bgavrilMS
Copy link
Member

@jennyf19 @jennyf19 - looks like our partners want to use ITelemetryClient. Do we need more clarifications from them?

@jennyf19
Copy link
Collaborator

@jennyf19 @jennyf19 - looks like our partners want to use ITelemetryClient. Do we need more clarifications from them?

We are seeing much progress with OTel, so, at this point, we would not use ITelemetryClient. Will know more in another week or two.

@trwalke trwalke closed this Aug 10, 2023
@trwalke trwalke reopened this Aug 10, 2023
Merge branch 'master' into trwalke/MSAL_Telemetry_Cache_Data
3910bd1 
# Conflicts:
#	Directory.Build.props
#	tests/Microsoft.Identity.Web.Test/CacheExtensionsTests.cs
#	tests/Microsoft.Identity.Web.Test/Microsoft.Identity.Web.Test.csproj
Merge remote-tracking branch 'origin/trwalke/MSAL_Telemetry_Cache_Dat…
a7432bc 
…a' into trwalke/MSAL_Telemetry_Cache_Data

# Conflicts:
#	Directory.Build.props
@jennyf19
Copy link
Collaborator

@trwalke do we have an ETA on wrapping this up? We are doing a release early next week. thanks.

@trwalke
Copy link
Member Author

trwalke commented Aug 15, 2023 •
edited
Loading

@trwalke do we have an ETA on wrapping this up? We are doing a release early next week. thanks.

Targeting end of week or a day before. I encountered an issue with MSAL tests as a result of some of MSAL's internal static caching which is causing tests to fail on this branch. It's not a bug, but it is something that has to be worked around in tests. Go ahead and release. @jennyf19

Removing unneeded use statements
73ff201 
resolving nullable warnings
@trwalke trwalke requested a review from jmprieur August 20, 2023 21:58
@bgavrilMS
Copy link
Member

All telemetry questions are sorted out. Cache Level is required data point for MSAL - irrespective of telemetry solution.

ITelemetryClient is not being deprecated for now, so it will appear there as well and it's acceptable way to test for this datapoint.

@bgavrilMS bgavrilMS merged commit 4d3491f into master Aug 24, 2023
@bgavrilMS bgavrilMS deleted the trwalke/MSAL_Telemetry_Cache_Data branch August 24, 2023 21:03
@jennyf19
Copy link
Collaborator

@bgavrilMS can you make sure someone from the Id Web side of things approves PRs in the future before merging them? Especially ones opened a few months ago. It's hard to keep track of what is ready to review and what is not.
Thanks

cc: @jmprieur @JoshLozensky @westin-m

@jennyf19
Copy link
Collaborator

jennyf19 commented Aug 25, 2023 •
edited
Loading

All telemetry questions are sorted out. Cache Level is required data point for MSAL - irrespective of telemetry solution.

ITelemetryClient is not being deprecated for now, so it will appear there as well and it's acceptable way to test for this datapoint.

also, we don't want ITelemetryClient that's not the solution we are going with. Where is that in the code? I'm not seeing it. Also, I had blocked the PR previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgavrilMS bgavrilMS bgavrilMS approved these changes

@jennyf19 jennyf19 Awaiting requested review from jennyf19

@jmprieur jmprieur Awaiting requested review from jmprieur

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trwalke @bgavrilMS @jennyf19 @jmprieur