-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Additional Telemetry - Implementation of RFC0036 #10336
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Additional Telemetry - Implementation of RFC0036 #10336
Conversation
We will have |
TravisEz13
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haven't reviewed everything
src/System.Management.Automation/engine/hostifaces/ConnectionBase.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.PowerShell.ConsoleHost/Microsoft.PowerShell.ConsoleHost.csproj
Outdated
Show resolved
Hide resolved
the entire assembly should be removed
Use Should -Exist rather than test-path Fix test when environment variable is not set
fix case of '-Command' address other codefactor issue
fix one more test to exclude profile
bfe0ca7 to
0253f1f
Compare
We put it in the PowerShell.Create code rather than in this location
iSazonov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one minor comment.
|
#10050 was merged and now it is possible to add telemetry in the code too. |
Co-Authored-By: Ilya <darpa@yandex.ru>
|
@JamesWTruher All looks good and ready to merge. As @iSazonov said above, are the plans to add telemetry to |
|
@iSazonov @adityapatwardhan We can add additional telemetry for login to a subsequent PR if we decide it is worth knowing. I don't believe that we must do that now, though. |
src/System.Management.Automation/engine/CommandCompletion/CommandCompletion.cs
Outdated
Show resolved
Hide resolved
I don't need to keep the static strings of allowed module names or do convoluted checking of the experimental feature name. Move the initialization of the HashSet with list of module names into the static constructor. Ifdef a couple stopwatches out of the code (keeping the location tagged if we want to bring that back). Add a comment about POWERSHELL_DISTRIBUTION_CHANNEL. Change the type of the sessionid and uniqueid into strings so I don't repeatedly call ToString.
Optimize hashset creation to not create a new string array.
|
🎉 Handy links: |
this is the implementation of https://github.com/PowerShell/PowerShell-RFC/blob/249e8d88eb779a6003fd138609ae94417ae13698/2-Draft-Accepted/RFC0036-AdditionalTelemetry.md
PR Summary
PowerShell.Createis used, send a metricChannelallows us to understand when PS is being used in a docker instance and what the OS is.I added an internal member to
System.Management.Automation.PlatformcalledCacheDirectorywhich makes it easy to find the location of the cache directory. I think this should probably be public, but that can be decided at a later time.PR Context
We knew that our initial telemetry was just that. The bare minimum of information to determine whether the shell was actually being used and on what platform. The additional information we think we need is captured in the RFC.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.