Fix StartOperation when user provides custom Ids in scope of unrelate…

…d Activity (#1206)

* Fix StartOperation when user provides custom Ids in scope of unrelated Activity

* fix standalone tests

* add operation handler tests

* fxcop!

* fix tests and changelog

* Fix invalid operation id assigned on the telemtery in StartOperation

* remove comments

CHANGELOG.md

@@ -13,6 +13,7 @@ This changelog will be used to generate documentation on [release notes page](ht
 - [Make BaseSDK use W3C Trace Context based correlation by default. Set TelemetryConfiguration.EnableW3CCorrelation=false to disable this.](https://github.com/microsoft/ApplicationInsights-dotnet/pull/1193)
 - [Removed TelemetryConfiguration.EnableW3CCorrelation. Users should do Activity.DefaultIdFormat = ActivityIdFormat.Hierarchical; Activity.ForceDefaultIdFormat = true; to disable W3C Format](https://github.com/microsoft/ApplicationInsights-dotnet/issues/1198)
 - [Enable sampling based on upstream sampling decision for adaptive sampling](https://github.com/microsoft/ApplicationInsights-dotnet/pull/1200)
+- [Fix: StartOperation ignores user-provided custom Ids in scope of Activity](https://github.com/microsoft/ApplicationInsights-dotnet/pull/1205)
 - [Set tracestate if available on requests and dependencies](https://github.com/microsoft/ApplicationInsights-dotnet/pull/1207)
 
 ## Version 2.11.0-beta1

Test/Microsoft.ApplicationInsights.Test/Shared/Extensibility/Implementation/OperationHolderTests.cs

@@ -1,7 +1,9 @@
 namespace Microsoft.ApplicationInsights.Extensibility.Implementation
 {
     using System;
+    using System.Diagnostics;
     using Microsoft.ApplicationInsights.DataContracts;
+    using Microsoft.ApplicationInsights.TestFramework;
     using Microsoft.VisualStudio.TestTools.UnitTesting;
 
     /// <summary>
@@ -10,6 +12,12 @@
     [TestClass]
     public class OperationHolderTests
     {
+        [TestInitialize]
+        public void Initialize()
+        {
+            ActivityFormatHelper.EnableW3CFormatInActivity();
+        }
+
         /// <summary>
         /// Tests the scenario if OperationItem throws ArgumentNullException with null telemetry client.
         /// </summary>
@@ -38,5 +46,51 @@ public void CreatingOperationItemDoesNotThrowOnPassingValidArguments()
         {
             var operationItem = new OperationHolder<DependencyTelemetry>(new TelemetryClient(TelemetryConfiguration.CreateDefault()), new DependencyTelemetry());
         }
+
+        [TestMethod]
+        public void CreatingOperationHolderWithDetachedOriginalActivityRestoresIt()
+        {
+            var client = new TelemetryClient(TelemetryConfiguration.CreateDefault());
+
+            var originalActivity = new Activity("original").Start();
+            var operation = new OperationHolder<DependencyTelemetry>(client, new DependencyTelemetry(), originalActivity);
+
+            var newActivity = new Activity("new").SetParentId("detached-parent").Start();
+            operation.Telemetry.Id = $"|{newActivity.TraceId.ToHexString()}.{newActivity.SpanId.ToHexString()}.";
+
+            operation.Dispose();
+            Assert.AreEqual(Activity.Current, originalActivity);
+        }
+
+        [TestMethod]
+        public void CreatingOperationHolderWithNullOriginalActivityDoesNotRestoreIt()
+        {
+            var client = new TelemetryClient(TelemetryConfiguration.CreateDefault());
+
+            var originalActivity = new Activity("original").Start();
+            var operation = new OperationHolder<DependencyTelemetry>(client, new DependencyTelemetry(), null);
+
+            var newActivity = new Activity("new").SetParentId("detached-parent").Start();
+            operation.Telemetry.Id = $"|{newActivity.TraceId.ToHexString()}.{newActivity.SpanId.ToHexString()}.";
+
+            operation.Dispose();
+            Assert.IsNull(Activity.Current);
+        }
+
+        [TestMethod]
+        public void CreatingOperationHolderWithParentActivityRestoresIt()
+        {
+            var client = new TelemetryClient(TelemetryConfiguration.CreateDefault());
+
+            var originalActivity = new Activity("original").Start();
+            var operation = new OperationHolder<DependencyTelemetry>(client, new DependencyTelemetry(), originalActivity);
+
+            // child of original
+            var newActivity = new Activity("new").Start();
+            operation.Telemetry.Id = $"|{newActivity.TraceId.ToHexString()}.{newActivity.SpanId.ToHexString()}.";
+            operation.Dispose();
+            Assert.AreEqual(Activity.Current, originalActivity);
+        }
+
     }
 }

Test/Microsoft.ApplicationInsights.Test/Shared/StartOperationActivityTests.cs

@@ -32,6 +32,8 @@ public void TestInitialize()
             configuration.TelemetryInitializers.Add(new OperationCorrelationTelemetryInitializer());
             this.telemetryClient = new TelemetryClient(configuration);
             CallContextHelpers.RestoreOperationContext(null);
+
+            ActivityFormatHelper.EnableW3CFormatInActivity();
         }
 
         [TestCleanup]
@@ -62,6 +64,63 @@ public void BasicStartOperationWithActivity()
             Assert.AreEqual(telemetry, this.sendItems.Single());
         }
 
+        [TestMethod]
+        public void BasicStartOperationWithActivityInScopeOfUnrelatedActivity()
+        {
+            var outerActivity = new Activity("foo").Start();
+
+            var activity = new Activity("name").SetParentId("parentId").AddBaggage("b1", "v1").AddTag("t1", "v1");
+
+            RequestTelemetry telemetry;
+            using (var operation = this.telemetryClient.StartOperation<RequestTelemetry>(activity))
+            {
+                telemetry = operation.Telemetry;
+                Assert.AreEqual(activity, Activity.Current);
+                Assert.AreNotEqual(outerActivity, Activity.Current.Parent);
+                Assert.IsNotNull(activity.Id);
+            }
+
+            this.ValidateTelemetry(telemetry, activity);
+
+            Assert.AreEqual(telemetry, this.sendItems.Single());
+            Assert.AreEqual(outerActivity, Activity.Current);
+
+            var request = this.sendItems.Single() as RequestTelemetry;
+            Assert.IsNotNull(request);
+            Assert.AreEqual(activity.TraceId.ToHexString(), request.Context.Operation.Id);
+            Assert.AreEqual($"|{activity.TraceId.ToHexString()}.{activity.SpanId.ToHexString()}.", request.Id);
+            Assert.AreEqual("parentId", request.Context.Operation.ParentId);
+        }
+
+        [TestMethod]
+        public void BasicStartOperationWithStartedActivityInScopeOfUnrelatedActivity()
+        {
+            var outerActivity = new Activity("foo").Start();
+
+            // this is not right to give started Activity to StartOperation, but nothing terrible should happen
+            // except it won't be possible to restore original context after StartOperation completes
+            var activity = new Activity("name").SetParentId("parentId").AddBaggage("b1", "v1").AddTag("t1", "v1").Start();
+
+            RequestTelemetry telemetry;
+            using (var operation = this.telemetryClient.StartOperation<RequestTelemetry>(activity))
+            {
+                telemetry = operation.Telemetry;
+                Assert.AreEqual(activity, Activity.Current);
+                Assert.AreNotEqual(outerActivity, Activity.Current.Parent);
+                Assert.IsNotNull(activity.Id);
+            }
+
+            this.ValidateTelemetry(telemetry, activity);
+
+            Assert.AreEqual(telemetry, this.sendItems.Single());
+            Assert.IsNull(Activity.Current);
+
+            var request = this.sendItems.Single() as RequestTelemetry;
+            Assert.IsNotNull(request);
+            Assert.AreEqual(activity.TraceId.ToHexString(), request.Context.Operation.Id);
+            Assert.AreEqual($"|{activity.TraceId.ToHexString()}.{activity.SpanId.ToHexString()}.", request.Id);
+            Assert.AreEqual("parentId", request.Context.Operation.ParentId);
+        }
 
         /// <summary>
         /// Invalid Usage! Tests that if Activity is started, StartOperation still works and does not crash.