Add telemetry to the console host to report platform and version by JamesWTruher · Pull Request #3620 · PowerShell/PowerShell

JamesWTruher · 2017-04-21T21:07:10Z

Produce simple telemetry via Application Insights for the PowerShell Core console host.

This is limited to the console host and is not meant as generalized telemetry code for PowerShell Core. It will capture the GitCommitID and Platform Information when the console host starts. It enables opting out of sending telemetry. This does not include the changes required in setup to opt out of telemetry, which will be in a later PR.

PaulHigin · 2017-04-21T21:48:14Z

$psscriptroot [](start = 40, length = 13)

This will create the file in the directory where Start-PSBuild is being run. But don't we want this location to be the output publish path? #Closed

The placement of the file in the actual publish directory is managed by putting it in the *.csproj files (same as powershell.version is done), but the file has to exist. #Closed

PaulHigin · 2017-04-21T21:54:41Z

This path shouldn't change per session so it seems they can also be static variables initialized on first use (for cases other then start up telemetry reporting). You could use a static constructor or static assignment.

SteveL-MSFT · 2017-04-21T21:54:57Z

we should set this to false now. if you need to test it, change it in your branch

also rename to _developerMode

fixed

In reply to: 112784357 [](ancestors = 112784357)

SteveL-MSFT · 2017-04-21T21:57:22Z

convention has been to use leading underscore _telemetryClient

SteveL-MSFT · 2017-04-21T21:58:23Z

shouldn't this be ConsoleHostStartup?

sure - fixed

In reply to: 112784414 [](ancestors = 112784414)

PaulHigin · 2017-04-21T22:03:18Z

if ( File.Exists(telemetrySemaphoreFilePath ) ) [](start = 12, length = 47)

Some of these method calls can throw (Access Denied, ArgumentException) so I think we need to wrap this in a try/catch to allow PowerShell to continue to run on telemetry send fail.

PaulHigin · 2017-04-21T22:04:26Z

🕐

PaulHigin

Please see my review comments

lzybkr · 2017-04-22T05:06:30Z

Can you measure the impact on startup?
I see that ApplicationInsights claims calls are non-blocking and telemetry is batched and sent on another thread, so there may be little or no impact.
I have a feeling we'll want to cross-gen the ApplicationInsights assembly though - JIT time is often noticeable.

i did some checking earlier (it was about 30ms), but will validate again since the code has changed to using a semaphore file. I will include that in the comments of the next push

In reply to: 112802910 [](ancestors = 112802910)

30ms is still quite a bit - it would be ~10% of total startup if we were matching performance of Windows PowerShell.

It would be good to know how much of that time is in JIT, see #3638 for some relative comparisons for time spent in JIT on other assemblies.

If the time is mostly in JIT (say > 95% of the time), then these changes are fine, and we can address cross-gen in the issue I've opened. If the time is not JIT time, then I think it should move to the background - for an example of where we do something similar in the console, see https://github.com/PowerShell/PowerShell/blob/master/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs#L52

@lzybkr initially I was also calculating a checksum of the SMA assembly which was removed, and now there's less than 2ms increase when sending telemetry. (see comments below).

lzybkr · 2017-04-22T05:09:23Z

To get PSHOME, we normally use this code:

var psHome = Utils.GetApplicationBase(Utils.DefaultPowerShellShellID);

excellent, will fix

In reply to: 112802942 [](ancestors = 112802942)

lzybkr · 2017-04-22T05:13:15Z

We have a fast version of File.Exists that we should probably use here for faster startup - it's Utils.NativeFileExists.

excellent, thanks
fixed

In reply to: 112802981 [](ancestors = 112802981)

add application insights reference Create telemetry class We will upload GitCommitId and OSVersion information This is just applicable to the console host, if we want to make it more general, we can refactor at that time

We need a way to allow the user to opt out of telemetry and with the config RFC still in flight, we don't have a centralized way to do that. This implements a semaphore file which when present will produce telemetry sends from the console host when it starts up. In order to disable telemetry, simply delete the file. Setup will need to be changed to handle this.

Also, protect the telemetry sending from exception. A failure to create the telemetry client should not be fatal to the shell starting.

SteveL-MSFT

LGTM

JamesWTruher · 2017-04-24T19:46:36Z

@lzybkr from a performance perspective, the cost is quite low. I did the following test with the semaphore file present and then not present:
measure-command { 1..60 | %{ ./powershell.exe -c "exit" }
the average difference was about +2ms per invocation when the semaphore file was present. I'm happy to use other methodologies, just let me know what you would like to see.

…tance have git ignore the telemetry semaphore file

daxian-dbw · 2017-04-24T23:58:19Z

+        /// The name of the file by when present in $PSHOME will enable telemetry.
+        /// If this file is not present, no telemetry will be sent.
+        /// </summary>
+        public const string TelemetrySemaphoreFilename = "DELETE_ME_TO_DISABLE_CONSOLEHOST_TELEMETRY";


Does this need to be public? It's recommended to not have a public const field because this will make changing this const value a breaking change to other assemblies that reference ConsoleHost.dll and use this const field.

More specifically, if another assembly uses it, this string will be embedded in that assembly instead of actually referencing ConsoleHost.dll and retrieving the value at runtime. So in future, say we have to change this string value, the existing assemblies that depend on this public const field will continue to use the old values even after it's changed in ConsoleHost.dll.

it doesn't need to be public. Do you think the TelemetrySemaphoreFilePath should also be private?

In reply to: 113082894 [](ancestors = 113082894)

I think it's better to be private because it's more like an implementation detail.

i will make it private - the path as well?

In reply to: 113084013 [](ancestors = 113084013)

daxian-dbw · 2017-04-24T23:59:13Z

+        /// </summary>
+        public static string TelemetrySemaphoreFilePath = Path.Combine(
+            Utils.GetApplicationBase(Utils.DefaultPowerShellShellID),
+            TelemetrySemaphoreFilename);


This also seems to be an implementation detail that shouldn't be exposed.

changed to private

In reply to: 113083044 [](ancestors = 113083044)

daxian-dbw · 2017-04-25T00:05:10Z

+    /// <summary>
+    /// send up telemetry for startup
+    /// </summary>
+    public class ApplicationInsightsTelemetry


It seems this could be declared as a static class, as it's not supposed to be instantiated.

changed

In reply to: 113083648 [](ancestors = 113083648)

daxian-dbw · 2017-04-25T00:07:02Z

+                if (Utils.NativeFileExists(TelemetrySemaphoreFilePath))
+                {
+                    TelemetryConfiguration.Active.InstrumentationKey = _psCoreTelemetryKey;
+                    TelemetryConfiguration.Active.TelemetryChannel.DeveloperMode = _developerMode;


Is this configuration a one-time thing, or do we have to do this configuration every time calling SendTelemetry? If it's a one-time thing, we probably can put it in the static constructor of this class.

right now, it's a one-time thing. I'll create a static constructor and move this inside

In reply to: 113083840 [](ancestors = 113083840)

daxian-dbw · 2017-04-25T00:13:52Z

+        {
+            var properties = new Dictionary<string, string>();
+            properties.Add("GitCommitID", PSVersionInfo.GitCommitId);
+            properties.Add("OSVersionInfo", Environment.OSVersion.VersionString);


Environment.OSVersion.VersionString doesn't give much useful information on different distro of Linux, for example, it show Unix 4.8.0.41 on Ubuntu 16.04. How about use [System.Runtime.InteropServices.RuntimeInformation]::OSDescription?

PS /> [System.Runtime.InteropServices.RuntimeInformation]::OSDescription Linux 4.8.0-41-generic #44~16.04.1-Ubuntu SMP Fri Mar 3 17:11:16 UTC 2017

Yes, it should be OSDescription, but I'm waiting for this to be available as part of PSVersionInfo, this is ok for now I think.

In reply to: 113084599 [](ancestors = 113084599)

I'd suggest changing to OSDescription now and using PSVersionTable is a change we can make later

daxian-dbw · 2017-04-25T00:17:18Z

+        /// <summary>
+        /// Create the startup payload and send it up
+        /// </summary>
+        public static void SendPSCoreStartupTelemetry()


Do we want to expose this API? By looking at the name, it's for a very specific scenario -- sending telemetry at PS startup time, and thus doesn't seem to be needed to be public.

I'll make it internal

In reply to: 113084933 [](ancestors = 113084933)

Sounds good. In that case, the class ApplicationInsightsTelemetry should also be made internal.

PaulHigin

LGTM

PaulHigin · 2017-04-24T21:24:52Z

I think this also needs to be under try/catch since TelemetrySempahoreFilePath static assignment will occur on first use.

And it can throw.

…sion create static constructor and initialize active telemetry in it change members to private rather than public

daxian-dbw · 2017-04-25T23:45:31Z

Minor comment: Some of the using directives seem not necessary, and it's better to have them cleaned up.

JamesWTruher requested review from PaulHigin, SteveL-MSFT and daxian-dbw April 21, 2017 21:07

msftclas added the cla-not-required label Apr 21, 2017

PaulHigin reviewed Apr 21, 2017

View reviewed changes

SteveL-MSFT requested changes Apr 21, 2017

View reviewed changes

PaulHigin reviewed Apr 21, 2017

View reviewed changes

PaulHigin suggested changes Apr 21, 2017

View reviewed changes

lzybkr reviewed Apr 22, 2017

View reviewed changes

JamesWTruher added 3 commits April 24, 2017 11:34

Add console host Application Insights telemetry

95acf77

add application insights reference Create telemetry class We will upload GitCommitId and OSVersion information This is just applicable to the console host, if we want to make it more general, we can refactor at that time

update telemetry class to use a static path for the semaphore file

c90d669

Also, protect the telemetry sending from exception. A failure to create the telemetry client should not be fatal to the shell starting.

JamesWTruher force-pushed the jameswtruher/telemetry02 branch from a8c84d4 to 0b2bd5e Compare April 24, 2017 19:33

SteveL-MSFT approved these changes Apr 24, 2017

View reviewed changes

changes to use standard PS apis to get PSHOME and check for file exis…

101a757

…tance have git ignore the telemetry semaphore file

JamesWTruher force-pushed the jameswtruher/telemetry02 branch from 0b2bd5e to 101a757 Compare April 24, 2017 22:14

daxian-dbw reviewed Apr 24, 2017

View reviewed changes

daxian-dbw reviewed Apr 25, 2017

View reviewed changes

PaulHigin approved these changes Apr 25, 2017

View reviewed changes

move class to internal and report on OSDescription rather than OS ver…

fcf9d82

…sion create static constructor and initialize active telemetry in it change members to private rather than public

daxian-dbw approved these changes Apr 25, 2017

View reviewed changes

daxian-dbw merged commit a2268ab into PowerShell:master Apr 26, 2017

SteveL-MSFT mentioned this pull request Apr 26, 2017

On-board and Enable telemetry for windows #1217

Closed

3 tasks

Conversation

JamesWTruher commented Apr 21, 2017

Uh oh!

PaulHigin Apr 21, 2017 • 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!

JamesWTruher Apr 22, 2017 • edited by PaulHigin 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!

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!

PaulHigin commented Apr 21, 2017

Uh oh!

PaulHigin 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!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

JamesWTruher commented Apr 24, 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!

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!

JamesWTruher Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

PaulHigin Apr 21, 2017 •

edited

Loading

JamesWTruher Apr 22, 2017 •

edited by PaulHigin

Loading

JamesWTruher Apr 25, 2017 •

edited

Loading

daxian-dbw Apr 25, 2017 •

edited

Loading