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.

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

[Azure-Identity] VisualStudioCredential has "slow" subsequent GetTokenAsync calls #21958

Closed
vicinting opened this issue Jun 18, 2021 · 9 comments
Assignees
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-author-feedback More information is needed from author to address the issue. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@vicinting
Copy link

Description

I'm comparing between VisualStudioCredential and InteractiveBrowserCredential token credentials for local development and from my understanding, the first get token call should be the slowest as subsequent calls should used the cached token.

Expected behavior

Subsequent GetTokenAsync calls should be faster as a cached token is used

Actual behavior

While using VisualStudioCredential, subsequent calls are equivalent/comparable to the first call, around 800ms for me. See screenshots below for comparison between both credentials mentioned above.

InteractiveBrowserCredential

InteractiveBrowserCredential2

VisualStudioCredential

VisualStudioCredential2

Environment

  • Azure.Identity (1.4.0.0)
  • Windows 10 Pro (Build 19043)
  • Visual Studio Professional (16.10.2)
@ghost ghost added needs-triage This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Jun 18, 2021
@jsquire jsquire added Azure.Identity Client This issue points to a problem in the data-plane of the library. needs-team-attention This issue needs attention from Azure service team or SDK team labels Jun 18, 2021
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Jun 18, 2021
@jsquire
Copy link
Member

jsquire commented Jun 18, 2021

Thank you for your feedback. Tagging and routing to the team members best able to assist.

@christothes
Copy link
Member

I'm comparing between VisualStudioCredential and InteractiveBrowserCredential token credentials for local development and from my understanding, the first get token call should be the slowest as subsequent calls should used the cached token.

Hi @vicinting - VisualStudioCredential relies on the Microsoft.Asal.TokenService.exe utility shipped with Visual Studio to fetch tokens which, in turn, relies on MSAL. MSAL should be caching tokens, as long as the token request was successful.

It's possible the delay you are seeing is from something other than token caching. In my local testing using Fiddler, I noticed that it sends some traffic related to telemetry. This credential is meant for development use, so performance is not one of its goals.

@christothes christothes added the needs-author-feedback More information is needed from author to address the issue. label Jun 18, 2021
@ghost ghost removed the needs-team-attention This issue needs attention from Azure service team or SDK team label Jun 18, 2021
@vicinting
Copy link
Author

Thank you @christothes. I'll look into monitoring the network traffic as well but just provided this as feedback in case it was an issue.

The delay is not really a big issue but it could lead to misleading metrics to someone trying to work on performance. I'll just note it as a comment in our project, closing this issue for now.

@peter-bertok
Copy link

This credential is meant for development use, so performance is not one of its goals.

Performance should always be a goal, even at development time.

I'm seeing a full second added to every dependency call, which would otherwise take single-digit milliseconds to complete.

This is slowing down dev-test cycles unacceptably. It makes it impossible to determine the performance of code while in Visual Studio. This generally makes Azure.Identity an absolute no-go for our purposes.

@christothes
Copy link
Member

I'm seeing a full second added to every dependency call

Could you clarify which dependency calls are adding the time? If this is related to fetching credentials from AAD, this should only occur on the first call to the service if the tokens are being cached. If you are seeing additional dependency calls, I'm curious to know what they are.

This is slowing down dev-test cycles unacceptably. It makes it impossible to determine the performance of code while in Visual Studio. This generally makes Azure.Identity an absolute no-go for our purposes.

Azure.Identity does not need to use VisualStudioCredential even if the code is being tested inside Visual Studio. If your goal is performance measurement, it should be possible to eliminate this from your scenario.

@peter-bertok
Copy link

Azure.Identity does not need to use VisualStudioCredential even if the code is being tested inside Visual Studio.

This is the specific scenario that we need to "just work", so alternatives aren't helpful.

In the meantime I did more research and discovered that Azure.Identity has essentially no caching at all. Downstream consumers may opt to do caching, but this is inconsistent and unreliable.

If I can make a recommendation: avoid the use of the words "you" or "your" when thinking about or communicating with the users of these SDKs. This is an enormous ecosystem of related libraries, and "I" don't get to control how they interact with each other. Even Microsoft is not in full control of this -- there are SDKs written by third parties that use Azure.Identity internally, and I may not have any choice in how those SDKs are used.

The performance issues with Azure.Identity appear to be fundamental and cannot be consistently resolved by individual end-users. The only thing that can be done is for Microsoft to ensure that all scenarios are fast, and that downstream users fall into the pit of success instead of the pit of failure. Right now, there are people complaining about 429 errors in all sorts of related libraries because Microsoft hasn't done this consistently or correctly enough. They're falling in to the pit of failure, because that's the default path.

@christothes
Copy link
Member

In the meantime I did more research and discovered that Azure.Identity has essentially no caching at all. Downstream consumers may opt to do caching, but this is inconsistent and unreliable.

Although the documentation indicates that caching is the callers responsibility, this is not entirely true. Some credentials do cache because of the underlying implementation provided by MSAL. We also allow for some configuration of token cache for scenarios where sharing cache across credential instances is needed. See this sample for more information.

The performance issues with Azure.Identity appear to be fundamental and cannot be consistently resolved by individual end-users. The only thing that can be done is for Microsoft to ensure that all scenarios are fast, and that downstream users fall into the pit of success instead of the pit of failure. Right now, there are people complaining about 429 errors in all sorts of related libraries because Microsoft hasn't done this consistently or correctly enough. They're falling in to the pit of failure, because that's the default path.

I apologize if you have encountered any performance issues with Azure.Identity. If there is a specific scenario we can assist with, please feel free to create an issue with the details and we will do our best to assist.

@Jawvig
Copy link

Jawvig commented Mar 12, 2022

Thanks for the sample link, @christothes. Unfortunately it looks like you can't pass TokenCachePersistenceOptions to DefaultAzureCredentials and have this propagate it down the chain.

My use case is that I'm running local xunit integration tests that test code use Entity Framework to perform some operations on an Azure SQL database. The tests make about 5 database calls each on < 10 rows of data, pretty simple. However, each test takes about 20s to run inside Visual Studio. In the DbContext I'm using the token based authentication as laid out in Mikaël's blog here: https://devblogs.microsoft.com/azure-sdk/azure-identity-with-sql-graph-ef/

Note that whilst it stores the ChainedTokenCredential statically, it does not cache the tokens retrieved.

The difference in my case is I'm using DefaultAzureCredential in the interceptor rather than new ChainedTokenCredential(new ManagedIdentityCredential(), new EnvironmentCredential()). We run the code under test in a variety of contexts - Visual Studio Test Explorer, dotnet test in Azure pipelines and then the code itself in both console apps and Azure functions with managed identities. DefaultCredential has been really handy to support all of these.

When I profiled the tests I could see that most of the time was spent by the VisualStudioCredential running processes:
image

I have a few different accounts logged into Visual Studio for different tenants. I noticed that DefaultAzureCredentialOptions supports SharedTokenCacheTenantId. From the code docs:

"Specifies the tenant id of the preferred authentication account, to be retrieved from the shared token cache for single sign on authentication with development tools, in the case multiple accounts are found in the shared token.
If multiple accounts are found in the shared token cache and no value is specified, or the specified value matches no accounts in the cache the SharedTokenCacheCredential will not be used for authentication."

However, specifying the required tenant id in this field did not alter the performance as far as I could see.

I'm nervous about caching the tokens as there's no information about their expiry and caching beyond their expiry would lead to an annoying class of bugs. But is that the way to go?

I appreciate any help here.

@christothes
Copy link
Member

christothes commented Mar 14, 2022

Hi @Jawvig - VisualStudioCredential does not participate in the cache options since we are just calling out to visual studio's token provider (Microsoft.Asal.TokenService.exe) out of proc. One option is to use InteractiveBrowswerCredential instead of VisualStudioCredential for your development time credential. If the cache options are configured for persistence as in the sample, this should allow the token to be cached across test runs.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-author-feedback More information is needed from author to address the issue. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

No branches or pull requests

6 participants