Skip to content

Add ability to turn off telemetry in SignTool #7623

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.

Sign up for GitHub

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

Closed
epananth wants to merge 6 commits into from
Closed

Add ability to turn off telemetry in SignTool #7623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78ca6c9
turn off telemetry
epananth Jul 13, 2021
fee7998
add turnOffTelemetry to task
epananth Jul 13, 2021
c307473
typo
epananth Jul 13, 2021
5d6f3e6
Added some comments
epananth Jul 14, 2021
6f542ea
review comments
epananth Jul 14, 2021
5f81e18
remove comments
epananth Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Microsoft.DotNet.Arcade.Sdk/tools/Sign.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

<_DesktopMSBuildRequired>false</_DesktopMSBuildRequired>
<_DesktopMSBuildRequired Condition="'$(_DryRun)' != 'true' and '$(MSBuildRuntimeType)' == 'Core'">true</_DesktopMSBuildRequired>
<_DisableTelemetry>false</_DisableTelemetry>
<_DisableTelemetry Condition="'$(DisableTelemetry)' != ''">$(DisableTelemetry)</_DisableTelemetry>
</PropertyGroup>

<Error Condition="'$(AllowEmptySignList)' != 'true' AND '@(ItemsToSign)' == ''"
Expand Down Expand Up @@ -61,6 +63,7 @@
LogDir="$(ArtifactsLogDir)"
MSBuildPath="$(_DesktopMSBuildPath)"
SNBinaryPath="$(NuGetPackageRoot)sn\$(SNVersion)\sn.exe"
MicroBuildCorePath="$(NuGetPackageRoot)microbuild.core\$(MicroBuildCoreVersion)"/>
MicroBuildCorePath="$(NuGetPackageRoot)microbuild.core\$(MicroBuildCoreVersion)"
DisableTelemetry="$(_DisableTelemetry)/>
</Target>
</Project>
10 changes: 9 additions & 1 deletion src/Microsoft.DotNet.SignTool/src/SignToolTask.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@ public class SignToolTask : BuildTask
/// </summary>
public bool TestSign { get; set; }

/// <summary>
/// Enable or disable telemetry
/// </summary>
public bool DisableTelemetry {get; set;}

/// <summary>
/// True to perform strong name check on signed files.
/// If enabled it will require SNBinaryPath to be informed.
Expand Down Expand Up @@ -263,7 +268,10 @@ public void ExecuteImpl()
}
finally
{
telemetry.SendEvents();
if (!DisableTelemetry)
{
telemetry.SendEvents();
Copy link
Member

@chcosta chcosta Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a partial fix and adding an entire settable property for a partial fix isn't ideal. ie, if we are going to support the "DisableTelemetry" feature, then setting it to true should really disable the feature. Not just don't send events, but also, don't collect them.

I'm generally very anti-environment variables, but it seems like that would be a better solution here, particularly because we want the feature to be someone difficult to discover (a settable property makes it look like an acceptable practice rather than an exceptional case.

My preference would be that there is a "magic" env variable (only documented via comment in Telemetry.cs), which when set, turned telemetry.cs function calls into no-ops. This is a rare case where I prefer "magic". This would mean making the change entirely in Telemetry.cs rather than in SignToolTask.cs and the calling Sign.proj. Plus that telemetry class is already doing other magic with env variables, so it's not a huge stretch.

Copy link
Member Author

@epananth epananth Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will work on adding a flag to disable entire telemetry. Thank you for suggesting this change.

Copy link
Member

@markwilkie markwilkie Jul 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the real fix here is to find out why telemetry is not working just for Linux in the first place. I think Epsi has an issue tracking this, and it would be great if you could provide some guidance once we're able to focus on that issue. For now, we need to get this in with the hack....

Copy link
Member

@chcosta chcosta Jul 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we're all saying the same thing. Perhaps my nomenclature was misleading. I meant his is an incomplete implementation of the "hack". Nobody believes that this work is an actual "fix".

Copy link
Member Author

@epananth epananth Jul 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this in a new PR #7638

}
}
}

Expand Down