Skip to content
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

Lakshanf/issue2066/telemetry #2145

Merged
merged 10 commits into from Apr 6, 2016

Conversation

Projects
None yet
9 participants
@LakshanF
Copy link

commented Mar 30, 2016

.NET Core Tools Telemetry collection

Overview

In RC2 of the command line tools, we've started collecting usage telemetry. We've made this choice in order to help us better understand the usage patterns of the CLI tools, and will thus allow us to see which parts of the CLI should be improved first

The data collected will not have any personal data, such as usernames or emails or anything that can be used to identify the actual user. We will also not scan the code and we will not extract any project-level data that can be considered sensitive, such as name, repo or author (if you set those in your project.json).

The data that we collect will be shown publicly in due time. Due time here means when we have enough data to show and when we figure out how to make it accessible.

Behavior

The telemetry tracking is "on" by default.

There is a way to opt-out of the telemetry gathering process by setting an environment variable DOTNET_CLI_TELEMETRY_OPTOUT. Doing this will stop the collection process from running. In order to set the environment variable, please use the existing operating system mechanisms for this (e.g. export on OS X/Linux).

What do we collect?

We collect the following pieces of data:

  • The command being used (e.g. "build", "restore")
  • Arguments passed to the command
  • ExitCode of the command
  • For test projects, the test runner being used
  • Timestamp of invocation
  • Details about the project commands are invoked on
    • Framework used
    • If RIDs are present in the "runtimes" node
  • The CLI version being used

Questions, feedback, comments

If you have any questions, feedback or comment, please feel free to leave them either on our issues, in our Gitter channel. If you do file an issue for this, please be sure to tag @blackdwarf and @piotrMSFT so we would get notified.


This change is Reviewable

@dnfclas

This comment has been minimized.

Copy link

commented Mar 30, 2016

Hi @LakshanF, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;


public Telemetry()
{
if (_isInitialized)

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Add the brackets.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done


bool optout = Env.GetEnvironmentVariableAsBool(TelemetryOptout);

if (optout)

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Add brackets.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done


public void TrackCommand(string command, IDictionary<string, string> properties = null, IDictionary<string, double> measurements = null)
{
if (!_isInitialized)

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Add brackets.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done

Dictionary<string, double> eventMeasurements = new Dictionary<string, double>(_commonMeasurements);
if (measurements != null)
{
foreach (var m in measurements)

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

change the name of m to measurement. There is no need to use these short variable names here.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done

{
foreach (var m in measurements)
{
if (eventMeasurements.ContainsKey(m.Key))

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Backets.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done

Dictionary<string, string> eventProperties = new Dictionary<string, string>(_commonProperties);
if (properties != null)
{
foreach (var p in properties)

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

p to properties.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done

{
foreach (var p in properties)
{
if (eventProperties.ContainsKey(p.Key))

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Brackets.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done


namespace Microsoft.DotNet.Cli.Utils
{
public class Telemetry

This comment has been minimized.

Copy link
@livarcocc

livarcocc Mar 31, 2016

Member

Can you add an ITelemetry interface with the TrackCommand method and then use that in the dotnet Program? Then you can write a unit test that verifies that TrackCommand is being called with the appropriate parameters.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done in 4d1895d and ffd1c87

@@ -45,7 +44,7 @@ public static int Main(string[] args)

}

private static int ProcessArgs(string[] args)
public int ProcessArgs(string[] args, ITelemetry telemetryClient)

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 4, 2016

Member

Why is making this public and non-static needed?

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 4, 2016

Member

Looks like it is public so it can be unit tested. How about using InternalsVisibleTo instead?

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 5, 2016

Collaborator

Added InternalsVisibleTo but still verifying whether I did it correctly.

@@ -121,22 +120,39 @@ private static int ProcessArgs(string[] args)
["test"] = TestCommand.Run
};

int exitCode;
var arguments = string.Empty;

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 4, 2016

Member

Is arguments used anymore? It appears it is only set and not read.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

correct, it was being passed to TrackEvent but now we're giving that no information on arguments so it's never used. We could delete this code but it's going to need to be re-added shortly after we figure out the correct scrubbing of argument values for PII. I could leave it there and add a comment on the TrackEvent call to this effect or I could just delete this stuff if you prefer.

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 4, 2016

Member

I'd prefer to just remove it for now. Add it back when it is needed/used.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done

private static TelemetryClient _client = null;

private static Dictionary<string, string> _commonProperties = null;
private static Dictionary<string, double> _commonMeasurements = null;

This comment has been minimized.

Copy link
@MichaelSimons

MichaelSimons Apr 4, 2016

Contributor

I am not seeing the point of having _isInitialized, _client, _commonProperties, and _commonMeasurements be static. The appear to be initialized each time the ctor is called and only referenced by instance members. Seems like this code would be holding references unnecessarily and prevent GC.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Done, I think you're right, will change them to instance variables. The Telemetry class is functionally a singleton anyway so there aren't any savings from the statics.

catch (Exception)
{
// we dont want to fail the tool if telemetry fais. We should be able to detect abnormalities from data
// at the server end

This comment has been minimized.

Copy link
@MichaelSimons

MichaelSimons Apr 4, 2016

Contributor

I made a comment in the previous pull request that I didn't see a response on. "I agree with you that this should not fail and have a negative effect on what the end user is trying to achieve. That being said, do you think it would make sense to Debug.Fail or something like that so that there is a better chance issues will be caught during development in case something is wrong?"

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Added a simple log statement with Debug.Fail here and to the other empty catch block

This comment has been minimized.

Copy link
@MichaelSimons

MichaelSimons Apr 5, 2016

Contributor

Consider including the originating Exception in the message to make diagnostics easier.


private Dictionary<string, string> GetEventProperties(IDictionary<string, string> properties)
{
Dictionary<string, string> eventProperties = new Dictionary<string, string>(_commonProperties);

This comment has been minimized.

Copy link
@MichaelSimons

MichaelSimons Apr 4, 2016

Contributor

Is it necessary to declare the new dictionary in the case when no additional properties are specified? It seems heavy handed to be declaring and coping the commonProperties in this case. If I read the code correctly, this is the only code path that is exercised currently.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 4, 2016

Collaborator

Makes sense. Now just returns _commonProperties when properties is null, avoiding the allocation.

@danquirk danquirk force-pushed the lakshanf/issue2066/telemetry branch from ffd1c87 to b5e1042 Apr 5, 2016

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

Did a force push after rebasing as requested. Letting the CI server do its thing now.

@@ -44,7 +43,7 @@ public static int Main(string[] args)

}

private static int ProcessArgs(string[] args)
internal int ProcessArgs(string[] args, ITelemetry telemetryClient)

This comment has been minimized.

Copy link
@eerhardt

eerhardt Apr 5, 2016

Member

Does this still need to be instance? I think it should be static.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 5, 2016

Collaborator

Will do

Edit: pushed

@@ -0,0 +1,3 @@
using System.Runtime.CompilerServices;

[assembly: InternalsVisibleTo("dotnet.Tests")]

This comment has been minimized.

This comment has been minimized.

Copy link
@danquirk

danquirk Apr 5, 2016

Collaborator

Hm, wasn't sure, trying to run it locally now. I thought you only needed that if the target assembly was strong name signed and sn said this one wasn't. I'm probably wrong though.

Edit: looks like the CI server was happy without it

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

What's with the Ubuntu x64 Debug Build failure? The results don't make much sense to me.

@eerhardt

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

@dotnet-bot test Ubuntu x64 Debug Build

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

Different set of CI failures with the last push =\

@brthor

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

@danquirk This commit here should fix the failure you're seeing on windows:

068062f

cherry pick it?

@eerhardt

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

@dotnet-bot test RHEL7.2 x64 Release Build
@dotnet-bot test Windows_NT x86 Debug Build

@eerhardt

This comment has been minimized.

Copy link
Member

commented Apr 5, 2016

:shipit: When CI is green.

@eerhardt eerhardt referenced this pull request Apr 5, 2016

Closed

Telemetry PR #2066

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 5, 2016

@brthor do I still need that? The Windows build didn't fail on my previous push. Looks like you guys are used to some flakiness? Just wanted to make sure we didn't introduce anything new.

@brthor

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2016

@danquirk You don't need it, but it will remove the flakiness. I'll make a separate PR if you don't pick it up here.

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2016

Ok, I'm gonna wait to see what shakes out with release processes and whether I need to merge some of these PRs together. If I do that's probably a good time to cherry pick that. Otherwise I want to make sure this PR continues to look green for anyone looking to sign off.

@eerhardt

This comment has been minimized.

Copy link
Member

commented Apr 6, 2016

@danquirk - I wouldn't mess around trying to merge them together. I would just merge all 3 PRs at the same time. Trying to cherry pick the changes into 1 PR is prone to errors. We have 3 separate PRs all green. Just go through each one and merge.

I've already signed off, but I will again. :shipit:

@danquirk

This comment has been minimized.

Copy link
Collaborator

commented Apr 6, 2016

Yes I definitely agree but it is not up to me :) Waiting to hear exactly how to proceed.

@eerhardt eerhardt merged commit 965547b into rel/1.0.0 Apr 6, 2016

7 checks passed

CentOS7.1 x64 Debug Build Build finished. 544 tests run, 8 skipped, 0 failed.
Details
OSX x64 Release Build Build finished. 544 tests run, 8 skipped, 0 failed.
Details
RHEL7.2 x64 Release Build Build finished. 544 tests run, 8 skipped, 0 failed.
Details
Ubuntu x64 Debug Build Build finished. 554 tests run, 11 skipped, 0 failed.
Details
Ubuntu x64 Release Build Build finished. 554 tests run, 11 skipped, 0 failed.
Details
Windows_NT x64 Release Build Build finished. 544 tests run, 5 skipped, 0 failed.
Details
Windows_NT x86 Debug Build Build finished. 544 tests run, 5 skipped, 0 failed.
Details

@leecow leecow removed the in progress label Apr 6, 2016

@TheRealPiotrP TheRealPiotrP deleted the lakshanf/issue2066/telemetry branch Apr 6, 2016

@guardrex

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2016

I don't like the automatic "opt in" policy for this data sharing. I think you should ask the user with a checkbox in the MSI installer in addition to honoring the env var.

With regard to setting the env var, I just want to make it clear to others who stumble on to this (and it should have been in the Annoncements btw) ... set the value of DOTNET_CLI_TELEMETRY_OPTOUT to any of true, 1, or yes in order to opt-out.

@guardrex

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2016

... and I have to ask: Does this apply to servers where the shared framework is installed? The dotnet command will be run on servers, too.

@ghost

This comment has been minimized.

Copy link

commented May 18, 2016

Very bad idea. This hidden "feature" will scare away users and developers. Must be disabled by default!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.