For build-from-source builds of .NET, opt-out of Microsoft telemetry #25935

tmds · 2022-06-09T12:36:39Z

We opt-out by setting DOTNET_CLI_TELEMETRY_OPTOUT at 'dotnet' Main
to ensure that anything that checks the variable later, including MSBuild Tasks,
will see the value.

Fixes #24474

cc @omajid @richlander @baronfel @jkotas

richlander · 2022-06-09T14:42:05Z

This isn't quite what I was expecting. Disable telemetry is still relying on setting an ENV. Don't we want something much more fundamental, where no ENVs are set by default AND telemetry is not enabled?

tmds · 2022-06-09T15:20:19Z

This isn't quite what I was expecting. Disable telemetry is still relying on setting an ENV. Don't we want something much more fundamental, where no ENVs are set by default AND telemetry is not enabled?

There are two ways to implement this:

Either, when the program starts, set the env to not enable telemetry (when it was not set).
Or, update all places where where the env is read so the default value matches the build-time setting.

I've opted for the first because that was simplest to implement.
I can change to the second if that is preferred. That will require changing all code that reads the envvar. And, anything that checks the envvar outside this repo will not be aware telemetry should be disabled by default.

richlander · 2022-06-09T15:47:19Z

It feels like the other option is the "right" architectural choice. That approach would mean there is a literal guarantee that telemetry can never be enabled through some accidental gesture.

src/Cli/dotnet/Program.cs

kasperk81 · 2022-06-10T19:33:22Z

this is also needed in src/WebSdk/Publish/Tasks/WebConfigTelemetry.cs?

tmds · 2022-06-13T06:58:32Z

this is also needed in src/WebSdk/Publish/Tasks/WebConfigTelemetry.cs?

Yes, this is needed everywhere where something reads the envvar. And why I mentioned MSBuild Tasks in the initial message.

Imo the safest is to set the envvar at startup to ensure every location that checks it (which maybe does not live in this repo?) is aware telemetry is disabled.

@richlander indicates he expects the PR to update locations that check to envvar in this repo to be updated. If the repo owners confirm this is their preference also, I'll update the PR to use that approach.

KalleOlaviNiemitalo · 2022-06-13T07:40:27Z

I thought you could have a test that searches for "DOTNET_CLI_TELEMETRY_OPTOUT" in the binaries after a source build, but:

The string is encoded in UTF-16 in the UserString heap but in UTF-8 elsewhere, complicating the search.
It is okay for Microsoft.DotNet.Configurer.dll to contain the TelemetryMessage string resource that mentions the environment variable, even if telemetry was disabled at compile time.

tmds · 2022-06-14T12:39:44Z

@KalleOlaviNiemitalo for this PR I'd like to scope to make the behavior opt-in rather than exclude it. If excluding gets implemented, that can be a useful test.

tmds · 2022-06-14T12:48:14Z

@dsplaisted @baronfel question: do you prefer this being implemented as:

DOTNET_CLI_TELEMETRY_OPTOUT envvar getting set at start, so it affects all readers of the envvar.
or by updating all locations where DOTNET_CLI_TELEMETRY_OPTOUT gets read to use a different defaultValue?

KalleOlaviNiemitalo · 2022-06-14T13:43:55Z

Is the environment variable read by any tools that are published to NuGet.org? Perhaps, even if a user installs a source-built SDK and does not expect it to send telemetry, the user might not expect this SDK to tell separately-installed tools not to send telemetry either.

richlander · 2022-06-14T14:52:29Z

We are typically hesitant for the SDK to set ENVs automatically (for any scenario). It can have unexpected consequences across process boundaries. I'm unaware of any scenario where tools would still have telemetry enabled that we need to worry about. My understanding is that telemetry is solely enabled by the system we're looking at in this PR.

marcpopMSFT · 2022-06-20T23:24:20Z

For the test failure on the Darwin leg, you can cherry pick this commit or wait for this PR to go in which includes that commit.

src/Common/CompileOptions.cs

richlander · 2022-06-21T02:02:50Z

Directory.Build.props

@@ -21,6 +21,7 @@
    <!-- <ArtifactsShippingSymbolsDir>$(ArtifactsDir)symbols\$(Configuration)\Shipping</ArtifactsShippingSymbolsDir> -->

    <DefineConstants Condition="'$(ContinuousIntegrationBuild)' == 'true'">$(DefineConstants);CI_BUILD</DefineConstants>
+    <DefineConstants Condition="'$(DotNetBuildFromSource)' != 'true'">$(DefineConstants);MICROSOFT_ENABLE_TELEMETRY</DefineConstants>


Per my understanding, with the naive build of the product, MICROSOFT_ENABLE_TELEMETRY will be set. Isn't that not what we want?

How about the follow?

Suggested change

<DefineConstants Condition="'$(DotNetBuildFromSource)' != 'true'">$(DefineConstants);MICROSOFT_ENABLE_TELEMETRY</DefineConstants>

<DefineConstants Condition="'$(MicrosoftOfficialBuild)' == 'true'">$(DefineConstants);MICROSOFT_ENABLE_TELEMETRY</DefineConstants>

What is there works to opt-out of telemetry for open-source builds of .NET.
DotNetBuildFromSource is used throughout the different repositories to detect that.

I'm not familiar with MicrosoftOfficialBuild. It's not used in this repo yet.
I'll apply the suggestion.

MicrosoftOfficialBuild is something I made up. We would just need the official build to set that condition and then everything would be perfect.

Can we do that @marcpopMSFT, for the official SDK build?

That's more of an engineering system question as items like ContinuousIntegrationBuild are set in the scripts that live in the eng\common folder it looks like. @mmitche do you know where MicrosoftOfficialBuild would need to be set so that our official builds would include that? that probably needs to be set before this PR can go in.

It would need to be set in the YAML files in this repo, similar to how we enable other Microsoft specific switches in our build. e.g. https://github.com/dotnet/sdk/blob/main/.vsts-ci.yml#L34-L35

I think this will end up being a common pattern with potentially other organizations having their own logic in shared code. Perhaps we change this to something like OfficialBuilder, setting it to Microsoft for us, or perhaps BuildOrganization?

Thoughts @richlander?

I'm not particularl on the name, but something like that sounds good.

Let's go for OfficialBuilder

We can always change it later if someone has a better name.

It would need to be set in the YAML files in this repo, similar to how we enable other Microsoft specific switches in our build. e.g. https://github.com/dotnet/sdk/blob/main/.vsts-ci.yml#L34-L35

I've added it to this location.

Let's go for OfficialBuilder

My personal preference was to use the existing DotNetBuildFromSource rather than introduce something new.

tmds · 2022-06-28T13:29:12Z

@marcpopMSFT can you review the PR, and give your input here: #25935 (comment)?

tmds · 2022-07-05T12:55:24Z

Is this ready to merge?

.vsts-ci.yml

Directory.Build.props

.vsts-ci.yml

baronfel

Looks good! Needs a rebase to incorporate changes from main in the meantime though.

richlander · 2022-08-08T21:17:52Z

Can we make this PR land this week? That'd would be really excellent for ensuring we make .NET 7.

baronfel · 2022-08-09T15:06:31Z

Merged in the CI fix from main, let's see if we can get to green now.

…by default. We opt-out by setting DOTNET_CLI_TELEMETRY_OPTOUT at 'dotnet' Main to ensure that anything that checks the variable later, including MSBuild Tasks, will see the value.

Co-authored-by: Rich Lander <[email protected]>

Use 'Microsoft' as 'OfficialBuilder' value. Co-authored-by: Jan Kotas <[email protected]>

dotnet-issue-labeler bot added the Area-Infrastructure label Jun 9, 2022

kasperk81 reviewed Jun 10, 2022

View reviewed changes

src/Cli/dotnet/Program.cs Outdated Show resolved Hide resolved

tmds requested a review from vijayrkn as a code owner June 20, 2022 08:42

richlander reviewed Jun 21, 2022

View reviewed changes

src/Common/CompileOptions.cs Outdated Show resolved Hide resolved

richlander reviewed Jun 21, 2022

View reviewed changes

richlander mentioned this pull request Jun 25, 2022

Make call-to-action host errors source-build friendly dotnet/runtime#71287

Open

jkotas reviewed Jul 5, 2022

View reviewed changes

.vsts-ci.yml Outdated Show resolved Hide resolved

tmds commented Jul 5, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

mmitche reviewed Jul 5, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

tmds commented Jul 13, 2022

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

marcpopMSFT reviewed Jul 13, 2022

View reviewed changes

.vsts-ci.yml Show resolved Hide resolved

marcpopMSFT requested a review from baronfel August 8, 2022 19:56

baronfel approved these changes Aug 8, 2022

View reviewed changes

tmds added 2 commits August 9, 2022 17:52

For build-from-source builds of .NET, opt-out of Microsoft telemetry …

b831a98

…by default. We opt-out by setting DOTNET_CLI_TELEMETRY_OPTOUT at 'dotnet' Main to ensure that anything that checks the variable later, including MSBuild Tasks, will see the value.

Use const string also for existing check.

ec48946

tmds and others added 6 commits August 9, 2022 17:53

Refactor to using the default value to control the opt-out.

d9a6074

Apply suggestions from code review

b5db7d6

Co-authored-by: Rich Lander <[email protected]>

Rename to OfficialBuilder, and set value to true in workflow.

afb63ae

Apply suggestions from code review

8126bbf

Use 'Microsoft' as 'OfficialBuilder' value. Co-authored-by: Jan Kotas <[email protected]>

Update Directory.Build.props

6d2a16b

Change Move Official builder into a separate build variable

2cd0f13

vlada-shubina force-pushed the build_from_source_no_telemetry branch from f0fcad7 to 2cd0f13 Compare August 9, 2022 15:57

baronfel enabled auto-merge (rebase) August 9, 2022 16:05

baronfel merged commit a43e07b into dotnet:main Aug 9, 2022

iskiselev mentioned this pull request Aug 16, 2023

Telemetry enhancement: Add "kill switch" for strict corporate environments devlooped/SponsorLink#50

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For build-from-source builds of .NET, opt-out of Microsoft telemetry #25935

For build-from-source builds of .NET, opt-out of Microsoft telemetry #25935

tmds commented Jun 9, 2022

richlander commented Jun 9, 2022

tmds commented Jun 9, 2022 •

edited

Loading

richlander commented Jun 9, 2022

kasperk81 commented Jun 10, 2022

tmds commented Jun 13, 2022

KalleOlaviNiemitalo commented Jun 13, 2022

tmds commented Jun 14, 2022

tmds commented Jun 14, 2022

KalleOlaviNiemitalo commented Jun 14, 2022

richlander commented Jun 14, 2022

marcpopMSFT commented Jun 20, 2022

richlander Jun 21, 2022

tmds Jun 21, 2022

richlander Jun 22, 2022

marcpopMSFT Jun 28, 2022

mmitche Jun 28, 2022

richlander Jun 28, 2022

mmitche Jun 28, 2022

richlander Jun 29, 2022

tmds Jul 5, 2022

tmds commented Jun 28, 2022

tmds commented Jul 5, 2022

baronfel left a comment

richlander commented Aug 8, 2022

baronfel commented Aug 9, 2022

	<DefineConstants Condition="'$(DotNetBuildFromSource)' != 'true'">$(DefineConstants);MICROSOFT_ENABLE_TELEMETRY</DefineConstants>
	<DefineConstants Condition="'$(MicrosoftOfficialBuild)' == 'true'">$(DefineConstants);MICROSOFT_ENABLE_TELEMETRY</DefineConstants>

For build-from-source builds of .NET, opt-out of Microsoft telemetry #25935

For build-from-source builds of .NET, opt-out of Microsoft telemetry #25935

Conversation

tmds commented Jun 9, 2022

richlander commented Jun 9, 2022

tmds commented Jun 9, 2022 • edited Loading

richlander commented Jun 9, 2022

kasperk81 commented Jun 10, 2022

tmds commented Jun 13, 2022

KalleOlaviNiemitalo commented Jun 13, 2022

tmds commented Jun 14, 2022

tmds commented Jun 14, 2022

KalleOlaviNiemitalo commented Jun 14, 2022

richlander commented Jun 14, 2022

marcpopMSFT commented Jun 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmds commented Jun 28, 2022

tmds commented Jul 5, 2022

baronfel left a comment

Choose a reason for hiding this comment

richlander commented Aug 8, 2022

baronfel commented Aug 9, 2022

tmds commented Jun 9, 2022 •

edited

Loading