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

A code sample to show how to create new root activities that link to a previously current activity. #4957

Merged
merged 21 commits into from
Nov 1, 2023

Conversation

kalyanaj
Copy link
Contributor

Fixes #4956

Changes

This example shows how to create new root activities that link to an existing
activity. This can be useful in a fan-out or batched operation situation when
you want to create a new trace with a new root activity before invoking each
of the fanned out operations.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@kalyanaj kalyanaj requested a review from a team October 17, 2023 01:56
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #4957 (637cc5e) into main (2de73b2) will decrease coverage by 0.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4957      +/-   ##
==========================================
- Coverage   83.61%   83.39%   -0.22%     
==========================================
  Files         296      296              
  Lines       12377    12377              
==========================================
- Hits        10349    10322      -27     
- Misses       2028     2055      +27     
Flag Coverage Δ
unittests 83.39% <ø> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 4 files with indirect coverage changes

@jacob7395
Copy link

Hey,

I'm wondering what the side effects of this could be there were many threads. All starting traces.

I don't really understand .net threading so can't see what the issue could be, I also sore some mention of activity using thread IDs to match up the current but not sure about this.

Any thoughts?

@utpilla
Copy link
Contributor

utpilla commented Nov 1, 2023

I'm wondering what the side effects of this could be there were many threads. All starting traces.

@jacob7395 Could you be more specific in terms what you think might be problematic with multiple threads each starting their own trace? From the standpoint of correctness, I don't see any issue with that.

I also sore some mention of activity using thread IDs to match up the current but not sure about this.

I'm not sure I follow. Is there some doc that you came across which mentions this? In that case, could you share the link to that?

@utpilla utpilla merged commit e454dc9 into open-telemetry:main Nov 1, 2023
67 checks passed
@jacob7395
Copy link

jacob7395 commented Nov 2, 2023

Hi @utpilla,

I'll try and explain my thoughts here with the help of my favorite tool paint:

example

In the example, we observe two processes: the red process and the green process. Each box signifies a new trace. Our objective is for the green process to initiate a new trace without any parent context. During the activity setup, we "cut" the context by setting Activity.Current. After this, we start the green activity and subsequently restore the context to its prior state.

Here's my query: What if, while we set Activity.Current to null, our ongoing red process wishes to maintain its parent context? Would it inadvertently begin as a new trace? Is such a scenario plausible or even probable? For instance, in this scenario, what if half of the NumConcurrentOperations intended to preserve the parent context?

Regarding your second query, I don't have a direct source. When I was delving into open telemetry and .NET, I came across numerous forum discussions. At that time, I contemplated the problem I've detailed above, especially concerning the general use of Activity.Current. Typically, the utilization of a static in a multi-threaded application raises concerns for me. Many questions arise, especially when multiple threads attempt to access a shared resource. I'm developing this for a Blazor server, so designing for multiple processes that access shared resources has been at the forefront of my considerations. During my research, I recall seeing something about Activity.Current utilizing the threadId to guarantee you retrieve the correct context and not the parent context from another process. However, I might be mistaken on this.

@jacob7395
Copy link

To answer the sample works even if you have lots of parallel tasks, bellow is a test class to run the sample, some with parent context some without. Everything appears to work as expected.

[TestFixture]
public class NewActivity
{
    private static readonly ActivitySource MyActivitySource = new("LinksCreationWithNewRootActivities");
    
    [Test]
    public async Task TestParallelBehaviour()
    {
        using TracerProvider? tracerProvider = Sdk.CreateTracerProviderBuilder()
            .AddSource("LinksCreationWithNewRootActivities")
            .AddConsoleExporter()
            .Build();

        using (Activity? activity = MyActivitySource.StartActivity("OrchestratingActivity"))
        {
            activity?.SetTag("foo", 1);
            await DoFanoutAsync();

            using (Activity? nestedActivity = MyActivitySource.StartActivity("WrapUp"))
            {
                nestedActivity?.SetTag("foo", 1);
            }
        }
    }
    
    public async Task DoFanoutAsync()
    {
        Activity? previous = Activity.Current;

        previous.Should().NotBeNull();
        
        const int NumConcurrentOperations = 1000;

        ActivityContext activityContext = Activity.Current!.Context;
        List<ActivityLink> links = new List<ActivityLink>
        {
            new ActivityLink(activityContext),
        };

        
        List<Task> tasks = new List<Task>();

        ConcurrentBag<Activity?> withParent = new ConcurrentBag<Activity>();
        ConcurrentBag<Activity?> withoutParent = new ConcurrentBag<Activity>();
        
        Random random = new Random();
        
        for (int i = 0; i < NumConcurrentOperations; i++)
        {
            int operationIndex = i;

            Task task = Task.Run(async () =>
            {
                bool keepParent = random.Next(0, 2) == 0;

                Activity? currentParent = null;
                if (!keepParent)
                {
                    currentParent = Activity.Current;
                    Activity.Current = null;
                }

                // minor delay to try and force parallel clashes
                await Task.Delay(random.Next(0, 100));
                
                using Activity? newActivity = MyActivitySource.StartActivity(
                    ActivityKind.Internal,
                    name: $"FannedOutActivity {operationIndex + 1}",
                    links: links);

                if (!keepParent)
                {
                    withoutParent.Add(newActivity);
                    Activity.Current = currentParent;
                    return;
                }
                
                withParent.Add(newActivity);
            });

            tasks.Add(task);
        }

        await Task.WhenAll(tasks);

        foreach (Activity? activity in withoutParent)
        {
            activity.Should().NotBeNull();

            activity!.Parent.Should().BeNull();
        }
        
        foreach (Activity? activity in withParent)
        {
            activity.Should().NotBeNull();

            activity!.Parent.Should().NotBeNull();
            activity!.Parent!.Id.Should().BeEquivalentTo(previous!.Id);
        }
    }
}

@utpilla
Copy link
Contributor

utpilla commented Nov 3, 2023

@jacob7395 It's AsyncLocal that makes it all work because it flows with call context. Activity.Current is a static AsyncLocal variable.

Here's a simple code snippet which shows the behavior of AsyncLocal. I have used AsyncLocal<int> to keep it simple. You can apply this to how Acitivity.Current works. Hope that helps!

public class Program
{
    private static AsyncLocal<int> asyncLocal = new AsyncLocal<int>();

    public static void Main()
    {
        asyncLocal.Value = 100;

        Console.WriteLine($"Main: asyncLocal value before update: {asyncLocal.Value}"); // 100

        var thread = new Thread(() =>
        {
            Console.WriteLine($"Thread: asyncLocal value before update: {asyncLocal.Value}"); // 100
            asyncLocal.Value = 250;
            Console.WriteLine($"Thread: asyncLocal value after update: {asyncLocal.Value}"); // 250
        });

        thread.Start();
        thread.Join();

        var task = Task.Run(() =>
        {
            Console.WriteLine($"Task: asyncLocal value before update: {asyncLocal.Value}"); // 100
            asyncLocal.Value = 500;
            Console.WriteLine($"Task: asyncLocal value after update: {asyncLocal.Value}"); // 500
        });

        task.Wait();

        Console.WriteLine($"Main: asyncLocal value after updates from Thread and Task: {asyncLocal.Value}"); // 100
    }
}

@jacob7395
Copy link

Okay, that makes more sense now. I hadn't heard of asynlocal, I'll look into it, I really appreciate you taking the time to help. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need code sample to show how to create new root activities that link to a previously current activity.
4 participants