Fixed telemetry data sharing exception (#3676)

src/Microsoft.TestPlatform.CrossPlatEngine/Discovery/DiscoveryManager.cs

@@ -45,7 +45,7 @@ public class DiscoveryManager : IDiscoveryManager
     /// <summary>
     /// Initializes a new instance of the <see cref="DiscoveryManager"/> class.
     /// </summary>
-    public DiscoveryManager(IRequestData requestData)
+    public DiscoveryManager(IRequestData requestData!!)
         : this(requestData, TestPlatformEventSource.Instance)
     {
     }
@@ -59,7 +59,7 @@ public DiscoveryManager(IRequestData requestData)
     /// <param name="testPlatformEventSource">
     ///     The test platform event source.
     /// </param>
-    protected DiscoveryManager(IRequestData requestData, ITestPlatformEventSource testPlatformEventSource)
+    protected DiscoveryManager(IRequestData requestData!!, ITestPlatformEventSource testPlatformEventSource)
     {
         _sessionMessageLogger = TestSessionMessageLogger.Instance;
         _sessionMessageLogger.TestRunMessage += TestSessionMessageHandler;
@@ -73,6 +73,9 @@ protected DiscoveryManager(IRequestData requestData, ITestPlatformEventSource te
     /// <param name="pathToAdditionalExtensions"> The path to additional extensions. </param>
     public void Initialize(IEnumerable<string> pathToAdditionalExtensions, ITestDiscoveryEventsHandler2 eventHandler)
     {
+        // Clear the request data metrics left over from a potential previous run.
+        _requestData.MetricsCollection?.Metrics?.Clear();
+
         _testPlatformEventSource.AdapterSearchStart();
         _testDiscoveryEventsHandler = eventHandler;
         if (pathToAdditionalExtensions != null && pathToAdditionalExtensions.Any())

src/Microsoft.TestPlatform.CrossPlatEngine/Execution/ExecutionManager.cs

@@ -40,7 +40,7 @@ public class ExecutionManager : IExecutionManager
     /// <summary>
     /// Initializes a new instance of the <see cref="ExecutionManager"/> class.
     /// </summary>
-    public ExecutionManager(IRequestData requestData) : this(TestPlatformEventSource.Instance, requestData)
+    public ExecutionManager(IRequestData requestData!!) : this(TestPlatformEventSource.Instance, requestData)
     {
         _sessionMessageLogger = TestSessionMessageLogger.Instance;
         _sessionMessageLogger.TestRunMessage += TestSessionMessageHandler;
@@ -50,7 +50,7 @@ public ExecutionManager(IRequestData requestData) : this(TestPlatformEventSource
     /// Initializes a new instance of the <see cref="ExecutionManager"/> class.
     /// </summary>
     /// <param name="testPlatformEventSource">Test platform event source.</param>
-    protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRequestData requestData)
+    protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRequestData requestData!!)
     {
         _testPlatformEventSource = testPlatformEventSource;
         _requestData = requestData;
@@ -64,6 +64,9 @@ protected ExecutionManager(ITestPlatformEventSource testPlatformEventSource, IRe
     /// <param name="pathToAdditionalExtensions"> The path to additional extensions. </param>
     public void Initialize(IEnumerable<string> pathToAdditionalExtensions, ITestMessageEventHandler testMessageEventsHandler)
     {
+        // Clear the request data metrics left over from a potential previous run.
+        _requestData.MetricsCollection?.Metrics?.Clear();
+
         _testMessageEventsHandler = testMessageEventsHandler;
         _testPlatformEventSource.AdapterSearchStart();
 

src/Microsoft.TestPlatform.CrossPlatEngine/PublicAPI/PublicAPI.Shipped.txt

@@ -98,7 +98,7 @@ Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestExtensionManager.UseAddi
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.GetDiscoveryManager() -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol.IDiscoveryManager
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.GetExecutionManager() -> Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol.IExecutionManager
-Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.TestHostManagerFactory(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData requestData) -> void
+Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestHostManagerFactory.TestHostManagerFactory(bool telemetryOptedIn) -> void
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool
 override Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManagerWithDataCollection.Initialize(bool skipDefaultAdapters) -> void
 override Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManagerWithDataCollection.SetupChannel(System.Collections.Generic.IEnumerable<string> sources, string runSettings, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.ITestMessageEventHandler eventHandler) -> bool
@@ -116,7 +116,7 @@ virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionMana
 virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionManager.EnqueueProxy(int proxyId) -> bool
 virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.AddSession(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.ProxyTestSessionManager proxyManager) -> bool
 virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.ReturnProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, int proxyId) -> bool
-virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.TryTakeProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, string source, string runSettings) -> Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager
+virtual Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.TestSessionPool.TryTakeProxy(Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.TestSessionInfo testSessionInfo, string source, string runSettings, Microsoft.VisualStudio.TestPlatform.ObjectModel.Client.IRequestData requestData) -> Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult.Attachments.get -> System.Collections.ObjectModel.Collection<Microsoft.VisualStudio.TestPlatform.ObjectModel.AttachmentSet>
 Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.DataCollection.DataCollectionResult.DataCollectionResult(System.Collections.ObjectModel.Collection<Microsoft.VisualStudio.TestPlatform.ObjectModel.AttachmentSet> attachments, System.Collections.ObjectModel.Collection<Microsoft.VisualStudio.TestPlatform.ObjectModel.InvokedDataCollector> invokedDataCollectors) -> void

src/Microsoft.TestPlatform.CrossPlatEngine/TestEngine.cs

@@ -100,9 +100,9 @@ public IProxyDiscoveryManager GetDiscoveryManager(
             var testHostManagerInfo = testHostManagers[0];
             testHostManager.Initialize(TestSessionMessageLogger.Instance, testHostManagerInfo.RunSettings);
 
-            var isTelemetryOptedIn = requestData.IsTelemetryOptedIn;
-            var newRequestData = GetRequestData(isTelemetryOptedIn);
-            return new InProcessProxyDiscoveryManager(testHostManager, new TestHostManagerFactory(newRequestData));
+            return new InProcessProxyDiscoveryManager(
+                testHostManager,
+                new TestHostManagerFactory(requestData.IsTelemetryOptedIn));
         }
 
         // Create one data aggregator per parallel discovery and share it with all the proxy discovery managers.
@@ -131,7 +131,8 @@ public IProxyDiscoveryManager GetDiscoveryManager(
                     var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy(
                         discoveryCriteria.TestSessionInfo,
                         source,
-                        runtimeProviderInfo.RunSettings);
+                        runtimeProviderInfo.RunSettings,
+                        requestData);
 
                     if (proxyOperationManager == null)
                     {
@@ -215,11 +216,9 @@ public IProxyExecutionManager GetExecutionManager(
                 testHostManager.SetCustomLauncher(testRunCriteria.TestHostLauncher);
             }
 
-            var isTelemetryOptedIn = requestData.IsTelemetryOptedIn;
-            var newRequestData = GetRequestData(isTelemetryOptedIn);
             return new InProcessProxyExecutionManager(
                 testHostManager,
-                new TestHostManagerFactory(newRequestData));
+                new TestHostManagerFactory(requestData.IsTelemetryOptedIn));
         }
 
         // This creates a single non-parallel execution manager, based requestData, isDataCollectorEnabled and the
@@ -271,7 +270,8 @@ internal IProxyExecutionManager CreateNonParallelExecutionManager(IRequestData r
                     var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy(
                         testRunCriteria.TestSessionInfo,
                         source,
-                        runtimeProviderInfo.RunSettings);
+                        runtimeProviderInfo.RunSettings,
+                        requestData);
 
                     if (proxyOperationManager == null)
                     {
@@ -569,24 +569,6 @@ private bool ShouldRunInProcess(
         return false;
     }
 
-    /// <summary>
-    /// Get request data on basis of telemetry opted in or not.
-    /// </summary>
-    ///
-    /// <param name="isTelemetryOptedIn">A flag indicating if telemetry is opted in.</param>
-    ///
-    /// <returns>The request data.</returns>
-    private IRequestData GetRequestData(bool isTelemetryOptedIn)
-    {
-        return new RequestData
-        {
-            MetricsCollection = isTelemetryOptedIn
-                ? (IMetricsCollection)new MetricsCollection()
-                : new NoOpMetricsCollection(),
-            IsTelemetryOptedIn = isTelemetryOptedIn
-        };
-    }
-
     private static void ThrowExceptionIfTestHostManagerIsNull(ITestRuntimeProvider testHostManager, string settingsXml)
     {
         if (testHostManager == null)

src/Microsoft.TestPlatform.CrossPlatEngine/TestHostManagerFactory.cs

@@ -1,12 +1,10 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using Microsoft.VisualStudio.TestPlatform.Common;
+using Microsoft.VisualStudio.TestPlatform.Common.Telemetry;
 using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Discovery;
-
 using Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Execution;
-
-using Microsoft.VisualStudio.TestPlatform.ObjectModel.Client;
-
 using Microsoft.VisualStudio.TestPlatform.ObjectModel.Engine.TesthostProtocol;
 
 #nullable disable
@@ -20,30 +18,43 @@ public class TestHostManagerFactory : ITestHostManagerFactory
 {
     private IDiscoveryManager _discoveryManager;
     private IExecutionManager _executionManager;
-    private readonly IRequestData _requestData;
+    private readonly bool _telemetryOptedIn;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="TestHostManagerFactory"/> class.
     /// </summary>
-    /// <param name="requestData">
-    /// Provide common services and data for a discovery/run request.
+    /// 
+    /// <param name="telemetryOptedIn">
+    /// A value indicating if the telemetry is opted in or not.
     /// </param>
-    public TestHostManagerFactory(IRequestData requestData!!)
+    public TestHostManagerFactory(bool telemetryOptedIn)
     {
-        _requestData = requestData;
+        _telemetryOptedIn = telemetryOptedIn;
     }
 
     /// <summary>
     /// The discovery manager instance for any discovery related operations inside of the test host.
     /// </summary>
     /// <returns>The discovery manager.</returns>
     public IDiscoveryManager GetDiscoveryManager()
-        => _discoveryManager ??= new DiscoveryManager(_requestData);
+        => _discoveryManager ??= new DiscoveryManager(GetRequestData(_telemetryOptedIn));
 
     /// <summary>
     /// The execution manager instance for any discovery related operations inside of the test host.
     /// </summary>
     /// <returns>The execution manager.</returns>
     public IExecutionManager GetExecutionManager()
-        => _executionManager ??= new ExecutionManager(_requestData);
+        => _executionManager ??= new ExecutionManager(GetRequestData(_telemetryOptedIn));
+
+    private RequestData GetRequestData(bool telemetryOptedIn)
+    {
+        return new RequestData
+        {
+            MetricsCollection =
+                telemetryOptedIn
+                    ? new MetricsCollection()
+                    : new NoOpMetricsCollection(),
+            IsTelemetryOptedIn = telemetryOptedIn
+        };
+    }
 }

src/Microsoft.TestPlatform.CrossPlatEngine/TestSession/TestSessionPool.cs

@@ -125,12 +125,14 @@ public virtual bool KillSession(TestSessionInfo testSessionInfo, IRequestData re
     /// <param name="testSessionInfo">The test session info object.</param>
     /// <param name="source">The source to be associated to this proxy.</param>
     /// <param name="runSettings">The run settings.</param>
+    /// <param name="requestData">The request data.</param>
     /// 
     /// <returns>The proxy object.</returns>
     public virtual ProxyOperationManager TryTakeProxy(
         TestSessionInfo testSessionInfo,
         string source,
-        string runSettings)
+        string runSettings,
+        IRequestData requestData!!)
     {
         ProxyTestSessionManager sessionManager = null;
         lock (_lockObject)
@@ -147,7 +149,15 @@ public virtual ProxyOperationManager TryTakeProxy(
         try
         {
             // Deque an actual proxy to do work.
-            return sessionManager.DequeueProxy(source, runSettings);
+            var proxy = sessionManager.DequeueProxy(source, runSettings);
+
+            // Make sure we use the per-request request data instead of the request data used when
+            // creating the test session. Otherwise we can end up having irrelevant telemetry for
+            // the current request being fulfilled or even duplicate telemetry which may cause an
+            // exception to be thrown.
+            proxy.RequestData = requestData;
+
+            return proxy;
         }
         catch (InvalidOperationException ex)
         {

src/testhost.x86/DefaultEngineInvoker.cs

@@ -9,8 +9,6 @@
 using System.Threading;
 using System.Threading.Tasks;
 
-using Microsoft.VisualStudio.TestPlatform.Common;
-using Microsoft.VisualStudio.TestPlatform.Common.Telemetry;
 using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities;
 using Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.Interfaces;
 using Microsoft.VisualStudio.TestPlatform.CoreUtilities.Helpers;
@@ -135,13 +133,13 @@ public void Invoke(IDictionary<string, string?> argsDictionary)
             ConnectToDatacollector(dcPort);
         }
 
-        var requestData = GetRequestData(argsDictionary);
+        var telemetryOptedIn = GetTelemetryStatusFromArgs(argsDictionary);
 
         // Start processing async in a different task
         EqtTrace.Info("DefaultEngineInvoker.Invoke: Start Request Processing.");
         try
         {
-            StartProcessingAsync(_requestHandler, new TestHostManagerFactory(requestData)).Wait();
+            StartProcessingAsync(_requestHandler, new TestHostManagerFactory(telemetryOptedIn)).Wait();
         }
         finally
         {
@@ -155,29 +153,14 @@ public void Invoke(IDictionary<string, string?> argsDictionary)
         }
     }
 
-    private static RequestData GetRequestData(IDictionary<string, string?> argsDictionary)
+    private static bool GetTelemetryStatusFromArgs(IDictionary<string, string?> argsDictionary)
     {
-        // Checks for Telemetry Opted in or not from Command line Arguments.
-        // By Default opting out in Test Host to handle scenario when user running old version of vstest.console
+        // Checks if telemetry is opted in in the command line arguments.
+        // By default opting out in testhost to handle the scenario when the user is running an old
+        // version of vstest.console.
         var telemetryStatus = CommandLineArgumentsHelper.GetStringArgFromDict(argsDictionary, TelemetryOptedIn);
-        var telemetryOptedIn = false;
-        if (!string.IsNullOrWhiteSpace(telemetryStatus))
-        {
-            if (telemetryStatus.Equals("true", StringComparison.Ordinal))
-            {
-                telemetryOptedIn = true;
-            }
-        }
 
-        var requestData = new RequestData
-        {
-            MetricsCollection =
-                telemetryOptedIn
-                    ? new MetricsCollection()
-                    : new NoOpMetricsCollection(),
-            IsTelemetryOptedIn = telemetryOptedIn
-        };
-        return requestData;
+        return string.Equals(telemetryStatus, "true", StringComparison.Ordinal);
     }
 
     private void ConnectToDatacollector(int dcPort)

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyDiscoveryManagerTests.cs

@@ -529,7 +529,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                 var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy(
                     testSessionInfo,
                     source,
-                    _discoveryCriteria.RunSettings);
+                    _discoveryCriteria.RunSettings,
+                    _mockRequestData.Object);
 
                 return proxyOperationManager;
             };
@@ -551,7 +552,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                     tsp => tsp.TryTakeProxy(
                         testSessionInfo,
                         It.IsAny<string>(),
-                        It.IsAny<string>()))
+                        It.IsAny<string>(),
+                        _mockRequestData.Object))
                 .Returns(mockProxyOperationManager.Object);
 
             testDiscoveryManager.Initialize(true);
@@ -563,7 +565,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                 tsp => tsp.TryTakeProxy(
                     testSessionInfo,
                     It.IsAny<string>(),
-                    It.IsAny<string>()),
+                    It.IsAny<string>(),
+                    _mockRequestData.Object),
                 Times.Once);
         }
         finally

test/Microsoft.TestPlatform.CrossPlatEngine.UnitTests/Client/ProxyExecutionManagerTests.cs

@@ -742,7 +742,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                 var proxyOperationManager = TestSessionPool.Instance.TryTakeProxy(
                     testSessionInfo,
                     source,
-                    string.Empty);
+                    string.Empty,
+                    _mockRequestData.Object);
 
                 return proxyOperationManager;
             };
@@ -765,7 +766,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                     tsp => tsp.TryTakeProxy(
                         testSessionInfo,
                         It.IsAny<string>(),
-                        It.IsAny<string>()))
+                        It.IsAny<string>(),
+                        _mockRequestData.Object))
                 .Returns(mockProxyOperationManager.Object);
 
             testExecutionManager.Initialize(true);
@@ -777,7 +779,8 @@ public void StartTestRunShouldAttemptToTakeProxyFromPoolIfProxyIsNull()
                 tsp => tsp.TryTakeProxy(
                     testSessionInfo,
                     It.IsAny<string>(),
-                    It.IsAny<string>()),
+                    It.IsAny<string>(),
+                    _mockRequestData.Object),
                 Times.Once);
         }
         finally