-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Emmit BuildTelemetry event #7778
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.
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
Emmit BuildTelemetry event #7778
Conversation
Forgind
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to seeing what we can learn from this!
| bool serverIsAlreadyRunning = ServerNamedMutex.WasOpen(serverRunningMutexName); | ||
| if (KnownTelemetry.BuildTelemetry != null) | ||
| { | ||
| KnownTelemetry.BuildTelemetry.InitialServerState = serverIsAlreadyRunning ? "hot" : "cold"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer it this way. For telemetry data I like the simplicity of adding new value and transparent passing to telemetry storage.
| if (KnownTelemetry.BuildTelemetry != null) | ||
| { | ||
| KnownTelemetry.BuildTelemetry.Project ??= requestData.ProjectFullPath; | ||
| KnownTelemetry.BuildTelemetry.Target ??= string.Join(",", requestData.TargetNames); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My general worry with allocating a lot of strings here for telemetry is that although it isn't a huge deal because it's a cold path, it worsens perf for users without helping them in any way, just helping us help them. Am I wrong about that? Are these telemetry events helpful to users in some way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no direct value for users. They could subscribe to our telemetry events in their own logger and somehow process it, but this scenarios is farfetched.
However, this is only one telemetry allocation per whole build and its affect on overall performance will be negligible.
|
|
||
| if (KnownTelemetry.BuildTelemetry != null) | ||
| { | ||
| KnownTelemetry.BuildTelemetry.Project ??= requestData.ProjectGraphEntryPoints?.FirstOrDefault().ProjectFile; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there be multiple graph entry points? If so, is this right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There can be, however, this is probably very rare situation and we are fine to have it slightly wrong.
'BuildTelemetry.Project' is designed to be used for events correlations it will correlate build telemetry even when it include just one entry point.
Context
For purpose of msbuild server and other we need to be able to collect and analyze build engine usage.
Changes Made
Data are collected at both msbuild client and msbuild server and then emitted when build is finished by BuildManager.
Testing
Unit tests.
Manual debug tests.
Notes