Skip to content
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

fix: Stopping the SDK from creating DynamicSamplingContext Exception #2592

Merged
merged 11 commits into from
Sep 5, 2023
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

- `Hints` now accept attachments provided as a file path via `AddAttachment` method ([#2585](https://github.com/getsentry/sentry-dotnet/pull/2585))

### Fixes

- Resolved an isse where the SDK would throw an exception while attempting to set the DynamicSamplingContext but the context exists already. ([#2592](https://github.com/getsentry/sentry-dotnet/pull/2592))

### Dependencies

- Bump CLI from v2.20.5 to v2.20.6 ([#2590](https://github.com/getsentry/sentry-dotnet/pull/2590))
Expand Down
7 changes: 2 additions & 5 deletions src/Sentry/Internal/Hub.cs
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ public BaggageHeader GetBaggage()
}

var propagationContext = ScopeManager.GetCurrent().Key.PropagationContext;
propagationContext.DynamicSamplingContext ??= propagationContext.CreateDynamicSamplingContext(_options);
return propagationContext.DynamicSamplingContext.ToBaggageHeader();
return propagationContext.GetOrCreateDynamicSamplingContext(_options).ToBaggageHeader();
}

public TransactionContext ContinueTrace(
Expand Down Expand Up @@ -352,9 +351,7 @@ private void ApplyTraceContextToEvent(SentryEvent evt, SentryPropagationContext
evt.Contexts.Trace.TraceId = propagationContext.TraceId;
evt.Contexts.Trace.SpanId = propagationContext.SpanId;
evt.Contexts.Trace.ParentSpanId = propagationContext.ParentSpanId;

propagationContext.DynamicSamplingContext ??= propagationContext.CreateDynamicSamplingContext(_options);
evt.DynamicSamplingContext = propagationContext.DynamicSamplingContext;
evt.DynamicSamplingContext = propagationContext.GetOrCreateDynamicSamplingContext(_options);
}

public SentryId CaptureEvent(SentryEvent evt, Action<Scope> configureScope) =>
Expand Down
2 changes: 1 addition & 1 deletion src/Sentry/Scope.cs
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ public Scope(SentryOptions? options)
internal Scope(SentryOptions? options, SentryPropagationContext? propagationContext)
{
Options = options ?? new SentryOptions();
PropagationContext = propagationContext ?? new SentryPropagationContext();
PropagationContext = new SentryPropagationContext(propagationContext);
}

// For testing. Should explicitly require SentryOptions.
Expand Down
34 changes: 18 additions & 16 deletions src/Sentry/SentryPropagationContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,17 @@ internal class SentryPropagationContext
public SpanId SpanId { get; }
public SpanId? ParentSpanId { get; }

private DynamicSamplingContext? _dynamicSamplingContext;
public DynamicSamplingContext? DynamicSamplingContext
internal DynamicSamplingContext? _dynamicSamplingContext;

public DynamicSamplingContext GetOrCreateDynamicSamplingContext(SentryOptions options)
{
get => _dynamicSamplingContext;
set
if (_dynamicSamplingContext is null)
{
if (_dynamicSamplingContext is null)
{
_dynamicSamplingContext = value;
}
else
{
throw new Exception("Attempted to set the DynamicSamplingContext but the context exists already.");
}
options.LogDebug("Creating the Dynamic Sampling Context from the Propagation Context");
_dynamicSamplingContext = this.CreateDynamicSamplingContext(options);
}

return _dynamicSamplingContext;
}

internal SentryPropagationContext(
Expand All @@ -33,13 +29,19 @@ internal class SentryPropagationContext
TraceId = traceId;
SpanId = SpanId.Create();
ParentSpanId = parentSpanId;
DynamicSamplingContext = dynamicSamplingContext;
_dynamicSamplingContext = dynamicSamplingContext;
}

public SentryPropagationContext()
public SentryPropagationContext() : this(null)
{ }
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved

public SentryPropagationContext(SentryPropagationContext? other)
{
TraceId = SentryId.Create();
SpanId = SpanId.Create();
TraceId = other?.TraceId ?? SentryId.Create();
SpanId = other?.SpanId ?? SpanId.Create();
ParentSpanId = other?.ParentSpanId;

_dynamicSamplingContext = other?._dynamicSamplingContext;
}

public static SentryPropagationContext CreateFromHeaders(IDiagnosticLogger? logger, SentryTraceHeader? traceHeader, BaggageHeader? baggageHeader)
Expand Down
3 changes: 2 additions & 1 deletion test/Sentry.Tests/ScopeTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ public void Clone_NewScope_IncludesPropagationContext()

var clone = sut.Clone();

Assert.Same(propagationContext, clone.PropagationContext);
Assert.Equal(propagationContext.TraceId, clone.PropagationContext.TraceId);
Assert.Equal(propagationContext.SpanId, clone.PropagationContext.SpanId);
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved
}

[Fact]
Expand Down
42 changes: 39 additions & 3 deletions test/Sentry.Tests/SentryPropagationContextTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,43 @@ namespace Sentry.Tests;

public class SentryPropagationContextTests
{
[Fact]
public void CopyConstructor_CreatesCopy()
{
var original = new SentryPropagationContext();
original.GetOrCreateDynamicSamplingContext(new SentryOptions {Dsn = ValidDsn});

var copy = new SentryPropagationContext(original);

Assert.Equal(original.TraceId, copy.TraceId);
Assert.Equal(original.SpanId, copy.SpanId);
Assert.Equal(original._dynamicSamplingContext, copy._dynamicSamplingContext);
}

[Fact]
public void GetOrCreateDynamicSamplingContext_DynamicSamplingContextIsNull_CreatesDynamicSamplingContext()
{
var options = new SentryOptions { Dsn = ValidDsn };
var propagationContext = new SentryPropagationContext();

Assert.Null(propagationContext._dynamicSamplingContext); // Sanity check
_ = propagationContext.GetOrCreateDynamicSamplingContext(options);

Assert.NotNull(propagationContext._dynamicSamplingContext);
}

[Fact]
public void GetOrCreateDynamicSamplingContext_DynamicSamplingContextIsNotNull_ReturnsSameDynamicSamplingContext()
{
var options = new SentryOptions { Dsn = ValidDsn };
var propagationContext = new SentryPropagationContext();
var firstDynamicSamplingContext = propagationContext.GetOrCreateDynamicSamplingContext(options);

var secondDynamicSamplingContext = propagationContext.GetOrCreateDynamicSamplingContext(options);

Assert.Same(firstDynamicSamplingContext, secondDynamicSamplingContext);
}

[Fact]
public void CreateFromHeaders_TraceHeaderNullButBaggageExists_CreatesPropagationContextWithoutDynamicSamplingContext()
bitsandfoxes marked this conversation as resolved.
Show resolved Hide resolved
{
Expand All @@ -15,7 +52,7 @@ public void CreateFromHeaders_TraceHeaderNullButBaggageExists_CreatesPropagation

var propagationContext = SentryPropagationContext.CreateFromHeaders(null, null, baggageHeader);

Assert.Null(propagationContext.DynamicSamplingContext);
Assert.Null(propagationContext._dynamicSamplingContext);
}

[Fact]
Expand All @@ -32,7 +69,6 @@ public void CreateFromHeaders_BaggageHeaderNotNull_CreatesPropagationContextWith

var propagationContext = SentryPropagationContext.CreateFromHeaders(null, traceHeader, baggageHeader);

Assert.NotNull(propagationContext.DynamicSamplingContext);
Assert.Equal(4, propagationContext.DynamicSamplingContext.Items.Count);
Assert.Equal(4, propagationContext.GetOrCreateDynamicSamplingContext(new SentryOptions()).Items.Count);
}
}