Skip to content
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Changelog

## Unreleased

### Features

- The sample seed used for sampling decisions is now propagated, for use in downstream custom trace samplers ([#3951](https://github.com/getsentry/sentry-dotnet/pull/3951))

## 5.1.1

### Fixes
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/BaggageHeader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ private BaggageHeader(IEnumerable<KeyValuePair<string, string>> members) =>

// We can safely return a dictionary of Sentry members, as we are in control over the keys added.
// Just to be safe though, we'll group by key and only take the first of each one.
internal IReadOnlyDictionary<string, string> GetSentryMembers() =>
internal Dictionary<string, string> GetSentryMembers() =>
Members
.Where(kvp => kvp.Key.StartsWith(SentryKeyPrefix))
.GroupBy(kvp => kvp.Key, kvp => kvp.Value)
Expand Down
45 changes: 40 additions & 5 deletions src/Sentry/DynamicSamplingContext.cs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Sentry.Internal;
using Sentry.Internal.Extensions;

namespace Sentry;
Expand All @@ -24,27 +25,33 @@ private DynamicSamplingContext(
string publicKey,
bool? sampled,
double? sampleRate = null,
double? sampleRand = null,
string? release = null,
string? environment = null,
string? transactionName = null)
{
// Validate and set required values
if (traceId == SentryId.Empty)
{
throw new ArgumentOutOfRangeException(nameof(traceId));
throw new ArgumentOutOfRangeException(nameof(traceId), "cannot be empty");
}

if (string.IsNullOrWhiteSpace(publicKey))
{
throw new ArgumentException(default, nameof(publicKey));
throw new ArgumentException("cannot be empty", nameof(publicKey));
}

if (sampleRate is < 0.0 or > 1.0)
{
throw new ArgumentOutOfRangeException(nameof(sampleRate));
throw new ArgumentOutOfRangeException(nameof(sampleRate), "Arg invalid if < 0.0 or > 1.0");
}

var items = new Dictionary<string, string>(capacity: 7)
if (sampleRand is < 0.0 or >= 1.0)
{
throw new ArgumentOutOfRangeException(nameof(sampleRand), "Arg invalid if < 0.0 or >= 1.0");
}

var items = new Dictionary<string, string>(capacity: 8)
{
["trace_id"] = traceId.ToString(),
["public_key"] = publicKey,
Expand All @@ -61,6 +68,11 @@ private DynamicSamplingContext(
items.Add("sample_rate", sampleRate.Value.ToString(CultureInfo.InvariantCulture));
}

if (sampleRand is not null)
{
items.Add("sample_rand", sampleRand.Value.ToString("N4", CultureInfo.InvariantCulture));
Copy link
Member

Choose a reason for hiding this comment

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

we want this even if we have sample_rate in? see line 68

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe so yes. The docs developer docs indicate:

If sample_rate (inside baggage) and the sampling decision (trailing -1 or -0 from the sentry-trace header) are present in the incoming trace, create sample_rand so that when compared to the incoming sample_rate it would lead to the same sampling decision that is in the sentry-trace header.

Which we're doing... and then we propagate the sample_rand so that if any downstream nodes can use this in their sampling decision.

}

if (!string.IsNullOrWhiteSpace(release))
{
items.Add("release", release);
Expand Down Expand Up @@ -99,7 +111,7 @@ private DynamicSamplingContext(
return null;
}

if (items.TryGetValue("sampled", out var sampledString) && !bool.TryParse(sampledString, out _))
if (items.TryGetValue("sampled", out var sampledString) && !bool.TryParse(sampledString, out var sampled))
{
return null;
}
Expand All @@ -111,6 +123,27 @@ private DynamicSamplingContext(
return null;
}

// See https://develop.sentry.dev/sdk/telemetry/traces/#propagated-random-value
if (items.TryGetValue("sample_rand", out var sampleRand))
Copy link
Member

Choose a reason for hiding this comment

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

odd that we validate this to be within a range as a float but then we make it a string then try to parse it back out.
Can we rely on the original value and avoid the parsing instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I follow. It may or may not be present as a string in the baggage header. If it's present then here we try to cast it to a float and ensure it's in the correct range.

If it's present but malformed then we don't create the DSC from the baggage header.

{
if (!double.TryParse(sampleRand, NumberStyles.Float, CultureInfo.InvariantCulture, out var rand) ||
rand is < 0.0 or >= 1.0)
Copy link
Member

Choose a reason for hiding this comment

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

didn't we validate this above before adding it to items? or it could already be in items when the method was caled? in which case wouldn't we be adding it twice? (I add Add so assume it's not a dictionary)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This method is creating a DSC from the baggage header. The BaggageHeader is a pretty simple class that just pulls stuff directly off a certain HTTP header, without any strong opinions about what the items in that header should be. Other vendors might be putting things on that header so we just propagate it, pretty much verbatim (we might add things to it but we never remove things from it).

But we can't control what will be on that baggage header in this SDK. A request could be made to our SDK with a malformed sentry-sample_rand item in the BaggageHeader.

{
return null;
}
}
else
{
var rand = SampleRandHelper.GenerateSampleRand(traceId);
if (!string.IsNullOrEmpty(sampledString))
{
// Ensure sample_rand is consistent with the sampling decision that has already been made
rand = bool.Parse(sampledString)
? rand * rate // 0 <= sampleRand < rate
: rate + (1 - rate) * rand; // rate < sampleRand < 1
}
items.Add("sample_rand", rand.ToString("N4", CultureInfo.InvariantCulture));
}
return new DynamicSamplingContext(items);
}

Expand All @@ -121,6 +154,7 @@ public static DynamicSamplingContext CreateFromTransaction(TransactionTracer tra
var traceId = transaction.TraceId;
var sampled = transaction.IsSampled;
var sampleRate = transaction.SampleRate!.Value;
var sampleRand = transaction.SampleRand;
var transactionName = transaction.NameSource.IsHighQuality() ? transaction.Name : null;

// These two may not have been set yet on the transaction, but we can get them directly.
Expand All @@ -132,6 +166,7 @@ public static DynamicSamplingContext CreateFromTransaction(TransactionTracer tra
publicKey,
sampled,
sampleRate,
sampleRand,
release,
environment,
transactionName);
Expand Down
43 changes: 43 additions & 0 deletions src/Sentry/Internal/FnvHash.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
namespace Sentry.Internal;

/// <summary>
/// FNV is a non-cryptographic hash.
///
/// See https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function#FNV_hash_parameters
/// </summary>
/// <remarks>
/// We use a struct to avoid heap allocations.
/// </remarks>
internal struct FnvHash
{
public FnvHash()
{
}

private const int Offset = unchecked((int)2166136261);
private const int Prime = 16777619;

private int HashCode { get; set; } = Offset;

private void Combine(byte data)
{
unchecked
{
HashCode ^= data;
HashCode *= Prime;
}
}

private static int ComputeHash(byte[] data)
{
var result = new FnvHash();
foreach (var b in data)
{
result.Combine(b);
}

return result.HashCode;
}

public static int ComputeHash(string data) => ComputeHash(Encoding.UTF8.GetBytes(data));
}
11 changes: 8 additions & 3 deletions src/Sentry/Internal/Hub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,12 @@ internal ITransactionTracer StartTransaction(
IReadOnlyDictionary<string, object?> customSamplingContext,
DynamicSamplingContext? dynamicSamplingContext)
{
var transaction = new TransactionTracer(this, context);
var transaction = new TransactionTracer(this, context)
{
SampleRand = dynamicSamplingContext?.Items.TryGetValue("sample_rand", out var sampleRand) ?? false
? double.Parse(sampleRand, NumberStyles.Float, CultureInfo.InvariantCulture)
: SampleRandHelper.GenerateSampleRand(context.TraceId.ToString())
};

// If the hub is disabled, we will always sample out. In other words, starting a transaction
// after disposing the hub will result in that transaction not being sent to Sentry.
Expand All @@ -151,7 +156,7 @@ internal ITransactionTracer StartTransaction(

if (tracesSampler(samplingContext) is { } sampleRate)
{
transaction.IsSampled = _randomValuesFactory.NextBool(sampleRate);
transaction.IsSampled = SampleRandHelper.IsSampled(transaction.SampleRand.Value, sampleRate);
transaction.SampleRate = sampleRate;
}
}
Expand All @@ -160,7 +165,7 @@ internal ITransactionTracer StartTransaction(
if (transaction.IsSampled == null)
{
var sampleRate = _options.TracesSampleRate ?? 0.0;
transaction.IsSampled = _randomValuesFactory.NextBool(sampleRate);
transaction.IsSampled = SampleRandHelper.IsSampled(transaction.SampleRand.Value, sampleRate);
transaction.SampleRate = sampleRate;
}

Expand Down
15 changes: 15 additions & 0 deletions src/Sentry/Internal/SampleRandHelper.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
namespace Sentry.Internal;

internal static class SampleRandHelper
{
internal static double GenerateSampleRand(string traceId)
=> new Random(FnvHash.ComputeHash(traceId)).NextDouble();

internal static bool IsSampled(double sampleRand, double rate) => rate switch
{
>= 1 => true,
<= 0 => false,
_ => sampleRand < rate
};

}
2 changes: 2 additions & 0 deletions src/Sentry/TransactionTracer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ internal set
/// </summary>
public double? SampleRate { get; internal set; }

internal double? SampleRand { get; set; }

/// <inheritdoc />
public SentryLevel? Level { get; set; }

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#if NETCOREAPP3_1_OR_GREATER
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
Expand Down Expand Up @@ -285,6 +284,7 @@ public async Task Baggage_header_propagates_to_outbound_requests(bool shouldProp
const string incomingBaggage =
"sentry-trace_id=75302ac48a024bde9a3b3734a82e36c8, " +
"sentry-public_key=d4d82fc1c2c4032a83f3a29aa3a3aff, " +
"sentry-sample_rand=0.1234, " +
"sentry-sample_rate=0.5, " +
"foo-bar=abc123";

Expand All @@ -299,6 +299,7 @@ public async Task Baggage_header_propagates_to_outbound_requests(bool shouldProp
"other-value=abc123, " +
"sentry-trace_id=75302ac48a024bde9a3b3734a82e36c8, " +
"sentry-public_key=d4d82fc1c2c4032a83f3a29aa3a3aff, " +
"sentry-sample_rand=0.1234, " +
"sentry-sample_rate=0.5";
}
else
Expand Down Expand Up @@ -382,6 +383,7 @@ public async Task Baggage_header_sets_dynamic_sampling_context()
const string baggage =
"sentry-trace_id=75302ac48a024bde9a3b3734a82e36c8, " +
"sentry-public_key=d4d82fc1c2c4032a83f3a29aa3a3aff, " +
"sentry-sample_rand=0.1234, " +
"sentry-sample_rate=0.5";

// Arrange
Expand Down Expand Up @@ -677,5 +679,3 @@ public async Task Transaction_TransactionNameProviderSetUnset_TransactionNameSet
transaction.NameSource.Should().Be(TransactionNameSource.Url);
}
}

#endif
1 change: 1 addition & 0 deletions test/Sentry.Testing/VerifyExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ public static SettingsTask IgnoreStandardSentryMembers(this SettingsTask setting
return settings
.ScrubMachineName()
.ScrubUserName()
.ScrubMember("sample_rand")
.AddExtraSettings(_ =>
{
_.Converters.Add(new SpansConverter());
Expand Down
Loading
Loading