Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix OpenTracing shim under legacy AspNetCore activities #3506

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Shims.OpenTracing/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

* Fix: Handling of OpenTracing spans when used in conjunction
with legacy "Microsoft.AspNetCore.Hosting.HttpRequestIn" activities.
([#3509](https://github.com/open-telemetry/opentelemetry-dotnet/pull/3506))

## 1.0.0-rc9.4

Released 2022-Jun-03
Expand Down
19 changes: 1 addition & 18 deletions src/OpenTelemetry.Shims.OpenTracing/SpanBuilderShim.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

using System;
using System.Collections.Generic;
using System.Diagnostics;
using OpenTelemetry.Internal;
using OpenTelemetry.Trace;
using OpenTracing;
Expand Down Expand Up @@ -50,14 +49,6 @@ internal sealed class SpanBuilderShim : ISpanBuilder
/// </summary>
private readonly List<KeyValuePair<string, object>> attributes = new();

/// <summary>
/// The set of operation names for System.Diagnostics.Activity based automatic instrumentations that indicate a root span.
/// </summary>
private readonly IList<string> rootOperationNamesForActivityBasedAutoInstrumentations = new List<string>
{
"Microsoft.AspNetCore.Hosting.HttpRequestIn",
};

/// <summary>
/// The parent as an TelemetrySpan, if any.
/// </summary>
Expand All @@ -79,15 +70,14 @@ internal sealed class SpanBuilderShim : ISpanBuilder

private bool error;

public SpanBuilderShim(Tracer tracer, string spanName, IList<string> rootOperationNamesForActivityBasedAutoInstrumentations = null)
public SpanBuilderShim(Tracer tracer, string spanName)
{
Guard.ThrowIfNull(tracer);
Guard.ThrowIfNull(spanName);

this.tracer = tracer;
this.spanName = spanName;
this.ScopeManager = new ScopeManagerShim(this.tracer);
this.rootOperationNamesForActivityBasedAutoInstrumentations = rootOperationNamesForActivityBasedAutoInstrumentations ?? this.rootOperationNamesForActivityBasedAutoInstrumentations;
}

private IScopeManager ScopeManager { get; }
Expand Down Expand Up @@ -172,13 +162,6 @@ public ISpan Start()
{
span = this.tracer.StartSpan(this.spanName, this.spanKind, this.parentSpanContext, default, this.links, this.explicitStartTime ?? default);
}
else if (this.parentSpan == null && !this.parentSpanContext.IsValid && Activity.Current != null && Activity.Current.IdFormat == ActivityIdFormat.W3C)
{
if (this.rootOperationNamesForActivityBasedAutoInstrumentations.Contains(Activity.Current.OperationName))
{
span = Tracer.CurrentSpan;
}
}

if (span == null)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ public void Start_ActivityOperationRootSpanChecks()

// matching root operation name
var tracer = TracerProvider.Default.GetTracer(TracerName);
var shim = new SpanBuilderShim(tracer, "foo", new List<string> { "foo" });
var shim = new SpanBuilderShim(tracer, "foo");
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
var spanShim1 = (SpanShim)shim.Start();

Assert.Equal("foo", spanShim1.Span.Activity.OperationName);

// mis-matched root operation name
shim = new SpanBuilderShim(tracer, "foo", new List<string> { "bar" });
shim = new SpanBuilderShim(tracer, "foo");
var spanShim2 = (SpanShim)shim.Start();

Assert.Equal("foo", spanShim2.Span.Activity.OperationName);
Expand Down Expand Up @@ -330,5 +330,29 @@ public void Start()
Assert.NotNull(span);
Assert.Equal("foo", span.Span.Activity.OperationName);
}

[Fact]
public void Start_UnderAspNetCoreInstrumentation()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test would have passed before this PR. Is it testing a scenario that was not already covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code fails on the assert at line 350

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also on 355 if check of 350 was removed.

{
// Simulate a span from AspNetCore instrumentation as parent.
using var source = new ActivitySource("Microsoft.AspNetCore.Hosting.HttpRequestIn");
using var parentSpan = source.StartActivity("OTelParent");
Assert.NotNull(parentSpan);

// Start the OpenTracing span.
var tracer = TracerProvider.Default.GetTracer(TracerName);
var builderShim = new SpanBuilderShim(tracer, "foo");
var spanShim = builderShim.StartActive().Span as SpanShim;
Assert.NotNull(spanShim);

var telemetrySpan = spanShim.Span;
Assert.Same(telemetrySpan.Activity, Activity.Current);
Assert.Same(parentSpan, telemetrySpan.Activity.Parent);

// Dispose the spanShim.Span and ensure correct state for Activity.Current
spanShim.Span.Dispose();

Assert.Same(parentSpan, Activity.Current);
}
}
}