Ending an activity will not restore the previously active one for explicit parents

[cijothomas]

Copied from open-telemetry/opentelemetry-dotnet#1677

Symptom

Activity.Current becomes null whenever any Activity with explicit parent is ended. I would instead have expected that the previously active activity would be restored, independent of whether it was the parent or not.

Following shows how to repro this with OpenTelemetry, but this can be reproed by setting up any Activitylistener.

Reproduce

using System;
using System.Diagnostics;
using OpenTelemetry;
using OpenTelemetry.Trace;

public class Program
{
    private static readonly ActivitySource MyActivitySource = new ActivitySource(
        "MyCompany.MyProduct.MyLibrary");

    public static void Main()
    {
        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .SetSampler(new AlwaysOnSampler())
            .AddSource("MyCompany.MyProduct.MyLibrary")
            .Build();

        using (var activity = MyActivitySource.StartActivity("SayHello"))
        {
            Console.WriteLine("Current before: {0}", Activity.Current?.DisplayName);
            using (var child = MyActivitySource.StartActivity("SayWorld", ActivityKind.Internal, activity.Context))
            {
                Console.WriteLine("Current child: {0}", Activity.Current?.DisplayName);
            }

            Console.WriteLine("Current after: {0}", Activity.Current?.DisplayName ?? "(null)");
        }
    }
}

Actual Output:

Current before: SayHello
Current child: SayWorld
Current after: (null)

Expected output would be that the last line is Current after: SayHello.

Additional context

Responsible line in Activity.Stop:

https://github.com/dotnet/runtime/blob/v5.0.1/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L629

I think the Activity should capture a "previous" Activity independently of the Parent Activity and then always set that as active when stopping it.

[ghost]

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Copied from open-telemetry/opentelemetry-dotnet#1677

Symptom

Activity.Current becomes null whenever any Activity with explicit parent is ended. I would instead have expected that the previously active activity would be restored, independent of whether it was the parent or not.

Following shows how to repro this with OpenTelemetry, but this can be reproed by setting up any Activitylistener.

Reproduce

using System;
using System.Diagnostics;
using OpenTelemetry;
using OpenTelemetry.Trace;

public class Program
{
    private static readonly ActivitySource MyActivitySource = new ActivitySource(
        "MyCompany.MyProduct.MyLibrary");

    public static void Main()
    {
        using var tracerProvider = Sdk.CreateTracerProviderBuilder()
            .SetSampler(new AlwaysOnSampler())
            .AddSource("MyCompany.MyProduct.MyLibrary")
            .Build();

        using (var activity = MyActivitySource.StartActivity("SayHello"))
        {
            Console.WriteLine("Current before: {0}", Activity.Current?.DisplayName);
            using (var child = MyActivitySource.StartActivity("SayWorld", ActivityKind.Internal, activity.Context))
            {
                Console.WriteLine("Current child: {0}", Activity.Current?.DisplayName);
            }

            Console.WriteLine("Current after: {0}", Activity.Current?.DisplayName ?? "(null)");
        }
    }
}

Actual Output:

Current before: SayHello
Current child: SayWorld
Current after: (null)

Expected output would be that the last line is Current after: SayHello.

Additional context

Responsible line in Activity.Stop:

https://github.com/dotnet/runtime/blob/v5.0.1/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L629

I think the Activity should capture a "previous" Activity independently of the Parent Activity and then always set that as active when stopping it.

Author:	cijothomas
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[tarekgh]

@cijothomas I am wondering how this suppose to work from the OTel specification point of view? I mean:

• Create a Span and set its customized parent SpanContext. note that current Tracer.Context is different than the parent context we set to the Span.
• Set Tracer.Context to the created span.
• Then dispose the created span
• request the current Tracer.Context.

what value you'll get?

CC @noahfalk

[Oberon00]

The Activity API is not conformant to the spec at all, see https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#span-creation:

Span creation MUST NOT set the newly created Span as the currently active Span by default, but this functionality MAY be offered additionally as a separate operation.

Which the activity API violates. It also says in https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#end:

End MUST NOT inactivate the Span in any Context it is active in. It MUST still be possible to use an ended span as parent via a Context it is contained in. Also, any mechanisms for putting the Span into a Context MUST still work after the Span was ended.

Which the activity API also does not conform to.

As per the spec for End, Tracer.Context would be the "disposed" span, if I understand you correctly.

[Oberon00]

Also note that the spec requires a concept of a "wrapped" SpanContext, i.e. a SpanContext can be wrapped in a non-recording Span so that it can be active in a context: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#wrapping-a-spancontext-in-a-span

The API MUST provide an operation for wrapping a SpanContext with an object implementing the Span interface. This is done in order to expose a SpanContext as a Span in operations such as in-process Span propagation.

[noahfalk]

I'm happy to explore what we can do to improve things, but I do want to be up-front that spec conformance is not the only consideration which drives .NET's design in this space. The Activity API long pre-dated OpenTelemetry's spec and there is a huge ecosystem of .NET apps that our customers depend on us not to break when they upgrade. Re-defining the behavior of existing APIs is always an area we are going to proceed cautiously.

[Oberon00]

I understand. That's why I suggested in open-telemetry/opentelemetry-dotnet#1677 (and copied here) that

I think the Activity should capture a "previous" Activity independently of the Parent Activity and then always set that as active when stopping it.

This may also break any use cases that rely on the activity being null when it is not after that change, but that would be a much smaller change.