Option to link Http Request Spans to the Root Transaction?

[lucas-zimerman]

Package

Sentry.AspNetCore

.NET Flavor

.NET Core

.NET Version

6.0.0

OS

Windows

SDK Version

3.17.1

Self-Hosted Sentry Version

No response

Steps to Reproduce

Sample project: https://github.com/lucas-zimerman/httpclient-stack-sample

• run and once you get information on the browser an event will be sent to Sentry SDKs org on the .NET project.

• but it should be replicable if you call an httpclient in parallel inside of a request.

Expected Result

Looking at the code, you may expect that each span generated on the ForEach loop would have the parentId as GetAnimals

Actual Result

https://sentry.io/organizations/sentry-sdks/discover/sentry-dotnet:dc044830d1994a2685d81dd22eb3cb7c/?field=title&field=event.type&field=project&field=user.display&field=timestamp&name=All+Events&query=&sort=-timestamp&statsPeriod=1h&yAxis=count%28%29

Due to the defined behavior the HttpClient will try to get the last opened span and due to the parallel behavior, you'll end up with multiple GetDog spans inside of each other.

One idea is to set an option that affects spans not generated by the user where you can define if the parent will be the last opened Span or always the scope Transaction, (or maybe even an user defined logic?).

EDIT: A brainstorm. Spans could have a parameter that would hide it from GetSpan, let's name it AllowChildren. in specific cases like a http request, we could easily assume it's the last span from a stack so we set AllowChildren as false on HttpClient spans, with that, when you call GetSpan you'll always get the last open span that could have a children.
The same idea could be applied to the DiagnosticSource Spans or even into parallel code from users.

[mattjohnsonpint]

Given how the results are nested under each other instead of the root, I consider this more of a bug than an enhancement.

[mattjohnsonpint]

I took another look at this recently, and I have a solution. My last statement was incorrect - it is an enhancement that is required, and the SDK is currently working as designed. The good news is, the enhancement is already specified in the Sentry Unified SDK spec:

The Scope holds a reference to the current Span or Transaction.

• Scope Introduce setSpan
  • This can be used internally to pass a Span / Transaction around so that integrations can attach children to it

In the .NET SDK, Scope currently only has a GetSpan method. There's no way to set the span. Adding that functionality (and making the get/set span a property), the example code from @lucas-zimerman's sample becomes this:

[HttpGet("GetAnimals")]
public async Task<IEnumerable<int>> GetDogs()
{
    var dogs = Enumerable.Range(0, 200).ToList();

    // Get the current span before starting the parallel operation.
    var baseSpan = SentrySdk.GetSpan();
    
    // Start the parallel operation.
    var parallelOptions = new ParallelOptions {MaxDegreeOfParallelism = 6};
    await Parallel.ForEachAsync(dogs, parallelOptions, async (dog, ct) =>
    {
        // Create a new scope for each iteration so they don't step on each other
        // and we don't interfere with the parent scope.
        await SentrySdk.WithScopeAsync(async scope =>
        {
            // Set the new scope's span to the base span we collected earlier.
            // This ensures that any operation will be rooted to the starting point,
            // rather than to just the latest span that happens to be open.
            scope.Span = baseSpan;
            
            // Now do the actual work.
            await _httpClient.GetAsync($"{baseUri}/GetDog/{dog}", ct);
        });
    });

    return dogs;
}

Note that WithScopeAsync was added in 3.31.0 with #2303.

So all that's needed is to implement the Scope.Span property setter, and then it will work.

[mattjohnsonpint]

[mattjohnsonpint]

I'm a bit embarrassed now, as it seems we reached a similar conclusion some time ago with #1845.

[mattjohnsonpint]

Fixed in #2364

[lucas-zimerman]

I'm a bit embarrassed now, as it seems we reached a similar conclusion some time ago with #1845.

The important thing is that it got fixed :D