review comments

4 files changed, 94 insertions, 40 deletions

diff --git a/src/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj b/src/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
index 655a17a8e8..7697d55182 100644
--- a/src/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
+++ b/src/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj
@@ -52,7 +52,7 @@
     <Reference Include="System.Diagnostics.Tracing" />
     <Reference Include="System.Reflection" />
     <Reference Include="System.Runtime" />
-    <Reference Include="System.Runtime.Extensions" />    
+    <Reference Include="System.Runtime.Extensions" />
     <Reference Include="System.Threading" />
   </ItemGroup>
   <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
diff --git a/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs b/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
index 33f008263e..55a8c583d3 100644
--- a/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
+++ b/src/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs
@@ -66,15 +66,13 @@ namespace System.Diagnostics
         {
             get
             {
-                if (ParentId != null)
-                    yield return new KeyValuePair<string, string>("ParentId", ParentId);
                 for (var tags = _tags; tags != null; tags = tags.Next)
                     yield return tags.keyValue;
             }
         }
 
         /// <summary>
-        /// Tags are string-string key-value pairs that represent information that will
+        /// Baggage is string-string key-value pairs that represent information that will
         /// be passed along to children of this activity.   Baggage is serialized 
         /// when requests leave the process (along with the ID).   Typically Baggage is
         /// used to do fine-grained control over logging of the activty and any children.  
@@ -112,6 +110,8 @@ namespace System.Diagnostics
         /// <param name="operationName">Operations name <see cref="OperationName"/></param>
         public Activity(string operationName)
         {
+            if (string.IsNullOrEmpty(operationName))
+                throw new ArgumentException($"{nameof(operationName)} must not be null or empty");
             OperationName = operationName;
         }
 
@@ -151,6 +151,17 @@ namespace System.Diagnostics
         /// </summary>
         public Activity SetParentId(string parentId)
         {
+            if (Parent != null)
+                throw new InvalidOperationException(
+                    $"Trying to set {nameof(ParentId)} on activity which has {nameof(Parent)}");
+
+            if (ParentId != null)
+                throw new InvalidOperationException(
+                    $"{nameof(ParentId)} is already set");
+
+            if (string.IsNullOrEmpty(parentId))
+                throw new ArgumentException($"{nameof(parentId)} must not be null or empty");
+
             ParentId = parentId;
             return this;
         }
@@ -162,6 +173,8 @@ namespace System.Diagnostics
         /// <returns>'this' for convinient chaining</returns>
         public Activity SetStartTime(DateTime startTimeUtc)
         {
+            if (startTimeUtc.Kind != DateTimeKind.Utc)
+                throw new InvalidOperationException($"{nameof(startTimeUtc)} is not UTC");
             StartTimeUtc = startTimeUtc;
             return this;
         }
@@ -174,6 +187,9 @@ namespace System.Diagnostics
         /// <returns>'this' for convinient chaining</returns>
         public Activity SetEndTime(DateTime endTimeUtc)
         {
+            if (endTimeUtc.Kind != DateTimeKind.Utc)
+                throw new InvalidOperationException($"{nameof(endTimeUtc)} is not UTC");
+
             Duration = endTimeUtc - StartTimeUtc;
             return this;
         }
@@ -249,39 +265,19 @@ namespace System.Diagnostics
             {
                 // Normal start within the process
                 Debug.Assert(!string.IsNullOrEmpty(Parent.Id));
-#if DEBUG
-                ret = Parent.Id + "/" + OperationName + "_" + Interlocked.Increment(ref Parent._currentChildId);
-#else           // To keep things short, we drop the operation name 
-                ret = Parent.Id + "/" + Interlocked.Increment(ref Parent._currentChildId);
-#endif
+                ret = $"{Parent.Id}.{Interlocked.Increment(ref Parent._currentChildId)}";
             }
             else if (ParentId != null)
             {
                 // Start from outside the process (e.g. incoming HTTP)
                 Debug.Assert(ParentId.Length != 0);
-#if DEBUG
-                ret = ParentId + "/" + OperationName + "_I_" + Interlocked.Increment(ref s_currentRootId);
-#else           // To keep things short, we drop the operation name 
-                ret = ParentId + "/I_" + Interlocked.Increment(ref s_currentRootId);
-#endif
+                ret = $"{ParentId}.{Interlocked.Increment(ref s_currentRootId)}";
             }
             else
             {
-                // A Root Activity (no parent).  
-                if (s_uniqPrefix == null)
-                {
-                    // Here we make an ID to represent the Process/AppDomain.   Ideally we use process ID but 
-                    // it is unclear if we have that ID handy.   Currently we use low bits of high freq tick 
-                    // as a unique random number (which is not bad, but loses randomness for startup scenarios).
-                    int uniqNum = (int)Stopwatch.GetTimestamp();
-                    string uniqPrefix = $"//{uniqNum:x}_";
-                    Interlocked.CompareExchange(ref s_uniqPrefix, uniqPrefix, null);
-                }
-#if DEBUG
-                ret = s_uniqPrefix + OperationName + "_" + Interlocked.Increment(ref s_currentRootId);
-#else           // To keep things short, we drop the operation name 
-                ret = s_uniqPrefix + Interlocked.Increment(ref s_currentRootId);
-#endif
+                byte[] bytes = new byte[8];
+                s_random.Value.NextBytes(bytes);
+                ret = $"{BitConverter.ToUInt64(bytes, 0)}";
             }
             // Useful place to place a conditional breakpoint.  
             return ret;
@@ -290,8 +286,7 @@ namespace System.Diagnostics
         // Used to generate an ID 
         long _currentChildId;            // A unique number for all children of this activity.  
         static long s_currentRootId;      // A unique number inside the appdomain.
-        static string s_uniqPrefix;      // A unique prefix that represents the machine/process/appdomain
-
+        private static readonly Lazy<Random> s_random = new Lazy<Random>();
         /// <summary>
         /// Having our own key-value linked list allows us to be more efficient  
         /// </summary>
diff --git a/src/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs b/src/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs
index ae740cee97..0f6a2c95a1 100644
--- a/src/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs
+++ b/src/System.Diagnostics.DiagnosticSource/tests/ActivityTests.cs
@@ -1,5 +1,6 @@
 using System.Collections.Generic;
 using System.Linq;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace System.Diagnostics.Tests
@@ -53,10 +54,25 @@ namespace System.Diagnostics.Tests
             Assert.Equal(1, tags.Count);
             Assert.Equal(tags[0].Key, "some tag");
             Assert.Equal(tags[0].Value, "value");
+        }
+
+        /// <summary>
+        /// Tests activity SetParentId
+        /// </summary>
+        [Fact]
+        public void SetParentId()
+        {
+            var parent = new Activity("parent");
+            Assert.Throws<ArgumentException>(() => parent.SetParentId(null));
+            Assert.Throws<ArgumentException>(() => parent.SetParentId(""));
+            parent.SetParentId("1");
+            Assert.Equal("1", parent.ParentId);
+            Assert.Throws<InvalidOperationException>(() => parent.SetParentId("2"));
 
-            Assert.Equal(activity, activity.SetParentId("1"));
-            Assert.Equal(2, activity.Tags.ToList().Count);
-            Assert.Equal("1", activity.ParentId);
+            parent.Start();
+            var child = new Activity("child");
+            child.Start();
+            Assert.Throws<InvalidOperationException>(() => child.SetParentId("3"));
         }
 
         /// <summary>
@@ -79,18 +95,60 @@ namespace System.Diagnostics.Tests
         }
 
         /// <summary>
+        /// Tests Id generation
+        /// </summary>
+        [Fact]
+        public void IdGenerationInternalParent()
+        {
+            var parent = new Activity("parent");
+            parent.Start();
+            var child1 = new Activity("child1");
+            var child2 = new Activity("child2");
+            //start 2 children in different execution contexts
+            var t1 = Task.Run(() => child1.Start());
+            var t2 = Task.Run(() => child2.Start());
+            Task.WhenAll(t1, t2).Wait();
+            Assert.Equal(parent.Id + ".1", child1.Id);
+            Assert.Equal(parent.Id + ".2", child2.Id);
+            child1.Stop();
+            child2.Stop();
+            var child3 = new Activity("child3");
+            child3.Start();
+            Assert.Equal(parent.Id + ".3", child3.Id);
+        }
+
+        /// <summary>
+        /// Tests Id generation
+        /// </summary>
+        [Fact]
+        public void IdGenerationExternalParentId()
+        {
+            var child1 = new Activity("child1");
+            child1.SetParentId("123");
+            child1.Start();
+            child1.Stop();
+            var child2 = new Activity("child2");
+            child2.SetParentId("123");
+            child2.Start();
+            Assert.NotEqual(child2.Id, child1.Id);
+        }
+
+        /// <summary>
         /// Tests Activity Start and Stop with timestamp
         /// </summary>
         [Fact]
         public void StartStopWithTimestamp()
         {
+            var activity = new Activity("activity");
+            Assert.Throws<InvalidOperationException>(() => activity.SetStartTime(DateTime.Now));
+
             var startTime = DateTime.UtcNow.AddSeconds(-1);
-            var activity = new Activity("activity")
-                .SetStartTime(startTime);
+            activity.SetStartTime(startTime);
 
             activity.Start();
             Assert.Equal(startTime, activity.StartTimeUtc);
 
+            Assert.Throws<InvalidOperationException>(() => activity.SetEndTime(DateTime.Now));
             var stopTime = DateTime.UtcNow;
             activity.SetEndTime(stopTime);
             Assert.Equal(stopTime - startTime, activity.Duration);
@@ -129,6 +187,7 @@ namespace System.Diagnostics.Tests
                 .AddTag("tag1", "tag from parent");
 
             parent.Start();
+
             Assert.Equal(parent, Activity.Current);
 
             var child = new Activity("child");
@@ -141,8 +200,7 @@ namespace System.Diagnostics.Tests
 
             //no tags from parent
             var childTags = child.Tags.ToList();
-            Assert.Equal(1, childTags.Count);
-            Assert.Equal(child.ParentId, childTags[0].Value);
+            Assert.Equal(0, childTags.Count);
 
             child.Stop();
             Assert.Equal(parent, Activity.Current);
@@ -172,7 +230,7 @@ namespace System.Diagnostics.Tests
         [Fact]
         public void StartTwice()
         {
-            var activity = new Activity("");
+            var activity = new Activity("activity");
             activity.Start();
             Assert.Throws<InvalidOperationException>(() => activity.Start());
         }
@@ -183,7 +241,7 @@ namespace System.Diagnostics.Tests
         [Fact]
         public void StopNotStarted()
         {
-            Assert.Throws<InvalidOperationException>(() => new Activity("").Stop());
+            Assert.Throws<InvalidOperationException>(() => new Activity("activity").Stop());
         }
 
         /// <summary>
diff --git a/src/System.Diagnostics.DiagnosticSource/tests/Configurations.props b/src/System.Diagnostics.DiagnosticSource/tests/Configurations.props
index c6602850c6..e239022946 100644
--- a/src/System.Diagnostics.DiagnosticSource/tests/Configurations.props
+++ b/src/System.Diagnostics.DiagnosticSource/tests/Configurations.props
@@ -5,6 +5,7 @@
       net45-Windows_NT;
       netstandard;
       netstandard1.1;
+      netstandard1.3;
     </BuildConfigurations>
   </PropertyGroup>
 </Project>
\ No newline at end of file