[API Proposal]: Api handle Activity.Current value changes

[pjanotti]

Background and motivation

[Edit 04/07/2022: Use the event as proposed by @noahfalk]
[Edit 04/07/2022: Updates per conversation with @tarekgh ]

A typical implementation of distributed tracing uses an AsyncLocal<T> to track the "span context" of managed threads. When changes to the span context needs to be tracked this is done by using the AsyncLocal<T> constructor that takes the valueChangedHandler parameter. However, with Activity becoming the standard to represent spans, as used by OpenTelemetry, it is impossible to set the value changed handler since the context is tracked via Activity.Current.

cc @tarekgh @open-telemetry/dotnet-instrumentation-approvers

API Proposal

namespace System.Diagnostics
{
    public readonly struct ActivityNotification
    {
        public Activity? Previous { get; }
        public Activity? Current { get; }
    }

    public partial class Activity : IDisposable
    {
        // Add an event to track updates to the current Activity
        public static event EventHandler<ActivityNotification>? CurrentChanged;
    }
}

API Usage

AsyncLocal<Activity> t_currentActvity = new AsyncLocal<Activity>(ValueChanged);

void Init()
{
    Activity.CurrentChanged += OnCurrentActivityChange;
}

void OnCurrentActivityChange(object sender, ActivityNotification notification)
{
    t_currentActvity = notification.Current;
}

void ValueChanged(AsyncLocalValueChangedArgs<Activity> args)
{
    // Profiler can do whatever it wants with the Activity here. This callback is invoked every time the Activity.Current is set. 
}

Alternative Designs

Using registration methods instead of an Event:

namespace System.Diagnostics
{
    public delegate void ActivityCurrentChangeHandler(Activity? previous, Activity? current);

    public partial class Activity : IDisposable
    {
        public static void RegisterActivityCurrentChangeNotification(ActivityCurrentChangeHandler handler) { }
        public static void CancelActivityCurrentChangeNotification(ActivityCurrentChangeHandler handler) { }
    }
}

Using an Event but using an action that receives only the new current Activity:

namespace System.Diagnostics
{
    public partial class Activity
    {
        public static event Action<Activity?, Activity?>? ActivityChanged;
    }
}

• Considered the AsyncLocal<Activity> backing Activity.Current to always be created with a valueChangedHandler and exposing a static method on Activity that would allow users to add their own handlers if desired.

• Implement the support to add value changed handlers directly in AsyncLocal<T> this way any other type of context that needs to be shared can leverage the same implementation and expose similar APIs if desired. However, any implementation still needs to expose an API to add handlers.

Risks

Potentially Activity.Current can change very frequently, especially in a server application, so the implementation should be "pay-for-play" as much as possible.

[ghost]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

A typical implementation of distributed tracing uses an AsyncLocal<T> to track the "span context" of managed threads. When changes to the span context needs to be tracked this is done by using the AsyncLocal<T> constructor that takes the valueChangedHandler parameter. However, with Activity becoming the standard to represent spans, as used by OpenTelemetry, it is impossible to set the value changed handler since the context is tracked via Activity.Current.

cc @tarekgh @open-telemetry/dotnet-instrumentation-approvers

API Proposal

namespace System.Diagnostics
{
    public class Activity
    {
        /// <summary>
        /// Adds a handler that will be called every time that the Activity.Current changes in the current context.
        /// </summary>
        public static void AddCurrentValueChangedHandler(Action<System.Threading.AsyncLocalValueChangedArgs<Activity>>? valueChangedHandler);
    }
}

API Usage

// Add a handler to track updates to the current Activity
Activity.AddCurrentValueChangedHandler((AsyncLocalValueChangedArgs<Activity> args) =>
        {
            UpdateProfilerContext(args.CurrentValue);
        }));

Alternative Designs

Implement the support to add value changed handlers directly in AsyncLocal<T> this way any other type of context that needs to be shared can leverage the same implementation and expose similar APIs if desired. However, any implementation still needs to expose an API to add handlers.

Risks

Potentially Activity.Current can change very frequently, especially in a server application, so the implementation should be "pay-for-play" as much as possible.

Author:	pjanotti
Assignees:	-
Labels:	api-suggestion, area-System.Threading, untriaged
Milestone:	-

[ghost]

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

A typical implementation of distributed tracing uses an AsyncLocal<T> to track the "span context" of managed threads. When changes to the span context needs to be tracked this is done by using the AsyncLocal<T> constructor that takes the valueChangedHandler parameter. However, with Activity becoming the standard to represent spans, as used by OpenTelemetry, it is impossible to set the value changed handler since the context is tracked via Activity.Current.

cc @tarekgh @open-telemetry/dotnet-instrumentation-approvers

API Proposal

namespace System.Diagnostics
{
    public class Activity
    {
        /// <summary>
        /// Adds a handler that will be called every time that the Activity.Current changes in the current context.
        /// </summary>
        public static void AddCurrentValueChangedHandler(Action<System.Threading.AsyncLocalValueChangedArgs<Activity>>? valueChangedHandler);
    }
}

API Usage

// Add a handler to track updates to the current Activity
Activity.AddCurrentValueChangedHandler((AsyncLocalValueChangedArgs<Activity> args) =>
        {
            UpdateProfilerContext(args.CurrentValue);
        }));

Alternative Designs

Implement the support to add value changed handlers directly in AsyncLocal<T> this way any other type of context that needs to be shared can leverage the same implementation and expose similar APIs if desired. However, any implementation still needs to expose an API to add handlers.

Risks

Potentially Activity.Current can change very frequently, especially in a server application, so the implementation should be "pay-for-play" as much as possible.

Author:	pjanotti
Assignees:	-
Labels:	api-suggestion, untriaged, area-System.Diagnostics.Activity
Milestone:	-

[tarekgh]

@pjanotti thanks for the proposal and nice to hear from you :-)

I have some questions here I hope you can help clarify as this will affect the design of such API. I am seeing you designed the API around the fact of internally using asynclocal. This will be a concern as we'll need to enable the asynclocal value change notification when instantiating it which may have some perf concern. I didn't measure that, but I'll do it in the first chance. So, my questions are:

• Does your scenario need the notification when the thread context changes with asynclocal or you need the notification only when the Activity.Current explicitly set?
• I assume we allow multiple registration for such notification, right?

CC @noahfalk @reyang @cijothomas

[pjanotti]

Hi @tarekgh 👋

Does your scenario need the notification when the thread context changes with asynclocal or you need the notification only when the Activity.Current explicitly set?

We used it to track thread context changes. In our specific usage we used that to do a P/Invoke back to a CLR profiler so we could know to which span/Activity associate a given managed Thread. That said I think there would be also situations that would be useful to do it even from managed code for tracking purposes.

I assume we allow multiple registration for such notification, right?

Right, I think that is desirable and avoids having one implementation and then having to add composition support later.

I am seeing you designed the API around the fact of internally using asynclocal.

Correct, of course this does not need to be the implementation. I wanted to started with something more concrete to capture what capability needs to be satisfied. Having that capability and completely hiding whatever is backing it will be ideal.

[tarekgh]

I have done an experiment to calculate the cost if we enable asynclocal context change notification. I just enabled the notification even without doing any real action inside the callback:

-        private static readonly AsyncLocal<Activity?> s_current = new AsyncLocal<Activity?>();
+       private const bool s_isRegistered = false;
+       private static readonly AsyncLocal<Activity?> s_current = new AsyncLocal<Activity?>((valueChangedArgs) => {
+           if (s_isRegistered)
+           {
+           }
+       });

Then I ran the following measurement tests:

        [Benchmark]
        public int SettingActivityCurrentInAsyncChain0()
        {
            using var a = new Activity("a1");
            a.Start();

            return 0;
        }

        [Benchmark]
        public async Task<int> SettingActivityCurrentInAsyncChain1()
        {
            using var a = new Activity("a1");
            using var b = new Activity("a2");

            a.Start();

            await Task.Run(() =>
                {
                    b.Start();
                });
            return 0;
        }

        [Benchmark]
        public async Task<int> SettingActivityCurrentInAsyncChain2()
        {
            using var a = new Activity("a1");
            using var b = new Activity("a2");
            using var c = new Activity("a3");

            a.Start();

            await Task.Run(async () =>
            {
                b.Start();
                await Task.Run(() =>
                {
                    c.Start();
                });
            });

            return 0;
        }

        [Benchmark]
        public async Task<int> SettingActivityCurrentInAsyncChain3()
        {
            using var a = new Activity("a1");
            using var b = new Activity("a2");
            using var c = new Activity("a3");
            using var d = new Activity("a4");

            a.Start();

            await Task.Run(async () =>
            {
                b.Start();
                await Task.Run(async () =>
                {
                    c.Start();

                    await Task.Run(() =>
                    {
                        d.Start();
                    });
                });
            });

            return 0;
        }

and I have gotten the following result:

Before the change

|                              Method |       Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------------------------ |-----------:|---------:|---------:|-------:|----------:|
| SettingActivityCurrentInAsyncChain0 |   191.8 ns |  1.38 ns |  1.29 ns | 0.0496 |     416 B |
| SettingActivityCurrentInAsyncChain1 | 1,243.2 ns | 12.03 ns | 10.66 ns | 0.1316 |   1,104 B |
| SettingActivityCurrentInAsyncChain2 | 2,110.0 ns | 25.74 ns | 22.82 ns | 0.2403 |   2,000 B |
| SettingActivityCurrentInAsyncChain3 | 2,903.3 ns | 18.77 ns | 16.64 ns | 0.3395 |   2,824 B |

After the change

|                              Method |       Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------------------------ |-----------:|---------:|---------:|-------:|----------:|
| SettingActivityCurrentInAsyncChain0 |   184.7 ns |  1.09 ns |  1.02 ns | 0.0582 |     488 B |	
| SettingActivityCurrentInAsyncChain1 | 1,345.6 ns | 25.97 ns | 29.91 ns | 0.1431 |   1,208 B |
| SettingActivityCurrentInAsyncChain2 | 2,242.9 ns | 21.95 ns | 19.46 ns | 0.2518 |   2,104 B |
| SettingActivityCurrentInAsyncChain3 | 3,249.8 ns | 37.52 ns | 31.33 ns | 0.3510 |   2,928 B |

The allocation cost is constant and doesn't grow so there is no allocation concern. It is clear there is some perf cost though between 6% to 11% depending on the scenario. This is the part we need to be careful with.

[noahfalk]

Tarek do you want to check what the perf overhead is if .NET only provides notification when the Activity.Current setter is called? I think that is sufficient for others to bootstrap themselves to the full set of notifications by using a 2nd AsyncLocal. I'd expect overhead when no event is attached to be very low though the overhead to achieve @pjanotti's scenario could be a little higher. I'm thinking something like this:

.NET Code:

class Activity
{
    public static Activity Current
    {
       get { ... }
       set 
       {
           CurrentActivityChanged?.Invoke(value);
           ...       
       }
    }

    public static event Action<Activity> CurrentActivityChanged;
}

Your benchmark above would be the same where CurrentActivityChanged = null. I expect the CPU will have no problem predicting that the conditional branch for Invoke() won't be taken so I'm hoping we are going to see overhead <= 1ns per Activity.

@pjanotti could then write something like this to generate the full set of notifications he is looking for:

AsyncLocal<Activity> t_currentActvity = new AsyncLocal<Activity>(ValueChanged);

void Init()
{
    Activity.CurrentActivityChanged += OnCurrentChanged;
}

void OnCurrentChanged(Activity newValue)
{
    t_currentActivity = newValue;
}

void ValueChanged(AsyncLocalValueChangedArgs<Activity> args)
{
    // profiler can do whatever it wants with the Activity here. This callback is invoked for all Activity changes
    // both ExecutionContext changes and Activity.Current setter invocations
}

[tarekgh]

@noahfalk I think your idea will work and I don't think there will be much overhead if we just have:

    public static Activity Current
    {
       get { ... }
       set 
       {
           CurrentActivityChanged?.Invoke(value);
           ...       
       }
    }

But I'll measure it anyway to ensure no issue with it.

I like the idea of having the asynclocal in the app side to track the context change. honestly I didn't think of this option before :-)

[pjanotti]

Nice idea @noahfalk! Given that

    // profiler can do whatever it wants with the Activity here. This callback is invoked for all Activity changes
    // both ExecutionContext changes and Activity.Current setter invocations

it works for my particular scenario and servers the general case.

@tarekgh a confirmation of the perf numbers would be great. Thanks!

[tarekgh]

I have run the same perf tests with the changes suggested by @noahfalk and here are the numbers:

With the change

|                              Method |       Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------------------------ |-----------:|---------:|---------:|-------:|----------:|
| SettingActivityCurrentInAsyncChain0 |   196.5 ns |  1.55 ns |  1.45 ns | 0.0496 |     416 B |
| SettingActivityCurrentInAsyncChain1 | 1,268.4 ns | 22.74 ns | 21.27 ns | 0.1316 |   1,104 B |
| SettingActivityCurrentInAsyncChain2 | 2,099.6 ns | 21.39 ns | 18.96 ns | 0.2403 |   2,000 B |
| SettingActivityCurrentInAsyncChain3 | 2,946.9 ns | 52.81 ns | 49.40 ns | 0.3395 |   2,824 B |

Baseline numbers from previous runs

|                              Method |       Mean |    Error |   StdDev |  Gen 0 | Allocated |
|------------------------------------ |-----------:|---------:|---------:|-------:|----------:|
| SettingActivityCurrentInAsyncChain0 |   191.8 ns |  1.38 ns |  1.29 ns | 0.0496 |     416 B |
| SettingActivityCurrentInAsyncChain1 | 1,243.2 ns | 12.03 ns | 10.66 ns | 0.1316 |   1,104 B |
| SettingActivityCurrentInAsyncChain2 | 2,110.0 ns | 25.74 ns | 22.82 ns | 0.2403 |   2,000 B |
| SettingActivityCurrentInAsyncChain3 | 2,903.3 ns | 18.77 ns | 16.64 ns | 0.3395 |   2,824 B |

I believe these are acceptable numbers considering the test is using Task.Run.

If we all agree, I can modify the proposal and proceed. I have a small tweak for what @noahfalk suggested to have the callback include the old and new activity.

public static event Action<Activity?, Activity?>? CurrentActivityChanged;

Also, I need to review the design guidelines for using events to ensure we conform to that.

[noahfalk]

This looks good to me

[pjanotti]

Thanks @tarekgh the numbers look good. I will update the description to reflect Noah's design.

[tarekgh]

Thanks @tarekgh the numbers look good. I will update the description to reflect Noah's design.

wait, we may need to tweak the proposal a little. We still use Noha's idea but the public API may need some little change.

[pjanotti]

np @tarekgh - I already updated it but will do it again as needed.

[tarekgh]

After looking at the design guidelines and talking to some people, here are a couple of proposals we can choose from:

// I got the feedback which when using event, must use EventHandler for consistency.  :-(

namespace System.Diagnostics
{
    public readonly struct ActivityNotification
    {
        public Activity? Previous { get; }
        public Activity? Current { get; }
    }

    public partial class Activity : IDisposable
    {
        public static event EventHandler<ActivityNotification>? ActivityCurrentChanged;
    }
}

Alternative Proposal

namespace System.Diagnostics
{
    // Didn't use Action<> to have explicit parameter names.
    public delegate void ActivityCurrentChangeEventHandler(Activity? previous, Activity? current);

    public partial class Activity : IDisposable
    {
        public static void RegisterActivityCurrentChangeNotification(ActivityCurrentChangeEventHandler handler) { }
        public static void CancelActivityCurrentChangeNotification(ActivityCurrentChangeEventHandler handler) { }
    }
}

@pjanotti @noahfalk let me know what you think.

[pjanotti]

@tarekgh assuming that perf for both is similar I prefer the first one.

Small suggestions regarding names:

1. Rename the event to CurrentChanged since the usage will already includes Activity - thus avoiding stuttering when reading the code.
2. IIRC the parameter should end in EventArgs, something like CurrentActivityChangedEventArgs - in this case Activity seems necessary since the type is under System.Diagnostics and without it in the name it would confusing. One other idea would be to define the type inside Activity, eg.: Activity.CurrentChangedEventArgs but I suspect that this is against the guidelines.

[tarekgh]

Rename the event to CurrentChanged since the usage will already includes Activity - thus avoiding stuttering when reading the code.

That makes sense.

IIRC the parameter should end in EventArgs, something like CurrentActivityChangedEventArgs - in this case Activity seems necessary since the type is under System.Diagnostics and without it in the name it would confusing.

I thought about that, but I didn't do it because users can think this type is subclassing EventArgs. I didn't use EventArgs or subclass it because this will cause memory allocation which we need to avoid. I am not objecting to suffix the name with EventArgs if you think it is not big deal having the users think it is subclassing EventArgs.

One other idea would be to define the type inside Activity, eg.: Activity.CurrentChangedEventArgs but I suspect that this is against the guidelines.

Note, the callback will be something like

static void OnActivityCurrentChange(object sender, ActivityNotificationEventArg notification) { }

If we have ActivityNotification inside Activity class that will make it

static void OnActivityCurrentChange(object sender, Activity.NotificationEventArg notification) { }

I am not seeing a big benefit having it inside Activity class,

[pjanotti]

I thought about that, but I didn't do it because users can think this type is subclassing EventArgs. I didn't use EventArgs or subclass it because this will cause memory allocation which we need to avoid.

I see and agree with you: it is more important to avoid the allocation. The name then shouldn't have EventArgs to avoid confusion.

namespace System.Diagnostics
{
    public readonly struct ActivityNotification
    {
        public Activity? Previous { get; }
        public Activity? Current { get; }
    }

    public partial class Activity : IDisposable
    {
        public static event EventHandler<ActivityNotification>? CurrentChanged;
    }
}

and then the user code will look like:

static void OnCurrentActivityChange(object sender, ActivityNotification notification)

Any ideas for the sender value?

[tarekgh]

Any ideas for the sender value?

I think it will always be null.

[pjanotti]

@tarekgh I just updated the description per our conversation above, ptal.

[tarekgh]

Could you please delete the part:

public static Activity Current
        {
                get { ... }
                set 
                {
                    ...       
                    CurrentChanged?.Invoke(null, new ActivityNotification(previousActivity, currentActivity));
                    ...       
                }
        }

This is implementation details and not part of the new APIs we need to expose. Just to not confuse anyone reviewing that. Feel free to move this part under some remarks section as needed.

Also, you may include the alternative proposal I sent just in case the reviewers can compare and discuss it too.

We may include the original Noah's proposal as an alternative proposal 2. Maybe we will get some fans to support it :-)

public static event Action<Activity?, Activity?> CurrentActivityChanged;

Thanks @pjanotti

[pjanotti]

Done.

[tarekgh]

@noahfalk let's know if you are ok with the final proposal.

[noahfalk]

[EDIT] Spoke too soon.
The API looks great but the comment in the API usage is misleading. The comment says the callback will be invoked for both setter invocations and ExecutionContext changes but that isn't the intended behavior. The callback would only be invoked for setter invocations, not ExecutionContext changes. To get EC changes the caller needs to set up a 2nd async local and sync it using the usage that I had showed. I'm guessing that was just a copy-paste error but it might mislead the reviewers.

[tarekgh]

I updated the text telling the callback will be called only when Activity.Current is set.

[pjanotti]

Forgot to mention that I had updated the usage per Noah's comment.

Not sure if useful but I wrote a quick test to validate the proposal, anyway here it goes:

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using FluentAssertions;
using Xunit;

namespace ApiProposalValidation
{
    public class ApiProposalValidation
    {
        [Fact(Timeout = 500_000)]
        public void PushAsyncContextToNative()
        {
            // Have the whole test running in a dedicated thread so Tasks get to
            // run in the thread pool, causing an async thread change. This is
            // not required on dev boxes but makes the test more robust for machines
            // with a smaller number of cores.
            // Running the test in a dedicated thread requires any exception throw
            // in the test, eg.: an assert, to be flowed to the original test thread.
            Exception testException = null;
            var testThread = new Thread(() =>
            {
                try
                {
                    // The actual code of the test will execute in different threads
                    // because of that the thread must wait for its completion.
                    TestThread().GetAwaiter().GetResult();
                }
                catch (Exception ex)
                {
                    testException = ex;
                }
            });

            testThread.Start();
            testThread.Join();

            // Check if there wasn't any assertion/exception on the test thread.
            testException.Should().BeNull();

            async Task TestThread()
            {
                // For the tests use a delegate that keeps track of current context
                // in the test thread.
                var tid2ProfilingCtx = new Dictionary<int, Guid>();

                var mockSetProfilingContext = (Guid id, int managedThreadId) =>
                {
                    if (id == Guid.Empty)
                    {
                        tid2ProfilingCtx.Remove(managedThreadId);
                        return;
                    }

                    tid2ProfilingCtx[managedThreadId] = id;
                };

                var manager = new ContextManager()
                {
                    SetProfilingContext = mockSetProfilingContext
                };

                var id = Guid.NewGuid();
                Context.Current = id;

                var currentManagedThreadId = Thread.CurrentThread.ManagedThreadId;
                var initialManagedThreadId = currentManagedThreadId;

                tid2ProfilingCtx[currentManagedThreadId].Should().Be(id);

                while (currentManagedThreadId == initialManagedThreadId)
                {
                    var blockingThreadTask = Task.Run(() => Thread.Sleep(200));
                    await Task.Delay(100);
                    await blockingThreadTask;
                    currentManagedThreadId = Thread.CurrentThread.ManagedThreadId;
                }

                // Context must have migrated to the new thread.
                tid2ProfilingCtx[currentManagedThreadId].Should().Be(id);

                // Context must have been cleaned up from old thread.
                tid2ProfilingCtx.Should().NotContainKey(initialManagedThreadId);

                Context.Current = Guid.Empty;

                tid2ProfilingCtx.Should().NotContainKey(currentManagedThreadId);
            }
        }

        // Represents an implementation like the proposed API
        private static class Context
        {
            private static AsyncLocal<Guid> _localGuid = new();

            public static event EventHandler<Guid> CurrentChanged;

            public static Guid Current
            {
                get
                {
                    return _localGuid.Value;
                }

                set
                {
                    CurrentChanged?.Invoke(null, value);
                    _localGuid.Value = value;
                }
            }
        }

        // Represents the API usage
        private class ContextManager
        {
            private readonly AsyncLocal<Guid> _localGuid;

            public ContextManager()
            {
                // TODO: need to think about possible races here.
                _localGuid = new(ValueChanged);
                Context.CurrentChanged += OnCurrentChanged;
            }

            public Action<Guid, int> SetProfilingContext { get; set; }

            public Guid Current { get => _localGuid.Value; }

            private void OnCurrentChanged(object sender, Guid e)
            {
                _localGuid.Value = e;
            }

            private void ValueChanged(AsyncLocalValueChangedArgs<Guid> args)
            {
                SetProfilingContext(args.CurrentValue, Thread.CurrentThread.ManagedThreadId);
            }
        }
    }
}

[terrajobst]

Video

• Makes sense, but we should rename the struct and make the properties init

namespace System.Diagnostics
{
    public readonly struct ActivityChangedEventArgs
    {
        public Activity? Previous { get; init; }
        public Activity? Current { get; init; }
    }

    public partial class Activity : IDisposable
    {
        public static event EventHandler<ActivityChangedEventArgs>? CurrentChanged;
    }
}