Refactor allowEmptyTelemetry to use Items and support hashing #30269
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
5 tasks
nagilson
reviewed
Feb 2, 2023
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAAllowEmptyTelemetry.cs
Show resolved
Hide resolved
src/Tasks/Microsoft.NET.Build.Tasks.UnitTests/GivenAAllowEmptyTelemetry.cs
Outdated
Show resolved
Hide resolved
nagilson
approved these changes
Feb 7, 2023
baronfel
added a commit
to dotnet/runtime
that referenced
this pull request
Mar 1, 2023
…he SDK In dotnet/sdk#30269 the API of the AllowEmptyTelemetry task changed to allow for more granular hashing of the collected telemetry properties. This change was against a servicing branch and flowed into SDK main. Sometime after that change, these targets moved from SDK to Runtime, and so the change was lost. This re-applies the change to Runtime main to address breaks seen in source-build.
github-actions bot
pushed a commit
to dotnet/runtime
that referenced
this pull request
Mar 1, 2023
…he SDK In dotnet/sdk#30269 the API of the AllowEmptyTelemetry task changed to allow for more granular hashing of the collected telemetry properties. This change was against a servicing branch and flowed into SDK main. Sometime after that change, these targets moved from SDK to Runtime, and so the change was lost. This re-applies the change to Runtime main to address breaks seen in source-build.
carlossanlop
pushed a commit
to dotnet/runtime
that referenced
this pull request
Mar 11, 2023
…he SDK (#82805) In dotnet/sdk#30269 the API of the AllowEmptyTelemetry task changed to allow for more granular hashing of the collected telemetry properties. This change was against a servicing branch and flowed into SDK main. Sometime after that change, these targets moved from SDK to Runtime, and so the change was lost. This re-applies the change to Runtime main to address breaks seen in source-build. Co-authored-by: Chet Husk <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As part of reviewing #30239, I realized that we could answer some questions about the way devs use PublishProfiles from the CLI if we collected a little more telemetry - specifically the name of the profile that was imported (hashed of course), and if a Profile was imported at all. This would allow us to securely get usage rates for SDK-bundled profiles, as well as get data on how often users specify a Profile but there's not a matching profile (i.e. typos or deleted files, that kind of thing).
The Name of the profile would require hashing to comply with out privacy guidelines, since it could contain sensitive user data. The current AllowEmptyTelemetry implementation didn't really allow for specifying if a piece of data needed to be hashed or not, so we needed a new implementation that would allow for this.
The Task now takes in ITaskITems and reads metadata to determine how each one should be handled -
Includeis the key,Valueis the value, andHashis a boolean that says if theValueshould be hashed in our standard way.Tests were updated to ensure that we didn't regress behavior, I added a new one showing the hashing, and I updated existing callsites as part of this.