StartSpan should not generate active span by default

src/OpenTelemetry.Api/CHANGELOG.md

@@ -16,6 +16,11 @@ changed to match the
   gets removed.
     ([#954](https://github.com/open-telemetry/opentelemetry-dotnet/pull/954)).
 * HttpStatusCode in all spans attribute (http.status_code) to use int value.
+* Added `StartRootSpan` and modified `StartSpan` API.
+  ([#989](https://github.com/open-telemetry/opentelemetry-dotnet/issues/989)).
+  Modified span creation not return active span by default to match the
+  [spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-creation).
+  Additionally, introduced `StartActiveSpan` to create an active span.
 
 ## 0.4.0-beta.2
 

src/OpenTelemetry.Api/Trace/Tracer.cs

@@ -54,62 +54,54 @@ public TelemetrySpan CurrentSpan
         }
 
         /// <summary>
-        /// Starts span.
+        /// Starts root span.
         /// </summary>
         /// <param name="name">Span name.</param>
         /// <param name="kind">Kind.</param>
+        /// <param name="attributes">Initial attributes for the span.</param>
+        /// <param name="links"> <see cref="Link"/> for the span.</param>
+        /// <param name="startTime"> Start time for the span.</param>
         /// <returns>Span instance.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public TelemetrySpan StartSpan(string name, SpanKind kind = SpanKind.Internal)
+        public TelemetrySpan StartRootSpan(string name, SpanKind kind = SpanKind.Internal, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
         {
-            // TODO: Open Question - should we have both StartSpan and StartActiveSpan?
-            // Or should we call this method StartActiveSpan?
-            // This method StartSpan is starting a Span and making it Active.
-            // OTel spec calls for StartSpan, and StartActiveSpan being separate.
-            // Need to see if it makes sense for .NET to strictly adhere to it.
-            // Some discussions in Spec: https://github.com/open-telemetry/opentelemetry-specification/pull/485
-            if (!this.ActivitySource.HasListeners())
-            {
-                return TelemetrySpan.NoopInstance;
-            }
-
-            var activityKind = this.ConvertToActivityKind(kind);
-            var activity = this.ActivitySource.StartActivity(name, activityKind);
-            if (activity == null)
-            {
-                return TelemetrySpan.NoopInstance;
-            }
-
-            return new TelemetrySpan(activity);
+            return this.StartSpanHelper(false, name, kind, default, attributes, links, startTime);
         }
 
         /// <summary>
-        /// Starts span.
+        /// Starts a span and does not make it as current span.
         /// </summary>
         /// <param name="name">Span name.</param>
         /// <param name="kind">Kind.</param>
-        /// <param name="parent">Parent for new span.</param>
+        /// <param name="parentSpan">Parent for new span.</param>
+        /// <param name="attributes">Initial attributes for the span.</param>
+        /// <param name="links"> <see cref="Link"/> for the span.</param>
+        /// <param name="startTime"> Start time for the span.</param>
         /// <returns>Span instance.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public TelemetrySpan StartSpan(string name, SpanKind kind, in SpanContext parent)
+        public TelemetrySpan StartSpan(string name, SpanKind kind, in TelemetrySpan parentSpan, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
         {
-            if (!this.ActivitySource.HasListeners())
-            {
-                return TelemetrySpan.NoopInstance;
-            }
-
-            var activityKind = this.ConvertToActivityKind(kind);
-            var activity = this.ActivitySource.StartActivity(name, activityKind, parent.ActivityContext);
-            if (activity == null)
-            {
-                return TelemetrySpan.NoopInstance;
-            }
+            return this.StartSpan(name, kind, parentSpan?.Context ?? default, attributes, links, startTime);
+        }
 
-            return new TelemetrySpan(activity);
+        /// <summary>
+        /// Starts a span and does not make it as current span.
+        /// </summary>
+        /// <param name="name">Span name.</param>
+        /// <param name="kind">Kind.</param>
+        /// <param name="parentContext">Parent Context for new span.</param>
+        /// <param name="attributes">Initial attributes for the span.</param>
+        /// <param name="links"> <see cref="Link"/> for the span.</param>
+        /// <param name="startTime"> Start time for the span.</param>
+        /// <returns>Span instance.</returns>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public TelemetrySpan StartSpan(string name, SpanKind kind = SpanKind.Internal, in SpanContext parentContext = default, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
+        {
+            return this.StartSpanHelper(false, name, kind, parentContext, attributes, links, startTime);
         }
 
         /// <summary>
-        /// Starts span.
+        /// Starts a span and make it the current active span.
         /// </summary>
         /// <param name="name">Span name.</param>
         /// <param name="kind">Kind.</param>
@@ -119,13 +111,13 @@ public TelemetrySpan StartSpan(string name, SpanKind kind, in SpanContext parent
         /// <param name="startTime"> Start time for the span.</param>
         /// <returns>Span instance.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public TelemetrySpan StartSpan(string name, SpanKind kind, in TelemetrySpan parentSpan, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
+        public TelemetrySpan StartActiveSpan(string name, SpanKind kind, in TelemetrySpan parentSpan, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
         {
-            return this.StartSpan(name, kind, parentSpan.Context, attributes, links, startTime);
+            return this.StartActiveSpan(name, kind, parentSpan?.Context ?? default, attributes, links, startTime);
         }
 
         /// <summary>
-        /// Starts span.
+        /// Starts a span and make it the current active span.
         /// </summary>
         /// <param name="name">Span name.</param>
         /// <param name="kind">Kind.</param>
@@ -135,23 +127,38 @@ public TelemetrySpan StartSpan(string name, SpanKind kind, in TelemetrySpan pare
         /// <param name="startTime"> Start time for the span.</param>
         /// <returns>Span instance.</returns>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public TelemetrySpan StartSpan(string name, SpanKind kind, in SpanContext parentContext, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
+        public TelemetrySpan StartActiveSpan(string name, SpanKind kind = SpanKind.Internal, in SpanContext parentContext = default, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
+        {
+            return this.StartSpanHelper(true, name, kind, parentContext, attributes, links, startTime);
+        }
+
+        /// <summary>
+        /// Makes the given span as the current one.
+        /// </summary>
+        /// <param name="span">The span to be made current.</param>
+        /// <returns>The current span.</returns>
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public TelemetrySpan WithSpan(TelemetrySpan span)
+        {
+            span.Activate();
+            return span;
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        private TelemetrySpan StartSpanHelper(bool isActiveSpan, string name, SpanKind kind, in SpanContext parentContext = default, IEnumerable<KeyValuePair<string, object>> attributes = null, IEnumerable<Link> links = null, DateTimeOffset startTime = default)
         {
             if (!this.ActivitySource.HasListeners())
             {
                 return TelemetrySpan.NoopInstance;
             }
 
             var activityKind = this.ConvertToActivityKind(kind);
+            var activityLinks = links?.Select(l => l.ActivityLink);
 
-            IList<ActivityLink> activityLinks = null;
-            if (links != null && links.Count() > 0)
+            Activity previousActivity = null;
+            if (!isActiveSpan)
             {
-                activityLinks = new List<ActivityLink>();
-                foreach (var link in links)
-                {
-                    activityLinks.Add(link.ActivityLink);
-                }
+                previousActivity = Activity.Current;
             }
 
             // TODO:
@@ -164,39 +171,26 @@ public TelemetrySpan StartSpan(string name, SpanKind kind, in SpanContext parent
                 return TelemetrySpan.NoopInstance;
             }
 
-            return new TelemetrySpan(activity);
-        }
+            if (!isActiveSpan)
+            {
+                Activity.Current = previousActivity;
+            }
 
-        /// <summary>
-        /// Makes the given span as the current one.
-        /// </summary>
-        /// <param name="span">The span to be made current.</param>
-        /// <returns>The current span.</returns>
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public TelemetrySpan WithSpan(TelemetrySpan span)
-        {
-            span.Activate();
-            return span;
+            return new TelemetrySpan(activity);
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private ActivityKind ConvertToActivityKind(SpanKind kind)
         {
-            switch (kind)
+            return kind switch
             {
-                case SpanKind.Client:
-                    return ActivityKind.Client;
-                case SpanKind.Consumer:
-                    return ActivityKind.Consumer;
-                case SpanKind.Internal:
-                    return ActivityKind.Internal;
-                case SpanKind.Producer:
-                    return ActivityKind.Producer;
-                case SpanKind.Server:
-                    return ActivityKind.Server;
-                default:
-                    return ActivityKind.Internal;
-            }
+                SpanKind.Client => ActivityKind.Client,
+                SpanKind.Consumer => ActivityKind.Consumer,
+                SpanKind.Internal => ActivityKind.Internal,
+                SpanKind.Producer => ActivityKind.Producer,
+                SpanKind.Server => ActivityKind.Server,
+                _ => ActivityKind.Internal,
+            };
         }
     }
 }

src/OpenTelemetry.Shims.OpenTracing/SpanBuilderShim.cs

@@ -16,6 +16,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics;
 using global::OpenTracing;
 using OpenTelemetry.Trace;
 
@@ -160,7 +161,7 @@ public ISpan Start()
             // If specified, this takes precedence.
             if (this.ignoreActiveSpan)
             {
-                span = this.tracer.StartSpan(this.spanName, this.spanKind, default(SpanContext), null, this.links, this.explicitStartTime ?? default);
+                span = this.tracer.StartRootSpan(this.spanName, this.spanKind, null, this.links, this.explicitStartTime ?? default);
             }
             else if (this.parentSpan != null)
             {
@@ -170,19 +171,12 @@ public ISpan Start()
             {
                 span = this.tracer.StartSpan(this.spanName, this.spanKind, this.parentSpanContext, null, this.links, this.explicitStartTime ?? default);
             }
-            else if (this.parentSpan == null && !this.parentSpanContext.IsValid && (this.tracer.CurrentSpan == null || !this.tracer.CurrentSpan.Context.IsValid))
+            else if (this.parentSpan == null && !this.parentSpanContext.IsValid && Activity.Current != null && Activity.Current.IdFormat == ActivityIdFormat.W3C)
             {
-                // TODO: We need to know if we should inherit an existing Activity-based context or start a new one.
-                /*
-                if (System.Diagnostics.Activity.Current != null && System.Diagnostics.Activity.Current.IdFormat == System.Diagnostics.ActivityIdFormat.W3C)
+                if (this.rootOperationNamesForActivityBasedAutoInstrumentations.Contains(Activity.Current.OperationName))
                 {
-                    var currentActivity = System.Diagnostics.Activity.Current;
-                    if (this.rootOperationNamesForActivityBasedAutoInstrumentations.Contains(currentActivity.OperationName))
-                    {
-                        this.tracer.StartSpanFromActivity(this.spanName, currentActivity, this.spanKind, this.links);
-                        span = this.tracer.CurrentSpan;
-                    }
-                }*/
+                    span = this.tracer.CurrentSpan;
+                }
             }
 
             if (span == null)

test/OpenTelemetry.Shims.OpenTracing.Tests/SpanBuilderShimTests.cs

@@ -121,19 +121,24 @@ public void AsChildOf_WithSpan()
         [Fact]
         public void Start_ActivityOperationRootSpanChecks()
         {
+            // Create an activity
+            var activity = new Activity("foo")
+                .SetIdFormat(System.Diagnostics.ActivityIdFormat.W3C)
+                .Start();
+
             // matching root operation name
             var tracer = TracerProvider.Default.GetTracer(TracerName);
             var shim = new SpanBuilderShim(tracer, "foo", new List<string> { "foo" });
-            var spanShim = (SpanShim)shim.Start();
+            var spanShim1 = (SpanShim)shim.Start();
 
-            Assert.Equal("foo", spanShim.Span.Activity.OperationName);
+            Assert.Equal("foo", spanShim1.Span.Activity.OperationName);
 
             // mis-matched root operation name
             shim = new SpanBuilderShim(tracer, "foo", new List<string> { "bar" });
-            spanShim = (SpanShim)shim.Start();
+            var spanShim2 = (SpanShim)shim.Start();
 
-            Assert.Equal("foo", spanShim.Span.Activity.OperationName);
-            Assert.Equal("foo", spanShim.Span.Activity.Parent.OperationName);
+            Assert.Equal("foo", spanShim2.Span.Activity.OperationName);
+            Assert.Equal(spanShim1.Context.TraceId, spanShim2.Context.TraceId);
         }
 
         [Fact]