Add Interface to Send Auth Info to Telemetry #421

msJinLei · 2024-06-12T05:48:44Z

The info added
- auth-info-head-tokencredentialname
- auth-info-head-authenticationsuccess
- auth-info-head-tokencacheenabled
Example To update later

isra-fel

1* Cmdlet telemetry : N * auth telemetry. Conclusion: data structure => array, but only 1 element. Todo: test if array is OK to be stored in kusto?
telemetry key naming convention. Conclusion: use dash. General conclusion: ?
Scope. Can we leverage MSAL telemetry? Todo: remove AuthorityUri. Consider adding correlation ID later. Consider enable telemetry of -Environment parameter.
leverage Beisi & Lei's design. Conclusion: not much.
Codegen => Todo: next sprint. Create task.
TokenCacheEnabled => nice to have. Consider risk & effort of testing.

src/Authentication.Abstractions/AuthenticationInfo.cs

isra-fel · 2024-06-24T10:21:41Z

src/Authentication.Abstractions/AuthenticationInfo.cs

+        /// <summary>
+        /// Authentication process succeed or not.
+        /// </summary>
+        public bool AuthenticationSuccess { get; set; } = false;


Why presuming it false? Or does it even matter?

Prefer to set it as negative result biased.

isra-fel · 2024-06-24T10:24:05Z

src/Authentication.Abstractions/AuthenticationInfo.cs

+            TokenCredentialName = null;
+        }
+
+        public AuthenticationInfo(IAuthenticationInfo other, bool? isSuccess = null)


Why is isSuccess bool? ? In what case will it be null?

Why is isSuccess bool? ? In what case will it be null?

If it is null, use the value in other. The case doesn't exist in current code.

isra-fel · 2024-06-24T23:19:57Z

src/Authentication.Abstractions/Interfaces/IAuthenticationFactory.cs

+        /// <summary>
+        /// Get the information to be recorded in Telemetry
+        /// </summary>
+        IList<IAuthenticationInfo> GetDataForTelemetry();


Updating a core interface can be risky. We must

Update all implementation of this interface. I believe there's one in Accounts and another in Accounts' tests.

Test collaboration of new Accounts and older version of service modules, especially if they use this interface.

src/Common/AzurePSCmdlet.cs

src/Common/MetricHelper.cs

msJinLei · 2024-08-21T02:44:03Z

No change since last review

MetricHelper
AuthTelemetryRecord

msJinLei · 2024-08-21T03:08:28Z

1* Cmdlet telemetry : N * auth telemetry. Conclusion: data structure => array, but only 1 element. Todo: test if array is OK to be stored in kusto?

Cannot use array directly, the current solution is to expand the first (most important) authentication telemetry record and serialize the other parts into a json string

telemetry key naming convention. Conclusion: use dash. General conclusion: ?

Scope. Can we leverage MSAL telemetry? Todo: remove AuthorityUri. Consider adding correlation ID later. Consider enable telemetry of -Environment parameter.
CorrelationId is added but not way to get id from MSAL, need MSAL team to support

leverage Beisi & Lei's design. Conclusion: not much.

Codegen => Todo: next sprint. Create task.
Done, need more test

TokenCacheEnabled => nice to have. Consider risk & effort of testing.
removed

BethanyZhou · 2024-08-22T02:02:11Z

src/Authentication.Abstractions/AzureCmdletContext.cs

+    /// </summary>
+    public class AzureCmdletContext : ICmdletContext
+    {
+        private string cmdletId;


Suggested change

private string cmdletId;

private string _cmdletId;

suggest using '_' as the prefix of private field.

We have both private data members that start with lower case and "_". Let get a unified decision for it.

BethanyZhou · 2024-08-22T02:12:54Z

src/Common/MetricHelper.cs

@@ -292,6 +292,20 @@ public void SetPSHost(PSHost host)
        {
        }

+        private static void PopulateAuthenticationPropertiesFromQos(AuthenticationTelemetryData telemetry, IDictionary<string, string> eventProperties)


Suggested change

private static void PopulateAuthenticationPropertiesFromQos(AuthenticationTelemetryData telemetry, IDictionary<string, string> eventProperties)

private static void PopulateAuthenticationPropertiesFromQos(AzurePSQoSEvent qos, IDictionary<string, string> eventProperties)

because the naming of this function is ...FromQos

Good point. Will also discuss with yeming about the naming

BethanyZhou · 2024-08-22T02:15:20Z

src/Authentication.Abstractions/Interfaces/IClientFactory.cs

+        /// <param name="cmdletContext">The cmdlet context called from</param>
+        /// <returns>A client properly authenticated in the given context, properly configured for use with Azure PowerShell, 
+        /// targeting the given named endpoint in the targeted environment</returns>
+        TClient CreateArmClient<TClient>(IAzureContext context, string endpoint, ICmdletContext cmdletContext) where TClient : ServiceClient<TClient>;


Could you explain why creating an ArmClient needs cmdletContext?

A lot of partners use Arm Client to handle the authentication. If we want to collect authentication telemetry from their cmdlets, a good way is to add cmdlet context here.
Also the CmdletContext can be treated as the same status as AzureContext. We can pass them both in the same interface.

msJinLei · 2024-09-11T02:51:41Z

src/Common/MetricHelper.cs

+            var record = telemetry?.Head;
+            if (record != null)
+            {
+                eventProperties[$"{AuthTelemetryRecord.AuthTelemetryPropertyHeadPrefix}-{nameof(record.TokenCredentialName).ToLower()}"] = record.TokenCredentialName;


Move out of metric helper

msJinLei force-pushed the jinlei/telemetry branch 2 times, most recently from eeb4c86 to ff84672 Compare June 17, 2024 02:35

isra-fel requested changes Jun 18, 2024

View reviewed changes

msJinLei force-pushed the jinlei/telemetry branch from ff84672 to f6a9cea Compare June 24, 2024 08:17

isra-fel requested changes Jun 24, 2024

View reviewed changes

msJinLei force-pushed the jinlei/telemetry branch 3 times, most recently from ce45e53 to 39e6304 Compare July 26, 2024 08:14

msJinLei force-pushed the jinlei/telemetry branch 4 times, most recently from 9f1fa26 to eabb4e3 Compare August 7, 2024 05:51

msJinLei added 4 commits August 15, 2024 09:12

Add Interface to Send Auth Info to Telemetry

beaf14e

Address review comments

ccc48dc

Address review comments

439c8e8

Address review comments

0358f72

msJinLei force-pushed the jinlei/telemetry branch 5 times, most recently from 9174668 to e4b4dc9 Compare August 18, 2024 22:38

Add CmdletContext

8468613

msJinLei force-pushed the jinlei/telemetry branch 2 times, most recently from a693ddf to 4574c39 Compare August 20, 2024 13:24

Improve the interface

b5b77e1

msJinLei force-pushed the jinlei/telemetry branch 4 times, most recently from 9415a08 to 662702c Compare August 21, 2024 02:19

Revise the comments

92308c0

msJinLei force-pushed the jinlei/telemetry branch from 662702c to 92308c0 Compare August 21, 2024 02:31

Fix the test

6d22fbe

msJinLei marked this pull request as ready for review August 21, 2024 03:05

msJinLei requested a review from isra-fel August 21, 2024 03:09

BethanyZhou reviewed Aug 22, 2024

View reviewed changes

Forget to commit the change

96f4a5d

msJinLei commented Sep 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Interface to Send Auth Info to Telemetry #421

Add Interface to Send Auth Info to Telemetry #421

msJinLei commented Jun 12, 2024 •

edited

Loading

isra-fel left a comment

isra-fel Jun 24, 2024

msJinLei Jun 25, 2024

isra-fel Jun 24, 2024

msJinLei Jun 25, 2024

isra-fel Jun 24, 2024

msJinLei commented Aug 21, 2024

msJinLei commented Aug 21, 2024

BethanyZhou Aug 22, 2024

msJinLei Aug 26, 2024

BethanyZhou Aug 22, 2024

msJinLei Aug 26, 2024

BethanyZhou Aug 22, 2024

msJinLei Aug 26, 2024

msJinLei Sep 11, 2024

	private static void PopulateAuthenticationPropertiesFromQos(AuthenticationTelemetryData telemetry, IDictionary<string, string> eventProperties)
	private static void PopulateAuthenticationPropertiesFromQos(AzurePSQoSEvent qos, IDictionary<string, string> eventProperties)

Add Interface to Send Auth Info to Telemetry #421

Are you sure you want to change the base?

Add Interface to Send Auth Info to Telemetry #421

Conversation

msJinLei commented Jun 12, 2024 • edited Loading

isra-fel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msJinLei commented Aug 21, 2024

msJinLei commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msJinLei commented Jun 12, 2024 •

edited

Loading