fix: http integration uses correct parent span id

[bruno-garcia]

We create a span for the HTTP operation, but we don't pass that span id as the parent to the outgoing request.
Resulting in the incorrect parent-child relationship

Changing the implementation didn't break a single test though, which is quite concerning.

I added a test that failed, and with my changed passed.
Verifying that we indeed use the correct span as parent int he outgoing HTTP request

Before:

After:
tested as 5.10.0-alpha.1 on symbol collector

[jamescrosswell]

Something is off here... see comment below.

[jamescrosswell]

This shouldn't be necessary. The previous behaviour simply got the header from _hub.GetTraceHeader(), which should be reading it off the current span.

This seems like a workaround for the fact that the span representing the HttpRequest hasn't been set correctly as the current span then.

Just looking at the code, I can't see why that would be. All of the machinery to make this happen seems to be in place.

Basically the HttpMessageHandler creates the span here:

sentry-dotnet/src/Sentry/SentryHttpMessageHandler.cs

Lines 70 to 73 in dc3c0dd

	var span = _hub.GetSpan()?.StartChild(
	"http.client",
	$"{method} {url}" // e.g. "GET https://example.com"
	);

And the new span gets pushed onto the stack to become the active span here:

sentry-dotnet/src/Sentry/TransactionTracer.cs

Lines 311 to 312 in aebd6a2

	_spans.Add(span);
	_activeSpanTracker.Push(span);

If you've got the code sample that results in the erroneous parent/child relationship, it'd be good to dig into this to try to work out what's going wrong with the active span tracking.

[bruno-garcia]

we don't want the http as current span. because otherwise any other span in parallel becomes a child of the http one. I saw this behavior before and it was broken

the whole thing when using MAUI (or any global mode approach) is totally broken, with this I was able to set a span explicitly and have the parent child work properly for outgoing http regardless of what's explicitly set to the scope (see scope.Span)

[jamescrosswell]

Just to summarise a chat that @bruno-garcia and I had...

The issue with not wanting the spans for outgoing HttpRequests to be mixed up with other spans (perhaps being performed in async Tasks) is a bit more nuanced.

Currently, the way the MessageHandler creates spans is this:

sentry-dotnet/src/Sentry/SentryHttpMessageHandler.cs

Lines 68 to 73 in dc3c0dd

	// Start a span that tracks this request
	// (may be null if transaction is not set on the scope)
	var span = _hub.GetSpan()?.StartChild(
	"http.client",
	$"{method} {url}" // e.g. "GET https://example.com"
	);

In the most common case then, if there's no Scope.Transaction then no span gets created. If there is a Scope.Transaction then a span gets created as a child of the transaction... and implicitly it becomes the "last active span" for the transaction.

The way Bruno managed to work around that in the symbol collector is by setting Scope.Span (not Scope.Transaction):
https://github.com/getsentry/symbol-collector/blob/8a0fdb2594c3028f63f180b9b56b1efea1cbce34/src/SymbolCollector.Core/Client.cs#L75

This appears to be completely undocumented.

1. We should add documentation for this... it's a useful API that allows SDK users to force spans that get automatically instrumented by our integrations to be created on specific transactions without having to set those transactions on the Scope.
2. It would be fantastic if we had a more intuitive solution to this. That would need some head scratching though (if it is even possible)

[bruno-garcia]

regardless, I think taking the header of the http span is the right approach, instead of hoping some magic functions finds the right span (which clearly doesn't in some cases). We know the newly created and internal span is the parent of what comes out of the server.