- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 225
feat: Add W3C traceparent support to tracing client and utils #4084
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
Changes from 6 commits
9735bfc
              0954802
              d05d9c9
              d572cef
              23b3d52
              e36a636
              cffea5c
              4818a25
              f14cad5
              108a0fc
              5c510a4
              8cf3195
              66e4d5a
              4d86280
              f3cd9c7
              dc5c2cf
              cc4b563
              9c56dbb
              44c0d98
              358e1da
              cb91a92
              5ced28c
              733544c
              a6209ee
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|  | @@ -71,7 +71,8 @@ public W3CTraceHeader(SentryTraceHeader source) | |||||||||||
| var traceId = SentryId.Parse(components[1]); | ||||||||||||
| var spanId = SpanId.Parse(components[2]); | ||||||||||||
|  | ||||||||||||
| var isSampled = string.Equals(components[3], "01", StringComparison.OrdinalIgnoreCase); | ||||||||||||
| var isSampled = string.Equals(components[3], "01", StringComparison.Ordinal) || | ||||||||||||
| string.Equals(components[3], "09", StringComparison.Ordinal); | ||||||||||||
|          | ||||||||||||
| 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;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at 358e1da for a proposed solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). 👍🏻
Uh oh!
There was an error while loading. Please reload this page.