Collect Authentication Telemetry #27560

msJinLei · 2025-04-14T08:32:02Z

Description

The purpose of the PR is to send new property to Azure PowerShell telemetry to collection information of authentication

The following figure is what new properties look like

The details of auth-info-secondary
[{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":true},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":false},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":false},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":false},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":true},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":false},{"TokenCredentialName":"SharedTokenCacheCredential","AuthenticationSuccess":false}]
API Change
Add Interface to Send Auth Info to Telemetry azure-powershell-common#421

Mandatory Checklist

Please choose the target release of Azure PowerShell. (⚠️Target release is a different concept from API readiness. Please click below links for details.)
- General release
- Public preview
- Private preview
- Engineering build
- No need for a release
Check this box to confirm: I have read the Submitting Changes section of CONTRIBUTING.md and reviewed the following information:

SHOULD update ChangeLog.md file(s) appropriately
- For SDK-based development mode, update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
  - A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
- For autorest-based development mode, include the changelog in the PR description.
- Should not change ChangeLog.md if no new release is required, such as fixing test case only.
SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
SHOULD have proper test coverage for changes in pull request.
SHOULD NOT adjust version of module manually in pull request

azure-client-tools-bot-prd · 2025-04-14T08:32:09Z

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

This PR introduces authentication telemetry collection by adding and passing a new CmdletContext property throughout the authentication workflow. Key changes include:

Adding a new public ICmdletContext property in RMProfileClient and updating authentication methods to use it.
Updating various components and test files to register and pass the _cmdletContext as part of authentication requests.
Modifying methods in ContextAdapter and others to support the additional telemetry context.

Reviewed Changes

Copilot reviewed 52 out of 52 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Accounts/Accounts/Models/RMProfileClient.cs	Added ICmdletContext property and updated Authenticate method.
src/Accounts/Accounts/Environment/GetAzureRMEnvironment.cs	Instantiated RMProfileClient with new CmdletContext property.
src/Accounts/Accounts/Default/SetAzureRMDefault.cs	Updated GetServiceClientCredentials call to include _cmdletContext.
src/Accounts/Accounts/Default/GetAzureRmDefault.cs	Updated GetServiceClientCredentials call to include _cmdletContext.
src/Accounts/Accounts/Context/GetAzureRMContext.cs	Modified RefreshContextsFromCache to pass _cmdletContext.
src/Accounts/Accounts/CommonModule/EnvironmentExtensions.cs	Minor update to use default literal for improved readability.
src/Accounts/Accounts/CommonModule/ContextAdapter.cs	Extended authentication helper methods to accept and pass cmdletContext.
src/Accounts/Accounts/Common/AzureContextModificationCmdlet.cs	Updated object initialization to include CmdletContext property.
Test Files	Updated tests to register AuthenticationTelemetry component.
src/Accounts/Accounts/ChangeLog.md	Updated changelog to document telemetry changes.

src/Accounts/Accounts/ChangeLog.md

NoriZC · 2025-04-24T05:20:46Z

src/Accounts/Accounts/CommonModule/ContextAdapter.cs

+                {
+                    optionalParameters.Add(AuthenticationFactory.CmdletContextParameterName, extensibleParameters[AuthenticationFactory.CmdletContextParameterName]);
+                }
+                var authToken = _authenticator.Authenticate(context.Account, context.Environment, context.Tenant.Id, null, "Never", null, optionalParameters);


Could you please explain the change here?

NoriZC · 2025-04-24T05:46:22Z

src/Accounts/Authentication/Factories/AuthenticationFactory.cs

+            {
+                { TokenCacheParameterName, tokenCache },
+                { ResourceIdParameterName, resourceId }
+            };


why are these 2 parameters set as optional parameters? Especially when tokenCache do not have a default value.

dolauli · 2025-04-24T05:54:46Z

src/Accounts/Accounts/CommonModule/ContextAdapter.cs

@@ -118,7 +121,12 @@ internal void AddAuthorizeRequestHandler(
                {
                    endpointResourceIdKey = endpointResourceIdKey ?? AzureEnvironment.Endpoint.ActiveDirectoryServiceEndpointResourceId;
                    var context = GetDefaultContext(_provider, invocationInfo);
-                    return await AuthenticationHelper(context, endpointResourceIdKey, endpointSuffixKey, request, cancelToken, cancelAction, signal, next);
+                    ICmdletContext cmdletContext = AzureCmdletContext.CmdletNone;


AddAuthorizeRequestHandler is used in Autogen Data plane module. But extensibleParameters is actually not populated when this function is called in autogen module. So for autogen modules, we will not generate telemetry for authentication, is it right?

Another question is whether we need to add similar logic for onnewrequest, which is used by manage plane modules.

dolauli · 2025-04-24T06:11:10Z

src/Accounts/Accounts/CommonModule/EnvironmentExtensions.cs

@@ -342,7 +342,7 @@ internal static void CheckAndEnqueue<T>(this ConcurrentQueue<T> queue,  T item)

        internal static bool TryDequeueIfNotNull<T>(this ConcurrentQueue<T> queue, out T result)
        {
-            result = default(T);
+            result = default;


Any reason to change this?

NoriZC · 2025-04-24T06:45:08Z

src/Accounts/Authentication/KeyStore/AzKeyStore.cs

@@ -72,7 +72,7 @@ private void UpdateStorage(KeyStoreNotificationArgs args)
            helper.WriteToCachedStorage(args.KeyCache);
        }

-        public void Clear()
+        public virtual void Clear()


Could you please explain here? Who is inheriting current class and overriding current function?

NoriZC · 2025-04-24T07:07:17Z

src/Accounts/Authenticators/ClientAssertionAuthenticator.cs

            options.DisableInstanceDiscovery = spParameters.DisableInstanceDiscovery ?? options.DisableInstanceDiscovery;
            options.Diagnostics.IsTelemetryEnabled = false; // disable telemetry to avoid error thrown from Azure.Core that AssemblyInformationalVersion is null
            TokenCredential tokenCredential = new ClientAssertionCredential(tenantId, spParameters.ClientId, () => GetClientAssertion(spParameters), options);
+
+            base.CollectTelemetry(tokenCredential);
+            CheckTokenCachePersistanceEnabled = () =>


Just questioning - If CheckTokenCachePersistanceEnabled is implemented in most authenticators, can we define this function in a more common place?

Co-authored-by: NoriZC <[email protected]>

msJinLei · 2025-04-24T08:41:13Z

src/Accounts/Accounts/CommonModule/ContextAdapter.cs

@@ -75,7 +78,7 @@ public void OnNewRequest(InvocationInfo invocationInfo, string correlationId, st
        {
            prependStep(UniqueId.Instance.SendAsync);
            appendStep(new UserAgent(invocationInfo).SendAsync);
-            appendStep(this.SendHandler(GetDefaultContext(_provider, invocationInfo), AzureEnvironment.Endpoint.ActiveDirectoryServiceEndpointResourceId));
+            appendStep(this.SendHandler(GetDefaultContext(_provider, invocationInfo), AzureEnvironment.Endpoint.ActiveDirectoryServiceEndpointResourceId, new AzureCmdletContext(correlationId)));


double check correlationId

msJinLei added this to the Az 13.5.0 (05/06/2025) milestone Apr 14, 2025

msJinLei and others added 11 commits April 23, 2025 04:45

Add Telemetry Data

1641039

Add CmdletContext

15f1e86

Add Authentication Telemetry

9a16ebf

Fix failed test cases

a4bff65

Address review comments

c6842f4

Update common library

b73f1fb

Fix the pipeline error

fafc740

Update src/Accounts/Accounts/CommonModule/ContextAdapter.cs

0b0bc47

Co-authored-by: Copilot <[email protected]>

Fix typo

6d5575e

Add test case file

c293e6b

Add test cases to collect authentication telemetry

e90adbf

msJinLei force-pushed the jinlei/includeLatestCommon branch from f2c75d6 to e90adbf Compare April 22, 2025 20:50

Add TokenCredentials to differentiate broker and non-broker flows

1b37d7f

msJinLei marked this pull request as ready for review April 23, 2025 07:52

msJinLei requested review from NoriZC and Copilot April 23, 2025 08:33

Copilot AI reviewed Apr 24, 2025

View reviewed changes

msJinLei requested a review from dolauli April 24, 2025 02:40

NoriZC reviewed Apr 24, 2025

View reviewed changes

src/Accounts/Accounts/ChangeLog.md Outdated Show resolved Hide resolved

NoriZC reviewed Apr 24, 2025

View reviewed changes

dolauli reviewed Apr 24, 2025

View reviewed changes

NoriZC reviewed Apr 24, 2025

View reviewed changes

Update src/Accounts/Accounts/ChangeLog.md

86a8802

Co-authored-by: NoriZC <[email protected]>

msJinLei commented Apr 24, 2025

View reviewed changes

Split PR

ef831e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collect Authentication Telemetry #27560

Collect Authentication Telemetry #27560

msJinLei commented Apr 14, 2025 •

edited

Loading

azure-client-tools-bot-prd bot commented Apr 14, 2025

Copilot AI left a comment

NoriZC Apr 24, 2025

NoriZC Apr 24, 2025

dolauli Apr 24, 2025

dolauli Apr 24, 2025

dolauli Apr 24, 2025

NoriZC Apr 24, 2025 •

edited

Loading

NoriZC Apr 24, 2025

msJinLei Apr 24, 2025

Collect Authentication Telemetry #27560

Are you sure you want to change the base?

Collect Authentication Telemetry #27560

Conversation

msJinLei commented Apr 14, 2025 • edited Loading

Description

Mandatory Checklist

azure-client-tools-bot-prd bot commented Apr 14, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

NoriZC Apr 24, 2025

Choose a reason for hiding this comment

NoriZC Apr 24, 2025

Choose a reason for hiding this comment

dolauli Apr 24, 2025

Choose a reason for hiding this comment

dolauli Apr 24, 2025

Choose a reason for hiding this comment

dolauli Apr 24, 2025

Choose a reason for hiding this comment

NoriZC Apr 24, 2025 • edited Loading

Choose a reason for hiding this comment

NoriZC Apr 24, 2025

Choose a reason for hiding this comment

msJinLei Apr 24, 2025

Choose a reason for hiding this comment

msJinLei commented Apr 14, 2025 •

edited

Loading

NoriZC Apr 24, 2025 •

edited

Loading