Implement OpenTelemetry infrastructure #11255
Conversation
| /// Create a list of properties sent to VS telemetry with the information whether they should be hashed. | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public IList<TelemetryItem> GetActivityProperties() |
There was a problem hiding this comment.
A suggestion, feel free to ignore:
can we logically split the types of collected data? e.g. AddFeatureFlags, AddBuildProperties, AddTimeMetrics, to improve readability and simplify adding new lines?
It looks like this method has a huge potential to grow :)
JanProvaznik
force-pushed
the
otel-basic
branch
from
7c1bae8 to
49467bb
Compare
| telemetryItems.Add(new TelemetryItem("BuildTarget", Target, true)); | ||
| } | ||
|
|
||
| if (Version != null) |
There was a problem hiding this comment.
can Version or other strings be empty?
|
|
||
| #else | ||
| // Create the SHA256 object and compute the hash | ||
| using (var sha256 = SHA256.Create()) |
There was a problem hiding this comment.
please check
msbuild/src/Shared/ConversionUtilities.cs
Line 17 in bfa266d
There was a problem hiding this comment.
The aim here is to do the same thing as sdk https://github.com/dotnet/sdk/blob/8bd19a2390a6bba4aa80d1ac3b6c5385527cc311/src/Cli/Microsoft.DotNet.Cli.Utils/Sha256Hasher.cs not to use MSBuild-specific implementations.
There was a problem hiding this comment.
how do be plan to stay up to date with sdk implementation?
There was a problem hiding this comment.
Context: This code only runs when customers hook their own telemetry in core.
The aim was so the hash matches the one produced by sdk which would only be helpful if internal people collected it and looked at the sdk data table. If sdk changed then it'd need to be changed manually, which is concerning. I don't see how we could depend on their hasher implementation, that would create a circular dependency, no? (And also it's not completely the same, since I made a netstandard2.0 conditionally compiled version based on it which is not in sdk).
An option is to omit implementing this logic and make it a problem of the people hooking their telemetry processor/exporter.
Another option is to make a "MSBuild hasher" that does not aim to be the same as sdk, but why not start with the one in sdk.
After trying to use ConversionUtilities, it would require moving a lot of internal stuff to framework which I don't know if is worth it or if there are any concerning implications.
| /// <summary> | ||
| /// For VS OpenTelemetry Collector to apply the correct privacy policy. | ||
| /// </summary> | ||
| public const string VSMajorVersion = "17.0"; |
There was a problem hiding this comment.
should we track this field somewhere if the docs to stay up to date?
| @@ -13,6 +13,14 @@ | |||
| <add key="dotnet8" value="https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet8/nuget/v3/index.json" /> | |||
| <add key="dotnet8-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json" /> | |||
| <add key="dotnet9" value="https://dnceng.pkgs.visualstudio.com/public/_packaging/dotnet9/nuget/v3/index.json" /> | |||
| <add key="vs-impl" value="https://pkgs.dev.azure.com/azure-public/vside/_packaging/vs-impl/nuget/v3/index.json" /> | |||
There was a problem hiding this comment.
is it a temporary thing?
I recall we had some limitations for using this feed in msbuild repo
There was a problem hiding this comment.
It's intended to be permanent. Under it, there is a restriction that this feed can pull only VS OpenTelemetry packages. (and it won't pull them outside Framework because they're conditionally included in Microsoft.Build.Framework.csproj
There was a problem hiding this comment.
@rainersigwald to draw your attention here
There was a problem hiding this comment.
The PSM configuration doesn't exactly say that: it says that Microsoft.VisualStudio.OpenTelemetry* must come from vs-impl, not that vs-impl can only be used for that package.
We need to be careful here--have you talked to the VMR/sourcebuild folks about the dependency?
There was a problem hiding this comment.
The PSM configuration doesn't exactly say that: it says that Microsoft.VisualStudio.OpenTelemetry* must come from vs-impl, not that vs-impl can only be used for that package.
Thanks, I misunderstood that. Is it possible to do? From a quick look at docs I guess no
We're in contact and it does not impact sourcebuild, because it's references are gated only for .NET Framework in Microsoft.Build.Framework.csproj, and Framework is not a part of sourcebuild.
There is no way to condition on that in NuGet.config though.
JanProvaznik
force-pushed
the
otel-basic
branch
from
62af4f0 to
cdaa52a
Compare
| @@ -453,6 +453,7 @@ private void UpdatePriority(Process p, ProcessPriorityClass priority) | |||
| /// <exception cref="InvalidOperationException">Thrown if a build is already in progress.</exception> | |||
| public void BeginBuild(BuildParameters parameters) | |||
| { | |||
| OpenTelemetryManager.Instance.Initialize(isStandalone: false); | |||
There was a problem hiding this comment.
Is it fine if Initialize is called twice? (in case of standalone msbuild)
There was a problem hiding this comment.
Oh I see it just skips then.
| /// <summary> | ||
| /// For core hook, ActivitySource is created. | ||
| /// </summary> | ||
| TracerInitialized, | ||
|
|
||
| /// <summary> | ||
| /// For VS scenario with a collector. ActivitySource, OTel TracerProvider are created. | ||
| /// </summary> | ||
| ExporterInitialized, | ||
|
|
||
| /// <summary> | ||
| /// For standalone, ActivitySource, OTel TracerProvider, VS OpenTelemetry Collector are created. | ||
| /// </summary> | ||
| CollectorInitialized, |
There was a problem hiding this comment.
How does the instrumentation code check whether a data should be collected? Does it need to check if state is any of those? Or is there otehr single property that can be checked in boolean style?
There was a problem hiding this comment.
No. It's for inner tracking of the state of the instrumentation (perhaps I can make it private).
Instrumented code does something or is a no-op based on the Tracer+Exporter objects.
In the spirit this PR not having 2k LoC it samples everything with one rate, subsequent PR will deal with sample rate differentiation based on namespaces. I figured this is acceptable since this is not a public API so we can easily change/refactor.
src/MSBuild/XMake.cs
Outdated
| @@ -296,6 +298,9 @@ string[] args | |||
| DumpCounters(false /* log to console */); | |||
| } | |||
|
|
|||
| // Send OpenTelemetry before exiting | |||
| OpenTelemetryManager.Instance.Shutdown(); | |||
There was a problem hiding this comment.
We don't seem to call this for the API usage case.
Let's try to see if we can have better place to control lifetime of the telemetry manager (probably BuildManager?)
There was a problem hiding this comment.
Oh that's an oversight given we don't have the collector in VS yet. Now that I look at it, it makes more sense to do everything in EndBuild.
The point is that somewhere the collector has to be shut down, whereas the Exporter is ok to just flush (if collector is somewhere else).
Is it also a goal to send telemetry when people are using MSBuild via API on their own?
Fixes #10945 #10948
Context
#10560
Changes Made
add singleton OpenTelemetryManager
executions in Framework (standalone and managed) trace and collect basic build events with a defined sampling rate.
Sending end of build telemetry comparable to the one sent in sdk.
Testing
Unit tests of the behavior,
monitoring if we get data to the backend
Notes