* NEW: Add --disable-telemetry argument to disable telemetry collection
#975
Conversation
| @@ -0,0 +1,123 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
I do not know how to review this, you might need a second set of eyes for it. #Resolved
There was a problem hiding this comment.
the .net resource file is auto gen,
the only thing that is worth looking is the DefaultTelemetryConnectionString at the end.
| @@ -172,6 +173,11 @@ public override int Run(AnalyzeOptions analyzeOptions) | |||
| { | |||
| Stopwatch stopwatch = null; | |||
|
|
|||
| if (analyzeOptions.DisableTelemetry == true) | |||
| { | |||
| this.Telemetry = null; | |||
There was a problem hiding this comment.
This worries me a bit. I see that if it's null, we just don't add a certain logger to an aggregate logger. That looks great.
But then a little bit later, we pass this thing into a method. Does that call flow lead to uses? Is it already nullchecked? #Resolved
There was a problem hiding this comment.
yes it is,
in fact our application is checking if it is null to see enabled or not,
if null, no Telemetry will be performed.
There was a problem hiding this comment.
Scott's right, you haven't gotten this change quite right, which is to enable telemetry collection by default but allow someone to override that via this argument.
Where is the code that makes telemetry work out of box with no special configuration?
| @@ -70,5 +70,40 @@ public void MostlyFunctionlessCommandlineTest() | |||
| builder.Length.Should().Be(0, | |||
| $"all test cases should pass, but the following test cases failed:\n{builder}"); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void DisableTelemetryCommandlineTest() | |||
There was a problem hiding this comment.
I don't know what the rest of this suite looks like, and I'm not 100% sure I'm following this test. So tell me if this is here and I'm not seeing it. But: I think you need a test that disables telemetry, runs a scan against a well-known file (I see this much) and then verifies that the scan results are the same as if you had not passed in disable-telemetry (I don't see this part.)
#Resolved
There was a problem hiding this comment.
good input. Added another test.
|
@shaopeng-gh Release history looks good. |
| @@ -70,5 +70,40 @@ public void MostlyFunctionlessCommandlineTest() | |||
| builder.Length.Should().Be(0, | |||
| $"all test cases should pass, but the following test cases failed:\n{builder}"); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void DisableTelemetryCommandlineTest() | |||
If you have the telemetry connection string set,
you can use this new setting to quickly disable it without actually remove the setting,
so that your detail scan results will not be send.