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

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.

$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

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.

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)

convention has been to use leading underscore _telemetryClient

fixed

shouldn't this be ConsoleHostStartup?

sure - fixed

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

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.

🕐

Please see my review comments

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

Jason Shirk (@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).

To get PSHOME, we normally use this code:

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

excellent, will fix

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

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.

LGTM

Jason Shirk (@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

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)

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

changed to private

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

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)

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)

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

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.

LGTM

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

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

…erShell#3620) 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.

James Truher (JamesWTruher) requested review from Paul Higinbotham (PaulHigin), Steve Lee (SteveL-MSFT) and Dongbo Wang (daxian-dbw) April 21, 2017 21:07

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Apr 21, 2017

Paul Higinbotham (PaulHigin) reviewed Apr 21, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) requested changes Apr 21, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) reviewed Apr 21, 2017

View reviewed changes

Paul Higinbotham (PaulHigin) suggested changes Apr 21, 2017

View reviewed changes

Jason Shirk (lzybkr) reviewed Apr 22, 2017

View reviewed changes

James Truher (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.

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

Steve Lee (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

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

Dongbo Wang (daxian-dbw) reviewed Apr 24, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) reviewed Apr 25, 2017

View reviewed changes

Paul Higinbotham (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

Dongbo Wang (daxian-dbw) approved these changes Apr 25, 2017

View reviewed changes

Dongbo Wang (daxian-dbw) merged commit a2268ab into PowerShell:master Apr 26, 2017

Steve Lee (SteveL-MSFT) mentioned this pull request Apr 26, 2017

On-board and Enable telemetry for windows #1217

Closed

3 tasks

+                      /// 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";

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

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

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

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

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

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

Uh oh!

Add telemetry to the console host to report platform and version - #3620

Add telemetry to the console host to report platform and version#3620Dongbo Wang (daxian-dbw) merged 5 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom JamesWTruher:jameswtruher/telemetry02JamesWTruher/PowerShell-1:jameswtruher/telemetry02Copy head branch name to clipboard

Conversation

James Truher (JamesWTruher) commented Apr 21, 2017

Uh oh!

Paul Higinbotham (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!

James Truher (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!

Paul Higinbotham (PaulHigin) commented Apr 21, 2017

Uh oh!

Paul Higinbotham (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!

Steve Lee (SteveL-MSFT) left a comment

Choose a reason for hiding this comment

Uh oh!

James Truher (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

Add telemetry to the console host to report platform and version#3620
Dongbo Wang (daxian-dbw) merged 5 commits into
PowerShell:masterfrom
JamesWTruher:jameswtruher/telemetry02

Paul Higinbotham (PaulHigin) Apr 21, 2017 •
edited
Loading

James Truher (JamesWTruher) Apr 22, 2017 •
edited by PaulHigin
Loading

James Truher (JamesWTruher) Apr 25, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Apr 25, 2017 •
edited
Loading