fix logger message importance and other cleanup

Author: YuliiaKovalova

eng/Signing.props

@@ -11,6 +11,9 @@
     <ItemsToSign Include="$(ArtifactsDir)\xsd\Update-MSBuildXsds.ps1" />
 
     <FileSignInfo Include="RuntimeContracts.dll" CertificateName="3PartySHA2" />
+
+    <!-- For Telemetry in VS.-->
+    <FileSignInfo Include="Newtonsoft.Json.dll" CertificateName="3PartySHA2" />
   </ItemGroup>
 
   <!-- Remove existing .nupkg signing info and set to None for testing to workaround the existing issue in arcade. -->

src/Build.UnitTests/BackEnd/BuildManager_Tests.cs

@@ -1801,7 +1801,6 @@ public void OverlappingBuildsOfTheSameProjectDifferentTargetsAreAllowed()
  </Target>
 </Project>
 ");
-
             Project project = CreateProject(contents, MSBuildDefaultToolsVersion, _projectCollection, true);
             ProjectInstance instance = _buildManager.GetProjectInstanceForBuild(project);
             _buildManager.BeginBuild(_parameters);

src/Build.UnitTests/Telemetry/Telemetry_Tests.cs

@@ -154,7 +154,6 @@ public void WorkerNodeTelemetryCollection_CustomTargetsAndTasks()
         public void NodeTelemetryE2E()
         {
             using TestEnvironment env = TestEnvironment.Create();
-            env.SetEnvironmentVariable("MSBUILD_TELEMETRY_OPTIN", "1");
             env.SetEnvironmentVariable("MSBUILD_TELEMETRY_OPTOUT", null);
             env.SetEnvironmentVariable("DOTNET_CLI_TELEMETRY_OPTOUT", null);
 
@@ -245,7 +244,7 @@ public void NodeTelemetryE2E()
             tags["VS.MSBuild.BuildTarget"].ShouldNotBeNullOrEmpty();
 
             // Verify task data
-            var tasks = activity.TagObjects.FirstOrDefault(to => to.Key.Contains("VS.MSBuild.Tasks"));
+            var tasks = activity.TagObjects.FirstOrDefault(to => to.Key == "VS.MSBuild.Tasks");
 
             var tasksData = tasks.Value as List<TaskDetailInfo>;
             var messageTaskData = tasksData!.FirstOrDefault(t => t.Name == "Microsoft.Build.Tasks.Message");

src/Build/BackEnd/BuildManager/BuildManager.cs

@@ -13,11 +13,7 @@
 using System.IO;
 using System.Linq;
 using System.Reflection;
-
-#if FEATURE_REPORTFILEACCESSES
 using System.Runtime.CompilerServices;
-#endif
-
 using System.Runtime.ExceptionServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -30,13 +26,13 @@
 using Microsoft.Build.Exceptions;
 using Microsoft.Build.Experimental.BuildCheck;
 using Microsoft.Build.Experimental.BuildCheck.Infrastructure;
-using Microsoft.Build.ProjectCache;
 using Microsoft.Build.FileAccesses;
 using Microsoft.Build.Framework;
 using Microsoft.Build.Framework.Telemetry;
 using Microsoft.Build.Graph;
 using Microsoft.Build.Internal;
 using Microsoft.Build.Logging;
+using Microsoft.Build.ProjectCache;
 using Microsoft.Build.Shared;
 using Microsoft.Build.Shared.Debugging;
 using Microsoft.Build.Shared.FileSystem;
@@ -299,6 +295,7 @@ public BuildManager()
         public BuildManager(string hostName)
         {
             ErrorUtilities.VerifyThrowArgumentNull(hostName);
+
             _hostName = hostName;
             _buildManagerState = BuildManagerState.Idle;
             _buildSubmissions = new Dictionary<int, BuildSubmissionBase>();
@@ -463,6 +460,11 @@ private void UpdatePriority(Process p, ProcessPriorityClass priority)
         /// <exception cref="InvalidOperationException">Thrown if a build is already in progress.</exception>
         public void BeginBuild(BuildParameters parameters)
         {
+#if NETFRAMEWORK
+            // Collect telemetry unless explicitly opted out via environment variable.
+            // The decision to send telemetry is made at EndBuild to avoid eager loading of telemetry assemblies.
+            parameters.IsTelemetryEnabled |= !TelemetryManager.IsOptOut();
+#endif
             if (_previousLowPriority != null)
             {
                 if (parameters.LowPriority != _previousLowPriority)
@@ -1103,6 +1105,7 @@ public void EndBuild()
                             {
                                 host = "VSCode";
                             }
+
                             _buildTelemetry.BuildEngineHost = host;
 
                             _buildTelemetry.BuildCheckEnabled = _buildParameters!.IsBuildCheckEnabled;
@@ -1112,6 +1115,7 @@ public void EndBuild()
                             _buildTelemetry.SACEnabled = sacState == NativeMethodsShared.SAC_State.Evaluation || sacState == NativeMethodsShared.SAC_State.Enforcement;
 
                             loggingService.LogTelemetry(buildEventContext: null, _buildTelemetry.EventName, _buildTelemetry.GetProperties());
+
                             EndBuildTelemetry();
 
                             // Clean telemetry to make it ready for next build submission.
@@ -1156,9 +1160,13 @@ void SerializeCaches()
             }
         }
 
+        [MethodImpl(MethodImplOptions.NoInlining)]
         private void EndBuildTelemetry()
         {
-            using IActivity? activity = TelemetryManager.Instance?.DefaultActivitySource
+            TelemetryManager.Instance.Initialize(isStandalone: false);
+
+            using IActivity? activity = TelemetryManager.Instance
+                ?.DefaultActivitySource
                 ?.StartActivity(TelemetryConstants.Build)
                 ?.SetTags(_buildTelemetry)
                 ?.SetTags(_telemetryConsumingLogger?.WorkerNodeTelemetryData.AsActivityDataHolder(
@@ -3005,10 +3013,7 @@ private ILoggingService CreateLoggingService(
                 forwardingLoggers = forwardingLoggers?.Concat(forwardingLogger) ?? forwardingLogger;
             }
 
-            TelemetryManager.Instance.Initialize(isStandalone: false, isExplicitlyRequested: _buildParameters.IsTelemetryEnabled);
-
-            // The telemetry is enabled - we need to add our consuming logger
-            if (TelemetryManager.Instance.DefaultActivitySource != null)
+            if (_buildParameters.IsTelemetryEnabled)
             {
                 // We do want to dictate our own forwarding logger (otherwise CentralForwardingLogger with minimum transferred importance MessageImportance.Low is used)
                 // In the future we might optimize for single, in-node build scenario - where forwarding logger is not needed (but it's just quick pass-through)

src/Build/BackEnd/Components/Logging/LoggingService.cs

@@ -1848,6 +1848,11 @@ private void UpdateMinimumMessageImportance(ILogger logger)
                 // The null logger has no effect on minimum verbosity.
                 Execution.BuildManager.NullLogger => null,
 
+                // Telemetry loggers only consume WorkerNodeTelemetryLogged events, not message events.
+                // They have no effect on minimum message verbosity.
+                TelemetryInfra.InternalTelemetryConsumingLogger => null,
+                Framework.Telemetry.InternalTelemetryForwardingLogger => null,
+
                 TerminalLogger terminalLogger => terminalLogger.GetMinimumMessageImportance(),
                 _ =>
                     innerLogger.GetType().FullName == "Microsoft.Build.Logging.TerminalLogger"

src/Build/BackEnd/Components/RequestBuilder/RequestBuilder.cs

@@ -1267,9 +1267,9 @@ private void UpdateStatisticsPostBuild()
         {
             ITelemetryForwarder telemetryForwarder =
                 ((TelemetryForwarderProvider)_componentHost.GetComponent(BuildComponentType.TelemetryForwarder))
-                .Instance;
+                ?.Instance;
 
-            if (!telemetryForwarder.IsTelemetryCollected)
+            if (telemetryForwarder == null || !telemetryForwarder.IsTelemetryCollected)
             {
                 return;
             }
@@ -1279,6 +1279,11 @@ private void UpdateStatisticsPostBuild()
             // Hence we need to fetch the original result from the cache - to get the data for all executed targets.
             BuildResult unfilteredResult = resultsCache.GetResultsForConfiguration(_requestEntry.Request.ConfigurationId);
 
+            if (unfilteredResult?.ResultsByTarget == null || _requestEntry.RequestConfiguration.Project?.Targets == null)
+            {
+                return;
+            }
+
             foreach (var projectTargetInstance in _requestEntry.RequestConfiguration.Project.Targets)
             {
                 bool wasExecuted =

src/Framework/Telemetry/TelemetryDataUtils.cs

@@ -31,8 +31,8 @@ internal static class TelemetryDataUtils
             tasksSummary.Process(telemetryData.TasksExecutionData);
 
             var buildInsights = new BuildInsights(
-                GetTasksDetails(telemetryData.TasksExecutionData),
-                GetTargetsDetails(telemetryData.TargetsExecutionData),
+                includeTasksDetails ? GetTasksDetails(telemetryData.TasksExecutionData) : [],
+                includeTargetDetails ? GetTargetsDetails(telemetryData.TargetsExecutionData) : [],
                 GetTargetsSummary(targetsSummary),
                 GetTasksSummary(tasksSummary));
 
@@ -128,8 +128,6 @@ public static string Hash(string text)
                 }
 #endif
             }
-
-            public static string HashWithNormalizedCasing(string text) => Hash(text.ToUpperInvariant());
         }
 
         internal record TaskDetailInfo(string Name, double TotalMilliseconds, int ExecutionsCount, long TotalMemoryBytes, bool IsCustom, bool IsNuget);
@@ -230,7 +228,7 @@ private TargetInfo GetTargetInfo(TaskOrTargetTelemetryKey key, bool isExecuted)
                     (true, true) => ExecutedCustomTargetInfo,
                     (true, false) => LoadedCustomTargetInfo,
                     (false, true) => ExecutedBuiltinTargetInfo,
-                    (false, false) => LoadedBuiltinTargetInfo
+                    (false, false) => LoadedBuiltinTargetInfo,
                 };
 
             internal class TargetInfo
@@ -294,12 +292,20 @@ Dictionary<string, object> IActivityTelemetryDataHolder.GetActivityProperties()
             {
                 Dictionary<string, object> properties = new()
                 {
-                    [nameof(BuildInsights.Tasks)] = insights.Tasks,
-                    [nameof(BuildInsights.Targets)] = insights.Targets,
                     [nameof(BuildInsights.TargetsSummary)] = insights.TargetsSummary,
                     [nameof(BuildInsights.TasksSummary)] = insights.TasksSummary,
                 };
 
+                if (insights.Targets.Count > 0)
+                {
+                    properties[nameof(BuildInsights.Targets)] = insights.Targets;
+                }
+
+                if (insights.Tasks.Count > 0)
+                {
+                    properties[nameof(BuildInsights.Tasks)] = insights.Tasks;
+                }
+
                 return properties;
             }
         }

src/Framework/Telemetry/TelemetryManager.cs

@@ -25,7 +25,7 @@ internal class TelemetryManager
         /// <summary>
         /// Lock object for thread-safe initialization and disposal.
         /// </summary>
-        private static readonly object s_lock = new object();
+        private static readonly LockType s_lock = new();
 
         private static bool s_initialized;
         private static bool s_disposed;
@@ -49,11 +49,8 @@ private TelemetryManager()
         /// versus integrated mode (e.g., running within Visual Studio or dotnet CLI).
         /// When <c>true</c>, creates and manages its own telemetry session on .NET Framework.
         /// </param>
-        /// <param name="isExplicitlyRequested">
-        /// Indicates whether telemetry was explicitly requested through command line arguments.
-        /// On .NET, telemetry is only enabled when this is <c>true</c>.
-        /// </param>
-        public void Initialize(bool isStandalone, bool isExplicitlyRequested)
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        public void Initialize(bool isStandalone)
         {
             lock (s_lock)
             {
@@ -69,7 +66,7 @@ public void Initialize(bool isStandalone, bool isExplicitlyRequested)
                     return;
                 }
 
-                TryInitializeTelemetry(isStandalone, isExplicitlyRequested);
+                TryInitializeTelemetry(isStandalone);
             }
         }
 
@@ -93,16 +90,14 @@ internal static void ResetForTest()
         /// allowing the calling code to catch assembly loading exceptions.
         /// </summary>
         [MethodImpl(MethodImplOptions.NoInlining)]
-        private void TryInitializeTelemetry(bool isStandalone, bool isExplicitlyRequested)
+        private void TryInitializeTelemetry(bool isStandalone)
         {
             try
             {
 #if NETFRAMEWORK
                 DefaultActivitySource = VsTelemetryInitializer.Initialize(isStandalone);
 #else
-                DefaultActivitySource = isExplicitlyRequested
-                    ? new MSBuildActivitySource(TelemetryConstants.DefaultActivitySourceNamespace)
-                    : null;
+                DefaultActivitySource = new MSBuildActivitySource(TelemetryConstants.DefaultActivitySourceNamespace);
 #endif
             }
             catch (Exception ex) when (ex is FileNotFoundException or FileLoadException or TypeLoadException)
@@ -139,66 +134,58 @@ FileLoadException or
             }
         }
 
-#if NETFRAMEWORK
-        [MethodImpl(MethodImplOptions.NoInlining)]
-        private static void DisposeVsTelemetry() => VsTelemetryInitializer.Dispose();
-#endif
-
         /// <summary>
         /// Determines if the user has explicitly opted out of telemetry.
         /// </summary>
-        private static bool IsOptOut() =>
+        internal static bool IsOptOut() =>
 #if NETFRAMEWORK
             Traits.Instance.FrameworkTelemetryOptOut;
 #else
             Traits.Instance.SdkTelemetryOptOut;
 #endif
+
+#if NETFRAMEWORK
+        [MethodImpl(MethodImplOptions.NoInlining)]
+        private static void DisposeVsTelemetry() => VsTelemetryInitializer.Dispose();
+#endif
     }
 
 #if NETFRAMEWORK
-    /// <summary>
-    /// Isolated class that references Microsoft.VisualStudio.Telemetry types.
-    /// This separation ensures the VS Telemetry assembly is only loaded when methods
-    /// on this class are actually invoked.
-    /// </summary>
-    /// <remarks>
-    /// Thread-safety: All public methods on this class must be called under the
-    /// <see cref="TelemetryManager"/> lock to ensure thread-safe access to static state.
-    /// Callers must not invoke <see cref="Initialize"/> or <see cref="Dispose"/> concurrently.
-    /// </remarks>
     internal static class VsTelemetryInitializer
     {
         // Telemetry API key for Visual Studio telemetry service.
         private const string CollectorApiKey = "f3e86b4023cc43f0be495508d51f588a-f70d0e59-0fb0-4473-9f19-b4024cc340be-7296";
 
-        private static TelemetrySession? s_telemetrySession;
-        private static bool s_ownsSession;
+        // Store as object to avoid type reference at class load time
+        private static object? s_telemetrySession;
+        private static bool s_ownsSession = false;
 
+        [MethodImpl(MethodImplOptions.NoInlining)]
         public static MSBuildActivitySource Initialize(bool isStandalone)
         {
+            TelemetrySession session;
             if (isStandalone)
             {
-                s_telemetrySession = TelemetryService.CreateAndGetDefaultSession(CollectorApiKey);
+                session = TelemetryService.CreateAndGetDefaultSession(CollectorApiKey);
                 TelemetryService.DefaultSession.UseVsIsOptedIn();
                 TelemetryService.DefaultSession.Start();
                 s_ownsSession = true;
             }
             else
             {
-                s_telemetrySession = TelemetryService.DefaultSession;
-                s_ownsSession = false;
+                session = TelemetryService.DefaultSession;
             }
 
-            return new MSBuildActivitySource(s_telemetrySession);
+            s_telemetrySession = session;
+            return new MSBuildActivitySource(session);
         }
 
+        [MethodImpl(MethodImplOptions.NoInlining)]
         public static void Dispose()
         {
-            // Only dispose the session if we created it (standalone scenario).
-            // In VS, the session is owned by VS and should not be disposed by MSBuild.
-            if (s_ownsSession)
+            if (s_ownsSession && s_telemetrySession is TelemetrySession session)
             {
-                s_telemetrySession?.Dispose();
+                session.Dispose();
             }
 
             s_telemetrySession = null;

src/MSBuild/XMake.cs

@@ -250,7 +250,7 @@ string[] args
             // Initialize new build telemetry and record start of this build.
             KnownTelemetry.PartialBuildTelemetry = new BuildTelemetry { StartAt = DateTime.UtcNow, IsStandaloneExecution = true };
 
-            TelemetryManager.Instance?.Initialize(isStandalone: true, isExplicitlyRequested: false);
+            TelemetryManager.Instance?.Initialize(isStandalone: true);
 
             using PerformanceLogEventListener eventListener = PerformanceLogEventListener.Create();
 

src/MSBuild/app.amd64.config

@@ -106,6 +106,10 @@
           <bindingRedirect oldVersion="0.0.0.0-9.0.0.11" newVersion="9.0.0.11" />
           <codeBase version="9.0.0.11" href="..\System.Collections.Immutable.dll"/>
         </dependentAssembly>
+        <dependentAssembly>
+          <assemblyIdentity name="Newtonsoft.Json" publicKeyToken="30ad4fe6b2a6aeed" culture="neutral" />
+          <codeBase version="13.0.0.0" href="..\Newtonsoft.Json.dll" />
+        </dependentAssembly>
         <dependentAssembly>
           <assemblyIdentity name="System.Memory" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
           <bindingRedirect oldVersion="0.0.0.0-4.0.2.0" newVersion="4.0.2.0" />