Collecting some build data for tasks/targets telemetry#11359
Merged
JanKrivanek merged 28 commits intomainfrom Feb 20, 2025
Merged
Collecting some build data for tasks/targets telemetry#11359JanKrivanek merged 28 commits intomainfrom
JanKrivanek merged 28 commits intomainfrom
Conversation
…/dotnet/msbuild into proto/task-telemetry-data-plan-b
Contributor
There was a problem hiding this comment.
Copilot reviewed 19 out of 34 changed files in this pull request and generated 5 comments.
Files not reviewed (15)
- src/Build/Microsoft.Build.csproj: Language not supported
- src/Build.UnitTests/BackEnd/NodePackets_Tests.cs: Evaluated as low risk
- src/Build/BackEnd/Components/BuildComponentFactoryCollection.cs: Evaluated as low risk
- src/Build/Definition/Toolset.cs: Evaluated as low risk
- src/Build/BackEnd/Components/RequestBuilder/TaskBuilder.cs: Evaluated as low risk
- src/Build/BackEnd/Components/Logging/CentralForwardingLogger.cs: Evaluated as low risk
- src/Build/BackEnd/Components/RequestBuilder/TargetBuilder.cs: Evaluated as low risk
- src/Build/BackEnd/Components/IBuildComponentHost.cs: Evaluated as low risk
- src/Framework.UnitTests/FileClassifier_Tests.cs: Evaluated as low risk
- src/Build/BackEnd/BuildManager/BuildParameters.cs: Evaluated as low risk
- src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs: Evaluated as low risk
- src/Build/BackEnd/Shared/TargetResult.cs: Evaluated as low risk
- src/Build/BackEnd/Components/Logging/EventSourceSink.cs: Evaluated as low risk
- src/Build/Instance/TaskFactoryWrapper.cs: Evaluated as low risk
- src/Build/BackEnd/BuildManager/BuildManager.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs:1263
- The new telemetry collection logic in 'UpdateStatisticsPostBuild' is not covered by tests. Add test cases to ensure this logic is tested.
private void UpdateStatisticsPostBuild()
src/Build/Instance/TaskRegistry.cs:1182
- The 'Stats' class should be instantiated with 'new Stats()' instead of 'new Stats'.
internal class Stats()
surayya-MS
reviewed
Feb 4, 2025
Member
surayya-MS
left a comment
There was a problem hiding this comment.
On the first round of review it looks good! I'll look more into this
JanProvaznik
approved these changes
Feb 7, 2025
Member
JanProvaznik
left a comment
There was a problem hiding this comment.
I left a few suggestions if you agree with the naming. All in all: ![]()
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs
Outdated
Show resolved
Hide resolved
src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jan Provazník <janprovaznik@microsoft.com>
…/dotnet/msbuild into proto/task-telemetry-data-plan-b
This was referenced Feb 21, 2025
2 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #10946
If split into separate PRs is prefered - please indicate so in comments
Goal
Obtaining information about build composition from microsoft versus 3rd party tasks and targets.
Approach
Since the information is present in the building worker nodes - the code collects it there and then (if requested) send it via
WorkerNodeTelemetryEventArgsto the main node.The classification of 3rd party versus 1st party is for simplicity being done based on location of defining msbuild project and naming of the assembly.
Changes
Performance considerations
By default the data collection logic is off and hence statistics are not collected on worker node, nor serialized to the event args. The perf impact of collection and serialization was though beyond the recognition level of basic 'full duration' testing of full and incremental build of small console and bigger size projects.
Sample of collected data
This is stringified flushed version of stats collected and aggregated from nodes.
Once inputing into our telemetry client, we might send just subset (topN).
We collect the info about whether the task/target is 1st or 3rd party ('C:' prefix), whether it's comming from nuget cache ('N:' prefix) or is generated within metaproj, the task/target name (will be hashed for 3rd party), the duration of the task, the memory consumption of the task and the number of executions of the task
Performance impact considerations
Somethings always costs more than nothing :-) Beyond that - those changes should be with hardly observable impact.
If telemetry is opted out - there is no extra data collection, processing and sending applied.
When telemetry is opted in - we keep and update hashtables with stats for all tasks and targets in all nodes and at the end of build send those stats to the main node. This minimizes costly traffic during the build.
When measured with 'wall clock time' on incremental build of OrchardCore via:
And excluding warmup and slowest and fastest runs - the versions seems indistinguishable:
main (@aff5455):
Minutes : 2
Seconds : 34
Milliseconds : 352
Minutes : 2
Seconds : 30
Milliseconds : 665
Minutes : 2
Seconds : 30
Milliseconds : 109
This PR (with _isTelemetryEnabled forced to true and
$env:MSBUILDOUTPUTNODESTELEMETRY=1):Minutes : 3
Seconds : 36
Milliseconds : 707
Minutes : 2
Seconds : 37
Milliseconds : 763
Minutes : 2
Seconds : 35
Milliseconds : 794