Fix for #4965, #6274

praries880 · 2018-05-22T21:07:33Z

Added InvocationName, InnerExceptionTypie and ParameterSetName to the telemetry emitted.

cormacpayne

@praries880 two minor comments, otherwise LGTM

Also, would you mind updating the change log for Profile to reflect the changes being made in this PR?

cormacpayne · 2018-05-23T22:10:59Z

src/Common/Commands.Common/MetricHelper.cs

@@ -266,6 +270,12 @@ private void PopulatePropertiesFromQos(AzurePSQoSEvent qos, IDictionary<string,
            eventProperties.Add("HashMacAddress", HashMacAddress);
            eventProperties.Add("PowerShellVersion", PSVersion);
            eventProperties.Add("Version", AzurePowerShell.AssemblyVersion);
+            eventProperties.Add("CommandParameterSetName", qos.ParameterSetName);
+            if (!string.IsNullOrWhiteSpace(qos.InvocationName))


@praries880 why do we need this if-statement? I didn't think it was possible for InvocationName to be null or empty

well, now I know!!!

cormacpayne · 2018-05-23T22:14:32Z

src/ResourceManager/Common/Commands.ResourceManager.Common/AzureRMCmdlet.cs

@@ -214,6 +219,11 @@ protected override void InitializeQosEvent()
                        s => string.Format(CultureInfo.InvariantCulture, "-{0} ***", s)));
            }

+            if (!string.IsNullOrWhiteSpace(this.ParameterSetName))


@praries880 since we don't need to do any additional checks (like we do with this.MyInvocation in the if-statements above), is there any reason we can't move the initialization of this property into the instantiation of the AzurePSQoSEvent object above?

_qosEvent = new AzurePSQoSEvent() { CommandName = commandAlias, ... ParameterSetName = this.ParameterSetName }

Also, I don't believe ParameterSetName can ever be null or empty (unless the cmdlet creator gives an empty set name, which doesn't happen in our cmdlets 😀 )

didnt know that either ;)

Added InnerExceptionTypie and ParameterSetName to the telemetry emitted. Please enter the commit message for your changes. Lines starting

cormacpayne · 2018-05-24T15:58:08Z

src/Common/Commands.Common/MetricHelper.cs

+                {
+                    //Increment the inner exception count so that we can tell which is the outermost
+                    //and which the innermost
+                    eventProperties.Add("InnerExceptionType-"+exceptionCount++, innerEx.GetType().ToString());


@praries880 do we need to return all of the inner exceptions? I want to make sure we're not cluttering the telemetry with information that may or may not be relevant to us

@cormacpayne I had initially missed this piece in the linked issue :"InnerExceptionType (shouls contain a list of inner exception types)"

Hence I am adding all the inner exceptions. I can create a comma separated list of exceptions and add it to the dictionary, but I thought it might be better to return each one as a separate item in the dictionary.

cormacpayne · 2018-05-24T15:58:22Z

src/ResourceManager/Common/Commands.ResourceManager.Common/AzureRMCmdlet.cs

@@ -204,7 +204,13 @@ protected override void InitializeQosEvent()
                ClientRequestId = this._clientRequestId,
                SessionId = _sessionId,
                IsSuccess = true,
-            };
+                ParameterSetName = this.ParameterSetName
+        };


@praries880 nit: the indentation for this curly brace is off

will fix it

cormacpayne

@praries880 LGTM

@twitchax would you mind checking this PR out when you get the chance?

twitchax

A few comments; otherwise, LGTM.

twitchax · 2018-05-29T07:43:37Z

src/Common/Commands.Common/MetricHelper.cs

@@ -226,6 +226,16 @@ private void LogExceptionEvent(AzurePSQoSEvent qos)
                eventProperties.Add("Message", "Message removed due to PII.");
                eventProperties.Add("StackTrace", qos.Exception.StackTrace);
                eventProperties.Add("ExceptionType", qos.Exception.GetType().ToString());
+                Exception innerEx = qos.Exception.InnerException;


I forget, how do we feel about var?

not sure :)
Having said that... i would use a var when dealing with anonymous types.. in this case i odnt foresee it changeing from Exception, so i explicitly use the type.

twitchax · 2018-05-29T07:46:21Z

src/ResourceManager/Common/Commands.ResourceManager.Common/AzureRMCmdlet.cs

+            {
+                _qosEvent.InvocationName = this.MyInvocation.InvocationName;
+            }
+


Why not replace this whole thing (if-statement and all) with:

_qosEvent.InvocationName = this.MyInvocation?.InvocationName;

Or, can this.MyInvocation.InvocationName actually be whitespace?

Not sure... What about the exception scenarios? Just wanted to play on the safer side and pass along only valid values.

@twitchax I don't believe that this.MyInvocation.InvocationName can be whitespace, but it never hurts to make the check 😀

praries880 · 2018-05-29T22:55:05Z

@cormacpayne can you kindly sign off again, took care of a merge conflict.

praries880 · 2018-05-30T16:33:20Z

on demand : https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/897/

praries880 added Azure PS Team needs-review labels May 22, 2018

praries880 assigned cormacpayne May 22, 2018

praries880 requested a review from cormacpayne May 22, 2018 21:07

maddieclayton removed the Azure PS Team label May 22, 2018

azuresdkci added the needs-review label May 22, 2018

cormacpayne suggested changes May 23, 2018

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels May 23, 2018

cormacpayne assigned praries880 and unassigned cormacpayne May 23, 2018

praries880 added 3 commits May 23, 2018 15:48

Fix for Azure#4965,

8b61bc8

Added InnerExceptionTypie and ParameterSetName to the telemetry emitted. Please enter the commit message for your changes. Lines starting

Add invocation name to the telemetry

41fd1c7

Took care of review comments

fdbcfb6

praries880 force-pushed the update_telemetry branch from ef94dc9 to fdbcfb6 Compare May 23, 2018 23:17

Fix typo

f0eac80

praries880 assigned cormacpayne and unassigned praries880 May 23, 2018

praries880 added needs-review and removed needs-revision labels May 23, 2018

cormacpayne reviewed May 24, 2018

View reviewed changes

cormacpayne assigned praries880 and unassigned cormacpayne May 24, 2018

cormacpayne added needs-revision and removed needs-review labels May 24, 2018

Fix indentation

51448db

praries880 assigned cormacpayne and unassigned praries880 May 24, 2018

praries880 added needs-review and removed needs-revision labels May 24, 2018

cormacpayne previously approved these changes May 25, 2018

View reviewed changes

cormacpayne assigned twitchax and unassigned cormacpayne May 25, 2018

twitchax previously approved these changes May 29, 2018

View reviewed changes

maddieclayton assigned praries880 and unassigned twitchax May 29, 2018

maddieclayton added needs-revision and removed needs-review labels May 29, 2018

Merge branch 'preview' into update_telemetry

8f3b381

praries880 dismissed stale reviews from twitchax and cormacpayne via 8f3b381 May 29, 2018 22:54

praries880 assigned cormacpayne May 29, 2018

praries880 added needs-review and removed needs-revision labels May 29, 2018

cormacpayne approved these changes May 30, 2018

View reviewed changes

cormacpayne changed the base branch from preview to release-6.2.0 May 30, 2018 16:50

praries880 merged commit fd20fba into Azure:release-6.2.0 May 30, 2018

Fix for #4965, #6274

Fix for #4965, #6274

Uh oh!

Conversation

praries880 commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cormacpayne left a comment

Choose a reason for hiding this comment

Uh oh!

twitchax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praries880 commented May 29, 2018

Uh oh!

praries880 commented May 30, 2018

Uh oh!

Uh oh!

praries880 commented May 22, 2018 •

edited

Loading

twitchax left a comment •

edited

Loading