Jmprieur/perf with scalable Public Client Application cache #614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
in a different file
to avoid the cache to grow (MSAL perf bug)
jmprieur
commented
Sep 25, 2020
| @@ -129,6 +123,13 @@ private async Task<AuthenticationResult> AcquireTokenAsync(int userIndex) | |||
| var scopes = new string[] { _configuration["ApiScopes"] }; | |||
| var upn = $"{NamePrefix}{userIndex}@{_configuration["TenantDomain"]}"; | |||
|
|
|||
| var _msalPublicClient = PublicClientApplicationBuilder | |||
There was a problem hiding this comment.
Recreating a PublicClientApplication each time to avoid the MSAL.NET performance bug which makes the cache grow with the number of users even if we store it in a different file (singleton)
jmprieur
commented
Sep 25, 2020
| @@ -14,29 +14,21 @@ static class TokenCacheHelper | |||
| /// </summary> | |||
| public static readonly string s_cacheFilePath = System.Reflection.Assembly.GetExecutingAssembly().Location + ".msalcache.bin3"; | |||
|
|
|||
| private static readonly object s_fileLock = new object(); | |||
|
|
|||
| public static void BeforeAccessNotification(TokenCacheNotificationArgs args) | |||
| { | |||
There was a problem hiding this comment.
The lock is not necessary and takes time
There was a problem hiding this comment.
yes, i already removed that
pmaytak
added a commit
that referenced
this pull request
Sep 25, 2020
* Perf project. * Add TestController. * Added Perf.Client project that creates and HttpClient and sends requests with a previously acquired token. * Updated to not using the token array. Small logging fixes. * Mostly updated to use AcquireTokenSilent instead of Tokens array. * Added a duration check to the inner loop. * Disable JWT Handler debug messages. Add Accept header. * Added empty action to weather controller. * serialize msal cache + add timers * Added three controllers with different dependencies to IntegrationTestServices. Minor TestRunner refactor. * Update Kesterl Url. * add msal logs * updates to perf testing w/app services * use distributed cache * fix constants in test * fix controller action * remove memory cache updates & fix test * try to fix tests * adding in timers around msal cache lookup * update testrunner part 5 * one more * Jmprieur/perf with scalable Public Client Application cache (#614) * Adding a scalable token cache that writes each entry in a different file * Moving the PCA creation in the loop to avoid the cache to grow (MSAL perf bug) * Adding comments Co-authored-by: jennyf19 <jeferrie@microsoft.com> * Fixing a couple of issues: - the AddMemoryCache override that takes a delegate does not accept null (fix done by Jenny lost in translation) - In the test runner allows to reduce the number of users Simplifying the matrix: - the UI test does not need to be on several frameworks (as it's not what is being tested) - the Integration test either (as the web API is only netcoreapp3.1) * Fixing the build Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com> Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
pmaytak
added a commit
that referenced
this pull request
Sep 30, 2020
* Perf project. * Add TestController. * Added Perf.Client project that creates and HttpClient and sends requests with a previously acquired token. * Updated to not using the token array. Small logging fixes. * Mostly updated to use AcquireTokenSilent instead of Tokens array. * Added a duration check to the inner loop. * Disable JWT Handler debug messages. Add Accept header. * Added empty action to weather controller. * serialize msal cache + add timers * Added three controllers with different dependencies to IntegrationTestServices. Minor TestRunner refactor. * Update Kesterl Url. * add msal logs * updates to perf testing w/app services * use distributed cache * fix constants in test * fix controller action * remove memory cache updates & fix test * try to fix tests * adding in timers around msal cache lookup * update testrunner part 5 * one more * Jmprieur/perf with scalable Public Client Application cache (#614) * Adding a scalable token cache that writes each entry in a different file * Moving the PCA creation in the loop to avoid the cache to grow (MSAL perf bug) * Adding comments Co-authored-by: jennyf19 <jeferrie@microsoft.com> * Fixing a couple of issues: - the AddMemoryCache override that takes a delegate does not accept null (fix done by Jenny lost in translation) - In the test runner allows to reduce the number of users Simplifying the matrix: - the UI test does not need to be on several frameworks (as it's not what is being tested) - the Integration test either (as the web API is only netcoreapp3.1) * Fixing the build Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com> Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com>
pmaytak
added a commit
that referenced
this pull request
Oct 2, 2020
* Perf project. * Add TestController. * Added Perf.Client project that creates and HttpClient and sends requests with a previously acquired token. * Updated to not using the token array. Small logging fixes. * Mostly updated to use AcquireTokenSilent instead of Tokens array. * Added a duration check to the inner loop. * Disable JWT Handler debug messages. Add Accept header. * Added empty action to weather controller. * serialize msal cache + add timers * Added three controllers with different dependencies to IntegrationTestServices. Minor TestRunner refactor. * Update Kesterl Url. * add msal logs * Added EventCounter counters for in-memory cache for read, write, remove operations. Added loop counter to test runner. * Jennyf/perf (#612) * Perf project. * Add TestController. * Added Perf.Client project that creates and HttpClient and sends requests with a previously acquired token. * Updated to not using the token array. Small logging fixes. * Mostly updated to use AcquireTokenSilent instead of Tokens array. * Added a duration check to the inner loop. * Disable JWT Handler debug messages. Add Accept header. * Added empty action to weather controller. * serialize msal cache + add timers * Added three controllers with different dependencies to IntegrationTestServices. Minor TestRunner refactor. * Update Kesterl Url. * add msal logs * updates to perf testing w/app services * use distributed cache * fix constants in test * fix controller action * remove memory cache updates & fix test * try to fix tests * adding in timers around msal cache lookup * update testrunner part 5 * one more * Jmprieur/perf with scalable Public Client Application cache (#614) * Adding a scalable token cache that writes each entry in a different file * Moving the PCA creation in the loop to avoid the cache to grow (MSAL perf bug) * Adding comments Co-authored-by: jennyf19 <jeferrie@microsoft.com> * Fixing a couple of issues: - the AddMemoryCache override that takes a delegate does not accept null (fix done by Jenny lost in translation) - In the test runner allows to reduce the number of users Simplifying the matrix: - the UI test does not need to be on several frameworks (as it's not what is being tested) - the Integration test either (as the web API is only netcoreapp3.1) * Fixing the build Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com> Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com> * changes to test runner (#615) * changes to test runner * fix user start value + add app insights logging * Update MicrosoftIdentityAppCallingWebApiAuthenticationBuilder.cs Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com> * Added EventCounter to distributed memory cache. Created folder EventSource with the new files. Added counters to App Insights telemetry. * Merge Henrikm/testrunner into pmaytak/perf (#634) * changes to test runner * fix user start value + add app insights logging * In memory db for test runner, and resilience in the test run itself, ensuring the runner continues to run until canceled by esc, or ctrl + c or ctrl + x * remove copy * update runner to not stop * Fix issue updating cache after refresh from AAD. Also update title to reflect summary of status * Added persistence of cache after each loop and loading of cache on start-up. This minimizes disk operations and enables restarting the console with-out having to fetch tokens again if those are still valid * Enable persistence if token was not obtained from cache * Testing Parallel.For and adding locks around file persistence. Cache and logging both are not threadsafe atm and will need more work. * Parrallel.For and Some persistence. Including a small pause between loops. * limiting persistence * removing parralel * Minor clean-up. Removing parrallel'ism. Requires more work and refactorings to make safe. Still not great code but better than previous iterations. * Update output display for test runner Co-authored-by: Jennyf19 <jeferrie@microsoft.com> Co-authored-by: Henrik Metzger <henrikm@microsoft.com> * Clean up. * Clean up. * Renamed Perf folder to PerformanceTests. Moved perf related code from IntegrationTestService to new project PerformanceTestService. * Fixes. * Fixes. * PR feedback. Co-authored-by: Jennyf19 <jeferrie@microsoft.com> Co-authored-by: Jean-Marc Prieur <jmprieur@microsoft.com> Co-authored-by: Henrik Metzger <henrikm@microsoft.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Jmprieur/perf with scalable Public Client Application cache in a different file for each account