Client-side telemetry is opt-in by default #4396

cormacpayne · 2017-08-01T20:36:34Z

Description

Fix for issue #4179 and #4330

Client-side telemetry is now opt-in by default
- The old AzureDataCollectionProfile.json file will be removed and replaced by AzurePSDataCollectionProfile.json
- Previously set environment variables will still be respected
- Updated warning messages for cmdlets to use for enabling/disabling telemetry
Added HashMacAddress to telemetry

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

General Guidelines

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

… telemetry

twitchax · 2017-08-01T20:39:06Z

src/Common/Commands.Common/MetricHelper.cs

@@ -52,6 +53,31 @@ public class MetricHelper
        /// </summary>
        private readonly object _lock = new object();

+        private static string _hashMacAddress = string.Empty;


Why not initialize to null?

I wanted null to represent that there was no valid NIC found. I used string.Empty to represent that we haven't tried to initialize the value of _hashMacAddress yet

twitchax · 2017-08-01T20:45:08Z

src/Common/Commands.Common/MetricHelper.cs

+                            _hashMacAddress = GenerateSha256HashString(macAddress).Replace("-", string.Empty).ToLowerInvariant();
+                            break;
+                        }
+                    }


This whole thing could be reduced to

var physicalAddress = nics.FirstOrDefault(nic => nic.OperationalStatus == OperationalStatus.Up)?.GetPhysicalAddress(); if(physicalAddress != null) _hashMacAddress = GenerateSha256HashString(macAddress.ToString()).Replace("-", string.Empty).ToLowerInvariant();

Sorry, not the whole thing...the loopy part.

I'll clean that up. I always forget about Linq 😄

twitchax · 2017-08-01T20:46:09Z

src/Common/Commands.Common/Properties/Resources.resx

-To enable data collection: PS &gt; Enable-AzureDataCollection
-
-Select Y to enable data collection [Y/N]:</value>
+Use the Disable-AzureRmDataCollection cmdlet to turn the feature Off. The cmdlet can be found in the AzureRM.Profile module.  To disable data collection: PS &amp;gt; Disable-AzureRmDataCollection.


Do we want the PS >?

cormacpayne · 2017-08-02T17:45:23Z

@azuresdkci test this please

markcowl · 2017-08-03T00:45:21Z

src/Common/Commands.Common/AzureDataCmdlet.cs

            InitializeDataCollectionProfile();

-            if (!_dataCollectionProfile.EnableAzureDataCollection.HasValue && CheckIfInteractive())
+            if (_dataCollectionProfile.EnableAzureDataCollection.HasValue || !CheckIfInteractive())


We should no longer need this (and in fact, we should remove this method if it is not used elsewhere (except tests)

markcowl · 2017-08-03T01:02:36Z

src/Common/Commands.Common/MetricHelper.cs

+
+        private static string HashMacAddress
+        {
+            get


Can we make this an instance value instead?

resolved offline - let's not test the value of _hashMacAddress in the code that sets the value.

twitchax · 2017-08-03T06:17:08Z

src/Common/Commands.Common/MetricHelper.cs

+                {
+                    var macAddress = NetworkInterface.GetAllNetworkInterfaces()
+                        .FirstOrDefault(nic => nic.OperationalStatus == OperationalStatus.Up)?
+                        .GetPhysicalAddress().ToString();


You actually need an elvis operator preceding the ToString() since the preceding code can evaluate to null.

var macAddress = NetworkInterface.GetAllNetworkInterfaces() .FirstOrDefault(nic => nic.OperationalStatus == OperationalStatus.Up)? .GetPhysicalAddress()?.ToString();

…ller

cormacpayne · 2017-08-08T02:35:05Z

On-demand: http://azuresdkci.cloudapp.net/view/1-AzurePowerShell/job/powershell-demand/1832/

markcowl · 2017-08-03T19:52:27Z

src/Common/Commands.Common/MetricHelper.cs

@@ -52,6 +54,24 @@ public class MetricHelper
        /// </summary>
        private readonly object _lock = new object();

+        private string _hashMacAddress = string.Empty;
+
+        private string HashMacAddress


this can be static

twitchax · 2017-08-08T07:41:05Z

👍

Opt in user to telemetry by default, send HashMacAddress as a part of…

dc0c059

… telemetry

cormacpayne assigned twitchax and markcowl Aug 1, 2017

msftclas added the cla-not-required label Aug 1, 2017

twitchax previously approved these changes Aug 1, 2017

View reviewed changes

azuresdkci added the needs-review label Aug 1, 2017

Resolve review feedback

1bbc4df

cormacpayne dismissed twitchax’s stale review via 1bbc4df August 1, 2017 22:34

markcowl requested changes Aug 3, 2017

View reviewed changes

Update HashMacAddress calculation

1b73dfc

markcowl added needs-revision and removed needs-review labels Aug 3, 2017

markcowl assigned cormacpayne and unassigned twitchax and markcowl Aug 3, 2017

twitchax reviewed Aug 3, 2017

View reviewed changes

Remove usage of CheckIfInteractive method in base cmdlet classes

d4bbb8f

cormacpayne changed the base branch from preview to release-4.3.0 August 4, 2017 16:04

cormacpayne added 4 commits August 4, 2017 14:55

Fix failing Profile test

4aeb0d1

Update Profile change log

e7d65f3

Move environment variable declaration to test constructor from contro…

33c51f7

…ller

Add extension methods to enable and disable data collection for tests

ff4bde9

cormacpayne added Waiting for On Demand and removed needs-revision labels Aug 7, 2017

cormacpayne removed their assignment Aug 7, 2017

Update failing Storage tests

fa08219

markcowl approved these changes Aug 8, 2017

View reviewed changes

markcowl merged commit 4fd2472 into Azure:release-4.3.0 Aug 8, 2017

cormacpayne deleted the telemetry-fix branch November 9, 2017 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Client-side telemetry is opt-in by default #4396

Client-side telemetry is opt-in by default #4396

Uh oh!

cormacpayne commented Aug 1, 2017 •

edited

Loading

Uh oh!

twitchax Aug 1, 2017

Uh oh!

cormacpayne Aug 1, 2017

Uh oh!

twitchax Aug 1, 2017

Uh oh!

twitchax Aug 1, 2017

Uh oh!

cormacpayne Aug 1, 2017

Uh oh!

twitchax Aug 1, 2017

Uh oh!

cormacpayne commented Aug 2, 2017

Uh oh!

markcowl Aug 3, 2017

Uh oh!

markcowl Aug 3, 2017

Uh oh!

markcowl Aug 3, 2017

Uh oh!

twitchax Aug 3, 2017

Uh oh!

cormacpayne commented Aug 8, 2017

Uh oh!

markcowl Aug 3, 2017

Uh oh!

twitchax commented Aug 8, 2017

Uh oh!

Uh oh!

Client-side telemetry is opt-in by default #4396

Client-side telemetry is opt-in by default #4396

Uh oh!

Conversation

cormacpayne commented Aug 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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 commented Aug 2, 2017

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 commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

twitchax commented Aug 8, 2017

Uh oh!

Uh oh!

cormacpayne commented Aug 1, 2017 •

edited

Loading