From 8bab4d51f21ba755a136c91ba503e2dbf1ddd4ab Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Thu, 14 Sep 2023 15:51:59 +0200 Subject: [PATCH 1/9] going with long instead of string --- src/Sentry/SpanId.cs | 75 ++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index 85aa9405e9..2927f1e164 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -1,4 +1,5 @@ using Sentry.Extensibility; +using Sentry.Internal; namespace Sentry; @@ -7,58 +8,60 @@ namespace Sentry; /// public readonly struct SpanId : IEquatable, IJsonSerializable { - private readonly string _value; + private static readonly RandomValuesFactory Random = new SynchronizedRandomValuesFactory(); - private const string EmptyValue = "0000000000000000"; + private readonly long _value; + + private long GetValue() => _value; /// /// An empty Sentry span ID. /// - public static readonly SpanId Empty = new(EmptyValue); + public static readonly SpanId Empty = new(0); /// /// Creates a new instance of a Sentry span Id. /// - public SpanId(string value) => _value = value; - - // This method is used to return a string with all zeroes in case - // the `_value` is equal to null. - // It can be equal to null because this is a struct and always has - // a parameterless constructor that evaluates to an instance with - // all fields initialized to default values. - // Effectively, using this method instead of just referencing `_value` - // makes the behavior more consistent, for example: - // default(SpanId).ToString() -> "0000000000000000" - // default(SpanId) == SpanId.Empty -> true - private string GetNormalizedValue() => !string.IsNullOrWhiteSpace(_value) - ? _value - : EmptyValue; + // TODO: mark as internal in version 4 + public SpanId(string value) => long.TryParse(value, NumberStyles.HexNumber, null, out _value); + + /// + /// Creates a new instance of a Sentry span Id. + /// + /// + public SpanId(long value) => _value = value; /// - public bool Equals(SpanId other) => StringComparer.Ordinal.Equals( - GetNormalizedValue(), - other.GetNormalizedValue()); + public bool Equals(SpanId other) => GetValue() == other.GetValue(); /// public override bool Equals(object? obj) => obj is SpanId other && Equals(other); /// - public override int GetHashCode() => StringComparer.Ordinal.GetHashCode(GetNormalizedValue()); + public override int GetHashCode() => StringComparer.Ordinal.GetHashCode(_value); /// - public override string ToString() => GetNormalizedValue(); + public override string ToString() => _value.ToString("x8"); - // Note: spans are sentry IDs with only 16 characters, rest being truncated. - // This is obviously a bad idea as it invalidates GUID's uniqueness properties - // (https://devblogs.microsoft.com/oldnewthing/20080627-00/?p=21823) - // but all other SDKs do it this way, so we have no choice but to comply. /// /// Generates a new Sentry ID. /// - public static SpanId Create() => new(Guid.NewGuid().ToString("n")[..16]); + public static SpanId Create() + { + var buf = new byte[8]; + long random; + + do + { + Random.NextBytes(buf); + random = BitConverter.ToInt64(buf, 0); + } while (random == 0); + + return new SpanId(random); + } /// - public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? _) => writer.WriteStringValue(GetNormalizedValue()); + public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? _) => writer.WriteStringValue(ToString()); /// /// Parses from string. @@ -91,20 +94,4 @@ public static SpanId FromJson(JsonElement json) /// The from the . /// public static implicit operator string(SpanId id) => id.ToString(); - - // Note: no implicit conversion from `string` to `SpanId` as that leads to serious bugs. - // For example, given a method: - // transaction.StartChild(SpanId parentSpanId, string operation) - // And an *extension* method: - // transaction.StartChild(string operation, string description) - // The following code: - // transaction.StartChild("foo", "bar") - // Will resolve to the first method and not the second, which is incorrect. - - /* - /// - /// A from a . - /// - public static implicit operator SpanId(string value) => new(value); - */ } From 96e3dee03dd44cec0a3e80ab7b37a17dcb3ce1c4 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Thu, 14 Sep 2023 15:52:06 +0200 Subject: [PATCH 2/9] verify --- test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt | 1 + test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt | 1 + test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt | 1 + test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt | 1 + 4 files changed, 4 insertions(+) diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt index 27e6905bae..df9f2642f1 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.Core3_1.verified.txt @@ -932,6 +932,7 @@ namespace Sentry public readonly struct SpanId : Sentry.IJsonSerializable, System.IEquatable { public static readonly Sentry.SpanId Empty; + public SpanId(long value) { } public SpanId(string value) { } public bool Equals(Sentry.SpanId other) { } public override bool Equals(object? obj) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt index 875f2835fc..8ef3bbea90 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet6_0.verified.txt @@ -933,6 +933,7 @@ namespace Sentry public readonly struct SpanId : Sentry.IJsonSerializable, System.IEquatable { public static readonly Sentry.SpanId Empty; + public SpanId(long value) { } public SpanId(string value) { } public bool Equals(Sentry.SpanId other) { } public override bool Equals(object? obj) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt index 875f2835fc..8ef3bbea90 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.DotNet7_0.verified.txt @@ -933,6 +933,7 @@ namespace Sentry public readonly struct SpanId : Sentry.IJsonSerializable, System.IEquatable { public static readonly Sentry.SpanId Empty; + public SpanId(long value) { } public SpanId(string value) { } public bool Equals(Sentry.SpanId other) { } public override bool Equals(object? obj) { } diff --git a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt index dd4c09e8ed..a3d09d677d 100644 --- a/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt +++ b/test/Sentry.Tests/ApiApprovalTests.Run.Net4_8.verified.txt @@ -931,6 +931,7 @@ namespace Sentry public readonly struct SpanId : Sentry.IJsonSerializable, System.IEquatable { public static readonly Sentry.SpanId Empty; + public SpanId(long value) { } public SpanId(string value) { } public bool Equals(Sentry.SpanId other) { } public override bool Equals(object? obj) { } From 2667700c839d8738d3bfa9fd94f918b179c6f5a6 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Thu, 14 Sep 2023 16:02:13 +0200 Subject: [PATCH 3/9] benchmark --- benchmarks/Sentry.Benchmarks/SpanIdBenchmarks.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 benchmarks/Sentry.Benchmarks/SpanIdBenchmarks.cs diff --git a/benchmarks/Sentry.Benchmarks/SpanIdBenchmarks.cs b/benchmarks/Sentry.Benchmarks/SpanIdBenchmarks.cs new file mode 100644 index 0000000000..41de5686d0 --- /dev/null +++ b/benchmarks/Sentry.Benchmarks/SpanIdBenchmarks.cs @@ -0,0 +1,12 @@ +using BenchmarkDotNet.Attributes; + +namespace Sentry.Benchmarks; + +public class SpanIdBenchmarks +{ + [Benchmark(Description = "Creates a Span ID")] + public void CreateSpanId() + { + SpanId.Create(); + } +} From 6e9854f6c4af7f569d9818a8469f935a838ba93c Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Thu, 14 Sep 2023 19:55:03 +0200 Subject: [PATCH 4/9] even better --- src/Sentry/Internal/RandomValuesFactory.cs | 1 + .../Internal/SynchronizedRandomValuesFactory.cs | 8 +++++++- src/Sentry/SpanId.cs | 14 ++++++++++++-- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/Sentry/Internal/RandomValuesFactory.cs b/src/Sentry/Internal/RandomValuesFactory.cs index 5c57429c7a..b7de78ca84 100644 --- a/src/Sentry/Internal/RandomValuesFactory.cs +++ b/src/Sentry/Internal/RandomValuesFactory.cs @@ -6,6 +6,7 @@ internal abstract class RandomValuesFactory public abstract int NextInt(int minValue, int maxValue); public abstract double NextDouble(); public abstract void NextBytes(byte[] bytes); + public abstract void NextBytes(Span bytes); public bool NextBool(double rate) => rate switch { diff --git a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs index 75b0988857..4ee0f1b76c 100644 --- a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs +++ b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs @@ -7,6 +7,7 @@ internal class SynchronizedRandomValuesFactory : RandomValuesFactory public override int NextInt(int minValue, int maxValue) => Random.Shared.Next(minValue, maxValue); public override double NextDouble() => Random.Shared.NextDouble(); public override void NextBytes(byte[] bytes) => Random.Shared.NextBytes(bytes); + public override void NextBytes(Span bytes) => Random.Shared.NextBytes(bytes); #else private static readonly AsyncLocal LocalRandom = new(); private static Random Random => LocalRandom.Value ??= new Random(); @@ -15,5 +16,10 @@ internal class SynchronizedRandomValuesFactory : RandomValuesFactory public override int NextInt(int minValue, int maxValue) => Random.Next(minValue, maxValue); public override double NextDouble() => Random.NextDouble(); public override void NextBytes(byte[] bytes) => Random.NextBytes(bytes); + public override void NextBytes(Span bytes) => Random.NextBytes(bytes +#if NETSTANDARD2_0 || NET461 + .ToArray() #endif -} \ No newline at end of file + ); +#endif +} diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index 2927f1e164..ab1ff00619 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -48,13 +48,23 @@ namespace Sentry; /// public static SpanId Create() { - var buf = new byte[8]; + Span buf = +#if NETSTANDARD2_0 || NET461 + new byte[8]; +#else + stackalloc byte[8]; +#endif long random; do { Random.NextBytes(buf); - random = BitConverter.ToInt64(buf, 0); + random = BitConverter.ToInt64( +#if NETSTANDARD2_0 || NET461 + buf.ToArray(), 0); +#else + buf); +#endif } while (random == 0); return new SpanId(random); From 1fc957da72cde71d6df6a053055b17a73bada34a Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Fri, 15 Sep 2023 12:05:37 +0200 Subject: [PATCH 5/9] fixed test random --- test/Sentry.Testing/IsolatedRandomValuesFactory.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs index 9da87fbd0a..881034066a 100644 --- a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs +++ b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs @@ -11,4 +11,9 @@ internal class IsolatedRandomValuesFactory : RandomValuesFactory public override double NextDouble() => _random.NextDouble(); public override void NextBytes(byte[] bytes) => _random.NextBytes(bytes); + public override void NextBytes(Span bytes) => _random.NextBytes(bytes +#if NETSTANDARD2_0 || NET48 + .ToArray() +#endif + ); } From 95173328f405675c45e583c8b30cd1cdfb9c7590 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Mon, 18 Sep 2023 09:10:01 +0200 Subject: [PATCH 6/9] no allocations during writing --- src/Sentry/SpanId.cs | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index ab1ff00619..6d705db24b 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -8,6 +8,7 @@ namespace Sentry; /// public readonly struct SpanId : IEquatable, IJsonSerializable { + private static readonly char[] HexChars = { '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f' }; private static readonly RandomValuesFactory Random = new SynchronizedRandomValuesFactory(); private readonly long _value; @@ -54,24 +55,33 @@ public static SpanId Create() #else stackalloc byte[8]; #endif - long random; - - do - { - Random.NextBytes(buf); - random = BitConverter.ToInt64( + Random.NextBytes(buf); + var random = BitConverter.ToInt64( #if NETSTANDARD2_0 || NET461 - buf.ToArray(), 0); + buf.ToArray(), 0); #else - buf); + buf); #endif - } while (random == 0); return new SpanId(random); } /// - public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? _) => writer.WriteStringValue(ToString()); + public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? _) + { + Span convertedBytes = stackalloc byte[sizeof(long)]; + Unsafe.As(ref convertedBytes[0]) = _value; + + Span output = stackalloc char[16]; + for (var i = 0; i < convertedBytes.Length; i++) + { + var value = convertedBytes[i]; + output[i * 2] = HexChars[value >> 4]; + output[i * 2 + 1] = HexChars[value & 0xF]; + } + + writer.WriteStringValue(output); + } /// /// Parses from string. From 009b6704b039d65fac5b41c6dcefc399ae05e168 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Tue, 19 Sep 2023 12:31:33 +0200 Subject: [PATCH 7/9] make. it. work. --- src/Sentry/Internal/RandomValuesFactory.cs | 3 +++ .../Internal/SynchronizedRandomValuesFactory.cs | 5 ----- src/Sentry/SpanId.cs | 15 ++++++++------- .../Sentry.Testing/IsolatedRandomValuesFactory.cs | 7 +++---- 4 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/Sentry/Internal/RandomValuesFactory.cs b/src/Sentry/Internal/RandomValuesFactory.cs index b7de78ca84..d89f487452 100644 --- a/src/Sentry/Internal/RandomValuesFactory.cs +++ b/src/Sentry/Internal/RandomValuesFactory.cs @@ -6,7 +6,10 @@ internal abstract class RandomValuesFactory public abstract int NextInt(int minValue, int maxValue); public abstract double NextDouble(); public abstract void NextBytes(byte[] bytes); + +#if NET6_0_OR_GREATER public abstract void NextBytes(Span bytes); +#endif public bool NextBool(double rate) => rate switch { diff --git a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs index 4ee0f1b76c..6e86a28ed8 100644 --- a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs +++ b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs @@ -16,10 +16,5 @@ internal class SynchronizedRandomValuesFactory : RandomValuesFactory public override int NextInt(int minValue, int maxValue) => Random.Next(minValue, maxValue); public override double NextDouble() => Random.NextDouble(); public override void NextBytes(byte[] bytes) => Random.NextBytes(bytes); - public override void NextBytes(Span bytes) => Random.NextBytes(bytes -#if NETSTANDARD2_0 || NET461 - .ToArray() -#endif - ); #endif } diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index 6d705db24b..ae6a938deb 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -49,18 +49,19 @@ namespace Sentry; /// public static SpanId Create() { - Span buf = -#if NETSTANDARD2_0 || NET461 - new byte[8]; +#if NET6_0_OR_GREATER + Span buf = stackalloc byte[8]; #else - stackalloc byte[8]; + byte[] buf = new byte[8]; #endif + Random.NextBytes(buf); - var random = BitConverter.ToInt64( + + var random = BitConverter.ToInt64(buf #if NETSTANDARD2_0 || NET461 - buf.ToArray(), 0); + , 0); #else - buf); + ); #endif return new SpanId(random); diff --git a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs index 881034066a..04838e8c41 100644 --- a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs +++ b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs @@ -11,9 +11,8 @@ internal class IsolatedRandomValuesFactory : RandomValuesFactory public override double NextDouble() => _random.NextDouble(); public override void NextBytes(byte[] bytes) => _random.NextBytes(bytes); - public override void NextBytes(Span bytes) => _random.NextBytes(bytes -#if NETSTANDARD2_0 || NET48 - .ToArray() +#if NET6_0_OR_GREATER + public override void NextBytes(Span bytes) => _random.NextBytes(bytes); #endif - ); + } From 5ba3b3fe1f61550bbe0c64caf47958fd5b5f2034 Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Tue, 19 Sep 2023 13:26:39 +0200 Subject: [PATCH 8/9] pull supported versions down --- src/Sentry/Internal/RandomValuesFactory.cs | 2 +- .../Internal/SynchronizedRandomValuesFactory.cs | 5 +++++ src/Sentry/SpanId.cs | 13 +++++++------ test/Sentry.Testing/IsolatedRandomValuesFactory.cs | 3 ++- 4 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/Sentry/Internal/RandomValuesFactory.cs b/src/Sentry/Internal/RandomValuesFactory.cs index d89f487452..e2a5f59f92 100644 --- a/src/Sentry/Internal/RandomValuesFactory.cs +++ b/src/Sentry/Internal/RandomValuesFactory.cs @@ -7,7 +7,7 @@ internal abstract class RandomValuesFactory public abstract double NextDouble(); public abstract void NextBytes(byte[] bytes); -#if NET6_0_OR_GREATER +#if !(NETSTANDARD2_0 || NET461) public abstract void NextBytes(Span bytes); #endif diff --git a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs index 6e86a28ed8..b1f591ce12 100644 --- a/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs +++ b/src/Sentry/Internal/SynchronizedRandomValuesFactory.cs @@ -16,5 +16,10 @@ internal class SynchronizedRandomValuesFactory : RandomValuesFactory public override int NextInt(int minValue, int maxValue) => Random.Next(minValue, maxValue); public override double NextDouble() => Random.NextDouble(); public override void NextBytes(byte[] bytes) => Random.NextBytes(bytes); + +#if !(NETSTANDARD2_0 || NET461) + public override void NextBytes(Span bytes) => Random.NextBytes(bytes); +#endif + #endif } diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index ae6a938deb..d63a1c4791 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -49,10 +49,10 @@ namespace Sentry; /// public static SpanId Create() { -#if NET6_0_OR_GREATER - Span buf = stackalloc byte[8]; -#else +#if NETSTANDARD2_0 || NET461 byte[] buf = new byte[8]; +#else + Span buf = stackalloc byte[8]; #endif Random.NextBytes(buf); @@ -73,12 +73,13 @@ public void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? _) Span convertedBytes = stackalloc byte[sizeof(long)]; Unsafe.As(ref convertedBytes[0]) = _value; + // Going backwards through the array to preserve the order of the output hex string (i.e. `4e76` -> `76e4`) Span output = stackalloc char[16]; - for (var i = 0; i < convertedBytes.Length; i++) + for (var i = convertedBytes.Length - 1; i >= 0; i--) { var value = convertedBytes[i]; - output[i * 2] = HexChars[value >> 4]; - output[i * 2 + 1] = HexChars[value & 0xF]; + output[(convertedBytes.Length - 1 - i) * 2] = HexChars[value >> 4]; + output[(convertedBytes.Length - 1 - i) * 2 + 1] = HexChars[value & 0xF]; } writer.WriteStringValue(output); diff --git a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs index 04838e8c41..b8d7123108 100644 --- a/test/Sentry.Testing/IsolatedRandomValuesFactory.cs +++ b/test/Sentry.Testing/IsolatedRandomValuesFactory.cs @@ -11,7 +11,8 @@ internal class IsolatedRandomValuesFactory : RandomValuesFactory public override double NextDouble() => _random.NextDouble(); public override void NextBytes(byte[] bytes) => _random.NextBytes(bytes); -#if NET6_0_OR_GREATER + +#if !(NETSTANDARD2_0 || NET48) public override void NextBytes(Span bytes) => _random.NextBytes(bytes); #endif From 063c1f748c7b2ccd7fbac0266b17062b9f820cfe Mon Sep 17 00:00:00 2001 From: bitsandfoxes Date: Tue, 19 Sep 2023 17:17:59 +0200 Subject: [PATCH 9/9] review --- src/Sentry/SpanId.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Sentry/SpanId.cs b/src/Sentry/SpanId.cs index d63a1c4791..5ede9357f2 100644 --- a/src/Sentry/SpanId.cs +++ b/src/Sentry/SpanId.cs @@ -23,7 +23,6 @@ namespace Sentry; /// /// Creates a new instance of a Sentry span Id. /// - // TODO: mark as internal in version 4 public SpanId(string value) => long.TryParse(value, NumberStyles.HexNumber, null, out _value); /// @@ -33,7 +32,7 @@ namespace Sentry; public SpanId(long value) => _value = value; /// - public bool Equals(SpanId other) => GetValue() == other.GetValue(); + public bool Equals(SpanId other) => GetValue().Equals(other.GetValue()); /// public override bool Equals(object? obj) => obj is SpanId other && Equals(other);