feat: Add W3C traceparent support to tracing client and utils #4084

hangy · 2025-04-05T18:27:19Z

This adds support for propagating the Sentry TraceId as a W3C TraceSpan header to support #3069, loosely based on getsentry/sentry-php@a31d418

I know the PR got kinda large to support both headers. I don't have any prior experience with this SDK, so please do critique thoroughly. :)

…opagation

Don't need this as an external API.

… class I want to use this as a base for parsing, as an extension method doesn't really help for this.

… for better coverage

jamescrosswell · 2025-04-07T06:14:29Z

This is amazing @hangy - thank you so much for the PR! ❤️

It might take a day or two to get a review done, but having prior implementations in PHP should make things easier.

codecov · 2025-04-07T06:28:24Z

Codecov Report

Attention: Patch coverage is 59.70149% with 27 lines in your changes missing coverage. Please review.

Project coverage is 73.60%. Comparing base (495e742) to head (a6209ee).
Report is 544 commits behind head on main.

Files with missing lines	Patch %	Lines
...zure.Functions.Worker/HttpRequestDataExtensions.cs	0.00%	12 Missing ⚠️
src/Sentry.AspNet/HttpContextExtensions.cs	0.00%	10 Missing ⚠️
...try.AspNetCore/Extensions/HttpContextExtensions.cs	55.55%	3 Missing and 1 partial ⚠️
...unctions.Worker/SentryFunctionsWorkerMiddleware.cs	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4084      +/-   ##
==========================================
- Coverage   75.73%   73.60%   -2.13%     
==========================================
  Files         357      445      +88     
  Lines       13466    16062    +2596     
  Branches     2671     3177     +506     
==========================================
+ Hits        10198    11822    +1624     
- Misses       2593     3425     +832     
- Partials      675      815     +140

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ests

Note that equality and hashcode methods were removed, as they're not implemented in the `SentryTraceHeader` class, either.

hangy · 2025-04-07T19:07:19Z

Note: I didn't find any tests testing the HTTP headers to verify the changes to Sentry.AspNet, Sentry.AspNetCore, and Sentry.AspNet. If there are any, please don't hesitate to point me to them

jamescrosswell · 2025-04-08T04:58:42Z

Note: I didn't find any tests testing the HTTP headers to verify the changes to Sentry.AspNet, Sentry.AspNetCore, and Sentry.AspNet. If there are any, please don't hesitate to point me to them

TLDR; I think you've already got all the right tests in place. 👍🏻

Reading headers basically happens in the middleware. For example, in Sentry.AspNetCore the middleware does this:

sentry-dotnet/src/Sentry.AspNetCore/SentryMiddleware.cs

Lines 108 to 111 in 4818a25

    
           var traceHeader = context.TryGetSentryTraceHeader(_options); 
        
           traceHeader ??= context.TryGetW3CTraceHeader(_options)?.SentryTraceHeader; 
        
           var baggageHeader = context.TryGetBaggageHeader(_options); 
        
           var transactionContext = hub.ContinueTrace(traceHeader, baggageHeader);

So the InvokeAsync implementation reads headers in from the context and then passes the resulting SentryTraceHeader on to the call to hub.ContinueTrace, which your tests capture and then verify against expectations here:

sentry-dotnet/test/Sentry.AspNetCore.Tests/SentryMiddlewareTests.cs

Lines 692 to 697 in f14cad5

    
           _fixture.Hub.When(hub => hub.ContinueTrace(Arg.Any<SentryTraceHeader>(), Arg.Any<BaggageHeader>())) 
        
               .Do(callInfo => 
        
               { 
        
                   capturedTraceHeader = callInfo.Arg<SentryTraceHeader>(); 
        
                   capturedBaggageHeader = callInfo.Arg<BaggageHeader>(); 
        
               });

ASP.NET is similar - only with a different implementation of the HttpContext class.

jamescrosswell

@hangy this is awesome 🚀.

I didn't see any issues with the code - I've just made a few suggestions for some changes we could make to the unit tests (many of which are things that were present prior to this PR... but we may as well tidy them up while we're changing the code).

If the changes seem to onerous, I'm happy to push some commits myself to cross the t's and dot the i's on this.

Once again, thank you so much for the PR! ❤️

src/Sentry.AspNetCore/SentryMiddleware.cs

src/Sentry.AspNet/HttpContextExtensions.cs

src/Sentry.Azure.Functions.Worker/SentryFunctionsWorkerMiddleware.cs

test/Sentry.Tests/SentryHttpMessageHandlerTests.cs

jamescrosswell · 2025-04-08T04:28:57Z

test/Sentry.Tests/SentryHttpMessageHandlerTests.cs

+            h.Key == headerName &&
+            string.Concat(h.Value) == expectedValue);


From "A quick TL;DR: How to Implement" in the docs, it looks like both headers should be present in outgoing requests. Maybe worth expanding this test to ensure that is the case (so check for both headers regardless of what value for headerName was passed in via the theory data).

Good idea! It's quite unlikely anyone will accidentally produce a code where only one of the headers will be passed without already modifying one of the test, but verifying that the behaviour matches the definition in the task won't hurt.

Added a new, explicit test case in 44c0d98

jamescrosswell · 2025-04-08T04:36:12Z

test/Sentry.Tests/SentryHttpMessageHandlerTests.cs

+    [InlineData("75302ac48a024bde9a3b3734a82e36c8-1000000000000000-0", "sentry-trace", "75302ac48a024bde9a3b3734a82e36c8-1000000000000000-0")]
+    [InlineData("75302ac48a024bde9a3b3734a82e36c8-1000000000000000-0", "traceparent", "00-75302ac48a024bde9a3b3734a82e36c8-1000000000000000-00")]
+    [InlineData("75302ac48a024bde9a3b3734a82e36c8-1000000000000000-1", "traceparent", "00-75302ac48a024bde9a3b3734a82e36c8-1000000000000000-01")]
+    public async Task SendAsync_SentryTraceHeaderNotSet_SetsHeader_ByDefault(string traceHeader, string headerName, string expectedValue)


Not introduced by your PR, but from what I can tell this test is redundant... all of this functionality is already tested by SendAsync_SentryTraceHeaderNotSet_SetsHeader_WhenUrlMatchesPropagationOptions.

It was probably added before TracePropagationTargets was added to the SentryOptions. Feel free to remove it in this PR if you want (will save duplicating identical changes across both of the tests).

Will do. tbh, I probably just duplicated/parameterized some of the existing test cases to also support the traceparent header without checking existing tests for redundancy.

Should be resolved by 9c56dbb

jamescrosswell · 2025-04-08T04:51:02Z

test/Sentry.Tests/W3CTraceHeaderTests.cs

+    public void Parse_ValidW3CHeader_ReturnsW3CTraceHeader()
+    {
+        // Arrange
+        var header = "00-4bc7d217a6721c0e60e85e46d25fb3e5-f51f11f284da5299-01";


If you wanted to go above and beyond, we could future proof this by testing with a value of 09 for the trace flag as well... to ensure that IsSampled() still returns true in that hypothetical case. At the moment, I believe only the last bit is used, so this isn't yet necessary.

See W3C docs for trace-flags.

Oh! That's a valid case I haven't seen in the spec yet. At least with version 00, other flags won't be allowed, so I'll probably stick to string comparison for now

Should be resolved by dc5c2cf

Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>

hangy · 2025-04-08T21:07:20Z

Note: I didn't find any tests testing the HTTP headers to verify the changes to Sentry.AspNet, Sentry.AspNetCore, and Sentry.AspNet. If there are any, please don't hesitate to point me to them

TLDR; I think you've already got all the right tests in place. 👍🏻

Perfect. Thank you! I was wondering if a E2E test for them might be interesting, but if this is enough, that's fine by me.

Reading headers basically happens in the middleware. For example, in Sentry.AspNetCore the middleware does this:

sentry-dotnet/src/Sentry.AspNetCore/SentryMiddleware.cs

Lines 108 to 111 in 4818a25

var traceHeader = context.TryGetSentryTraceHeader(_options);

traceHeader ??= context.TryGetW3CTraceHeader(_options)?.SentryTraceHeader;

var baggageHeader = context.TryGetBaggageHeader(_options);

var transactionContext = hub.ContinueTrace(traceHeader, baggageHeader);

So the InvokeAsync implementation reads headers in from the context and then passes the resulting SentryTraceHeader on to the call to hub.ContinueTrace, which your tests capture and then verify against expectations here:

sentry-dotnet/test/Sentry.AspNetCore.Tests/SentryMiddlewareTests.cs

Lines 692 to 697 in f14cad5

_fixture.Hub.When(hub => hub.ContinueTrace(Arg.Any<SentryTraceHeader>(), Arg.Any<BaggageHeader>()))

.Do(callInfo =>

{

capturedTraceHeader = callInfo.Arg<SentryTraceHeader>();

capturedBaggageHeader = callInfo.Arg<BaggageHeader>();

});

ASP.NET is similar - only with a different implementation of the HttpContext class.

Yep I noticed that. And I noticed that they used (or at least some of them use) the sealed HttpCotext class directly, making it difficult to mock that dependency for a test

I didn't see any issues with the code - I've just made a few suggestions for some changes we could make to the unit tests (many of which are things that were present prior to this PR... but we may as well tidy them up while we're changing the code).

I already committed some of your suggestions. Thank you! I'm taking a look at the rest now.

SendAsync_SentryTraceHeaderNotSet_SetsHeader_ByDefault is already covered by SendAsync_SentryTraceHeaderNotSet_SetsHeader_WhenUrlMatchesPropagationOptions

…s are both set

jamescrosswell

Cool, there's just one change I think needs making to W3CTraceHeader.cs - otherwise LGTM!

I notice CI broke as well (due to XCode 16.3) but we'll address that separately.

jamescrosswell · 2025-04-09T04:24:08Z

src/Sentry/W3CTraceHeader.cs

+        var isSampled = string.Equals(components[3], "01", StringComparison.Ordinal) ||
+                        string.Equals(components[3], "09", StringComparison.Ordinal);


This looks a little bit fragile. components[2] contains a hex encoded a bit flag type. The last bit in that flag indicates whether the trace is sampled or not.

So yes, 01 and 09 are examples of values where the sampled flag is 1... there are other examples as well though (e.g. FF)

HEX Binary

0x01 0b00000001

0x09 0b00001001

.. ..

0xFF 0b11111111

Currently I think the W3C spec only uses the last bit in this flag, so in practice the value will only ever be "00" or "01" (today)... but I think worth writing this code with the expectation that the other bits in the flag may be used in the future.

Something like this:

var isSampled = Convert.ToInt32(components[3], 16) & 0x01 == 1;

I wanted to avoid parsing the string as honestly, as long as only version 00 is supported, other bits being set would be against the W3C spec, anyways. Will look into it, though. Potentially with byte.Parse

I wanted to avoid parsing the string

For performance reasons?

We could leave it as is (don't bother with 09 either) and just put a comment in the code warning that as only the least significant bit is used at the moment, this code only works for scenarios where all the other bits are zero.

I'm a bit concerned we might see variants like "01" and "1" in the string representation... which wouldn't be per the W3C spec either but, as Sentry is in the business of capturing bugs, we don't want to be crashing people's applications... so we want to take a pretty cautious approach about assumptions re inputs.

For performance reasons?

Yup. My assumption was that string comparison is highly optimized and will work well. I don't want to argue based solely on my gut feeling, so I just ran a benchmark that kinda surprised me:

benchmark

using System.Globalization; using BenchmarkDotNet.Attributes; using BenchmarkDotNet.Running; BenchmarkRunner.Run<Benchmarks>(); [MemoryDiagnoser, RankColumn] public class Benchmarks { [Benchmark] public bool StringCompareNoMatchOrdinalIgnoreCase() => "00".Equals("01", StringComparison.OrdinalIgnoreCase); [Benchmark] public bool StringCompareMatchOrdinalIgnoreCase() => "00".Equals("00", StringComparison.OrdinalIgnoreCase); [Benchmark] public bool StringCompareNoMatchOrdinal() => "00".Equals("01", StringComparison.Ordinal); [Benchmark] public bool ByteParseMatch() => byte.TryParse("01", System.Globalization.NumberStyles.HexNumber, CultureInfo.InvariantCulture, out byte b) && (b & 0x01) == 1; [Benchmark] public bool ByteParseNoMatch() => byte.TryParse("00", System.Globalization.NumberStyles.HexNumber, CultureInfo.InvariantCulture, out byte b) && (b & 0x01) == 1; [Benchmark] public bool IntParseMatch() => int.TryParse("01", System.Globalization.NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int b) && (b & 0x01) == 1; [Benchmark] public bool IntParseNoMatch() => int.TryParse("00", System.Globalization.NumberStyles.HexNumber, CultureInfo.InvariantCulture, out int b) && (b & 0x01) == 1; }

// * Summary * BenchmarkDotNet v0.14.0, Ubuntu 24.10 (Oracular Oriole) WSL AMD Ryzen 7 7800X3D, 1 CPU, 16 logical and 8 physical cores .NET SDK 9.0.202 [Host] : .NET 9.0.3 (9.0.325.11113), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI DefaultJob : .NET 9.0.3 (9.0.325.11113), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI | Method | Mean | Error | StdDev | Rank | Allocated | |-------------------------------------- |----------:|----------:|----------:|-----:|----------:| | StringCompareNoMatchOrdinalIgnoreCase | 0.0000 ns | 0.0000 ns | 0.0000 ns | 1 | - | | StringCompareMatchOrdinalIgnoreCase | 0.0000 ns | 0.0000 ns | 0.0000 ns | 1 | - | | StringCompareNoMatchOrdinal | 0.0000 ns | 0.0000 ns | 0.0000 ns | 1 | - | | ByteParseMatch | 3.0734 ns | 0.0212 ns | 0.0199 ns | 3 | - | | ByteParseNoMatch | 2.6390 ns | 0.0196 ns | 0.0183 ns | 2 | - | | IntParseMatch | 3.2432 ns | 0.0161 ns | 0.0151 ns | 4 | - | | IntParseNoMatch | 2.6208 ns | 0.0158 ns | 0.0140 ns | 2 | - | // * Warnings * ZeroMeasurement Benchmarks.StringCompareNoMatchOrdinalIgnoreCase: Default -> The method duration is indistinguishable from the empty method duration Benchmarks.StringCompareMatchOrdinalIgnoreCase: Default -> The method duration is indistinguishable from the empty method duration Benchmarks.StringCompareNoMatchOrdinal: Default -> The method duration is indistinguishable from the empty method duration // * Hints * Outliers Benchmarks.ByteParseMatch: Default -> 1 outlier was detected (4.12 ns) Benchmarks.IntParseNoMatch: Default -> 1 outlier was removed (3.76 ns) // * Legends * Mean : Arithmetic mean of all measurements Error : Half of 99.9% confidence interval StdDev : Standard deviation of all measurements Rank : Relative position of current benchmark mean among all benchmarks (Arabic style) Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B) 1 ns : 1 Nanosecond (0.000000001 sec)

This makes it look like parsing the string to a byte/int and then checking for the bitmask is orders of magnitude slower than string compare. There's no real harm in combining this, though, so I'll probably do that. 😄

I'm a bit concerned we might see variants like "01" and "1" in the string representation... which wouldn't be per the W3C spec either but, as Sentry is in the business of capturing bugs, we don't want to be crashing people's applications... so we want to take a pretty cautious approach about assumptions re inputs.

ACK. The worst case should be that a sampled = true flag isn't captured. That should be covered by the approach, though.

Please take a look at 358e1da for a proposed solution

Nice, that seems like the best of both worlds. Ordinarily the most efficient code path should run. And if processes are doing weird stuff with the traceparent headers upstream, it won't break (will just burn a few more cycles to process it). 👍🏻

…flags

…er class for use in unit tests

jamescrosswell

Huge thanks for the PR @hangy ! ❤️

hangy · 2025-04-10T14:23:23Z

Huge thanks for the PR @hangy ! ❤️

Thanks for the kind and quick review! 😊

bruno-garcia · 2025-05-14T00:39:39Z

@hangy there's a discussing to revert this PR, and remove all traceparent support from Sentry's server's SDKs.

Did you add this to contribute to .NET what was going on on PHP or was this something you need for some reason?
I wonder if we just remove it now we'll be breaking anyone, if it's worth/valid use case to keep this around with an opt-in flag, or we should just remove it.

Thanks!

hangy added 9 commits April 5, 2025 15:24

feat: Send W3C traceparent using SentryMessageHandler

9735bfc

docs: Update documentation to include 'traceparent' in HTTP header pr…

0954802

…opagation

test: Refactor Sentry trace header tests to use parameterized inputs

d05d9c9

fix: Typo

d572cef

refactor: Make SentryTraceHeaderExtensions internal

23b3d52

Don't need this as an external API.

refactor: Move W3C header related stuff to dedicated W3CTraceHeader…

e36a636

… class I want to use this as a base for parsing, as an extension method doesn't really help for this.

refactor: Update W3CTraceHeader to improve parsing and error handling

cffea5c

feat: Implement TryGetW3CTraceHeader in Middlewares and Extensions

4818a25

refactor: Convert Sentry header tests to use [Theory] with InlineData…

f14cad5

… for better coverage

hangy requested review from aritchie, jamescrosswell and Flash0ver as code owners April 5, 2025 18:27

fix: traceparent's trace-flags are mandatory

108a0fc

hangy added 2 commits April 7, 2025 20:35

docs: Add changelog entry for traceparent header support in HTTP requ…

5c510a4

…ests

test: Improve code coverage of W3CTraceHeader class

8cf3195

Note that equality and hashcode methods were removed, as they're not implemented in the `SentryTraceHeader` class, either.

Merge branch 'main' into 3069-traceparent-header

66e4d5a

jamescrosswell reviewed Apr 8, 2025

View reviewed changes

hangy and others added 2 commits April 8, 2025 10:26

docs: Document the priority of sentry-trace and traceparent headers

4d86280

Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>

test: Use HeaderName contstant to refer to HTTP tracing headers

f3cd9c7

Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>

hangy added 4 commits April 8, 2025 23:25

feat: Update W3C trace header parsing to support additional sampled flag

dc5c2cf

test: Add additional test case for Sentry trace header parsing

cc4b563

chore: Remove redundant tests

9c56dbb

SendAsync_SentryTraceHeaderNotSet_SetsHeader_ByDefault is already covered by SendAsync_SentryTraceHeaderNotSet_SetsHeader_WhenUrlMatchesPropagationOptions

test: Add test to ensure that sentry-trace and traceparent header…

44c0d98

…s are both set

jamescrosswell requested changes Apr 9, 2025

View reviewed changes

test: Enhance W3C trace header parsing tests to handle invalid trace …

358e1da

…flags

refactor: Move trace flags constants to public access in W3CTraceHead…

cb91a92

…er class for use in unit tests

jamescrosswell approved these changes Apr 9, 2025

View reviewed changes

jamescrosswell and others added 3 commits April 11, 2025 09:08

Merge branch 'main' into pr/4084

5ced28c

Update CHANGELOG.md

733544c

Merge branch 'main' into 3069-traceparent-header

a6209ee

jamescrosswell merged commit 1ebdb8f into getsentry:main Apr 14, 2025
27 checks passed

hangy deleted the 3069-traceparent-header branch April 14, 2025 08:05

Flash0ver mentioned this pull request May 13, 2025

Remove traceparent propagation (per default) #4185

Open

hangy mentioned this pull request May 13, 2025

feat: Add W3C traceparent support to tracing client and utils getsentry/sentry-dart#2843

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add W3C traceparent support to tracing client and utils #4084

feat: Add W3C traceparent support to tracing client and utils #4084

hangy commented Apr 5, 2025 •

edited

Loading

jamescrosswell commented Apr 7, 2025

codecov bot commented Apr 7, 2025 •

edited

Loading

hangy commented Apr 7, 2025

jamescrosswell commented Apr 8, 2025

jamescrosswell left a comment

jamescrosswell Apr 8, 2025

hangy Apr 8, 2025

hangy Apr 8, 2025

jamescrosswell Apr 8, 2025

hangy Apr 8, 2025

hangy Apr 8, 2025

jamescrosswell Apr 8, 2025

hangy Apr 8, 2025

hangy Apr 8, 2025

hangy commented Apr 8, 2025

jamescrosswell left a comment

jamescrosswell Apr 9, 2025

hangy Apr 9, 2025

jamescrosswell Apr 9, 2025

hangy Apr 9, 2025

hangy Apr 9, 2025

jamescrosswell Apr 9, 2025

jamescrosswell left a comment

hangy commented Apr 10, 2025

bruno-garcia commented May 14, 2025

		h.Key == headerName &&
		string.Concat(h.Value) == expectedValue);

		var isSampled = string.Equals(components[3], "01", StringComparison.Ordinal) \|\|
		string.Equals(components[3], "09", StringComparison.Ordinal);

feat: Add W3C traceparent support to tracing client and utils #4084

feat: Add W3C traceparent support to tracing client and utils #4084

Conversation

hangy commented Apr 5, 2025 • edited Loading

jamescrosswell commented Apr 7, 2025

codecov bot commented Apr 7, 2025 • edited Loading

Codecov Report

hangy commented Apr 7, 2025

jamescrosswell commented Apr 8, 2025

jamescrosswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangy commented Apr 8, 2025

jamescrosswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamescrosswell left a comment

Choose a reason for hiding this comment

hangy commented Apr 10, 2025

bruno-garcia commented May 14, 2025

hangy commented Apr 5, 2025 •

edited

Loading

codecov bot commented Apr 7, 2025 •

edited

Loading