Skip to content

fix: http integration uses correct parent span id #4264

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 9, 2025
Merged

fix: http integration uses correct parent span id #4264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
21c7fc1
use correct parent span in http
bruno-garcia Jun 7, 2025
0233ca2
test
bruno-garcia Jun 7, 2025
18fae46
changelog
bruno-garcia Jun 7, 2025
4655627
release: 5.11.0-alpha.1
getsentry-bot Jun 7, 2025
20fa9b7
Merge branch 'release/5.11.0-alpha.1' into fix/http-parent-span
Jun 7, 2025
59fb4e8
Update CHANGELOG.md
bruno-garcia Jun 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Fixes

- The HTTP instrumentation uses the span created for the outgoing request in the sentry-trace header, fixing the parent-child relationship between client and server ([#4264](https://github.com/getsentry/sentry-dotnet/pull/4264))

### Dependencies

- Bump Native SDK from v0.8.5 to v0.9.0 ([#4260](https://github.com/getsentry/sentry-dotnet/pull/4260))
Expand Down
2 changes: 1 addition & 1 deletion Directory.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<Project>

<PropertyGroup>
<VersionPrefix>5.10.0</VersionPrefix>
<VersionPrefix>5.11.0-alpha.1</VersionPrefix>
<LangVersion>13</LangVersion>
<AccelerateBuildsInVisualStudio>true</AccelerateBuildsInVisualStudio>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
Expand Down
14 changes: 8 additions & 6 deletions src/Sentry/SentryMessageHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ protected override async Task<HttpResponseMessage> SendAsync(
var span = ProcessRequest(request, method, url);
try
{
PropagateTraceHeaders(request, url);
PropagateTraceHeaders(request, url, span);
var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
HandleResponse(response, span, method, url);
return response;
Expand All @@ -108,7 +108,7 @@ protected override HttpResponseMessage Send(HttpRequestMessage request, Cancella
var span = ProcessRequest(request, method, url);
try
{
PropagateTraceHeaders(request, url);
PropagateTraceHeaders(request, url, span);
var response = base.Send(request, cancellationToken);
HandleResponse(response, span, method, url);
return response;
Expand All @@ -121,7 +121,7 @@ protected override HttpResponseMessage Send(HttpRequestMessage request, Cancella
}
#endif

private void PropagateTraceHeaders(HttpRequestMessage request, string url)
private void PropagateTraceHeaders(HttpRequestMessage request, string url, ISpan? parentSpan)
{
// Assign a default inner handler for convenience the first time this is used.
// We can't do this in a constructor, or it will throw when used with HttpMessageHandlerBuilderFilter.
Expand All @@ -135,15 +135,17 @@ private void PropagateTraceHeaders(HttpRequestMessage request, string url)

if (_options?.TracePropagationTargets.MatchesSubstringOrRegex(url) is true or null)
{
AddSentryTraceHeader(request);
AddSentryTraceHeader(request, parentSpan);
AddBaggageHeader(request);
}
}

private void AddSentryTraceHeader(HttpRequestMessage request)
private void AddSentryTraceHeader(HttpRequestMessage request, ISpan? parentSpan)
{
// Set trace header if it hasn't already been set
if (!request.Headers.Contains(SentryTraceHeader.HttpHeaderName) && _hub.GetTraceHeader() is { } traceHeader)
if (!request.Headers.Contains(SentryTraceHeader.HttpHeaderName) &&
// Use the span created by this integration as parent, instead of its own parent
(parentSpan?.GetTraceHeader() ?? _hub.GetTraceHeader()) is { } traceHeader)
Copy link
Collaborator

@jamescrosswell jamescrosswell Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@bruno-garcia bruno-garcia Jun 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@jamescrosswell jamescrosswell Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

{
request.Headers.Add(SentryTraceHeader.HttpHeaderName, traceHeader.ToString());
}
Expand Down
34 changes: 34 additions & 0 deletions test/Sentry.Tests/SentryHttpMessageHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,40 @@ public async Task SendAsync_SentryTraceHeaderNotSet_SetsHeader_ByDefault()
string.Concat(h.Value) == "75302ac48a024bde9a3b3734a82e36c8-1000000000000000-0");
}

[Fact]
public async Task SendAsync_SentryTraceHeaderNotSet_SetsHeader_ToCorrectParent()
{
// Arrange
var hub = Substitute.For<IHub>();

const string rootTraceHeader = "75302ac48a024bde9a3b3734a82e36c8-1000000000000000-1";
hub.GetTraceHeader().ReturnsForAnyArgs(
SentryTraceHeader.Parse(rootTraceHeader));
var parentSpan = Substitute.For<ISpan>();
parentSpan.GetTraceHeader().ReturnsForAnyArgs(
SentryTraceHeader.Parse(rootTraceHeader));
hub.GetSpan().ReturnsForAnyArgs(parentSpan);
const string httpSpanTraceHeader = "75302ac48a024bde9a3b3734a82e36c8-2000000000000000-1";
var httpSpan = Substitute.For<ISpan>();
httpSpan.GetTraceHeader().ReturnsForAnyArgs(
SentryTraceHeader.Parse(httpSpanTraceHeader));
parentSpan.StartChild(Arg.Any<string>()).ReturnsForAnyArgs(httpSpan);

using var innerHandler = new RecordingHttpMessageHandler(new FakeHttpMessageHandler());
using var sentryHandler = new SentryHttpMessageHandler(innerHandler, hub);
using var client = new HttpClient(sentryHandler);

// Act
await client.GetAsync("https://localhost/");

using var request = innerHandler.GetRequests().Single();

// Assert
request.Headers.Should().Contain(h =>
h.Key == "sentry-trace" &&
string.Concat(h.Value) == httpSpanTraceHeader);
}

[Fact]
public async Task SendAsync_SentryTraceHeaderNotSet_SetsHeader_WhenUrlMatchesPropagationOptions()
{
Expand Down
Loading