Skip to content

Commit

Permalink
StartSpan should not generate active span by default (#994)
Browse files Browse the repository at this point in the history 
* Modified StartSpan, added RootSpan

* Update changelog

* Fix changelog

* Added test

* Resolve conflict

* Update changelog

* update changelog

Co-authored-by: Cijo Thomas <cithomas@microsoft.com>
  • Loading branch information
@rajkumar-rangaraj @cijothomas
rajkumar-rangaraj and cijothomas authored Aug 5, 2020
1 parent 67607b7 commit 7030026
Show file tree
Hide file tree
Showing 5 changed files with 267 additions and 92 deletions.
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ changed to match the
to be implemented (`IsInjected`)
* Added `CompositePropagator` that accepts a list of `ITextFormat` following
[specification](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md#create-a-composite-propagator)
* Added `StartRootSpan`, `StartActiveSpan` and modified `StartSpan` API.
([#989](https://github.com/open-telemetry/opentelemetry-dotnet/issues/989)).
Modified StartSpan not to set the created span as Active to match
[spec](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-creation).

## 0.4.0-beta.2

Expand Down
142 changes: 68 additions & 74 deletions src/OpenTelemetry.Api/Trace/Tracer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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>
Expand All @@ -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.Any())
Activity previousActivity = null;
if (!isActiveSpan)
{
activityLinks = new List<ActivityLink>();
foreach (var link in links)
{
activityLinks.Add(link.ActivityLink);
}
previousActivity = Activity.Current;
}

// TODO:
Expand All @@ -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,
};
}
}
}
18 changes: 6 additions & 12 deletions src/OpenTelemetry.Shims.OpenTracing/SpanBuilderShim.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using global::OpenTracing;
using OpenTelemetry.Trace;

Expand Down Expand Up @@ -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)
{
Expand All @@ -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)
Expand Down
15 changes: 10 additions & 5 deletions test/OpenTelemetry.Shims.OpenTracing.Tests/SpanBuilderShimTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
Loading

0 comments on commit 7030026

Please sign in to comment.