Skip to content

feat: Observe transaction on scope #4153

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
May 2, 2025
Merged

feat: Observe transaction on scope #4153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dbbf35d
initial commit
bitsandfoxes Apr 29, 2025
f4cf166
tests
bitsandfoxes Apr 29, 2025
6179c8d
Updated CHANGELOG.md
bitsandfoxes Apr 29, 2025
0dd5fe5
Merge branch 'main' into feat/sync-transaction-trace
bitsandfoxes May 2, 2025
26c5319
create new propagation context when transaction finishes
bitsandfoxes May 2, 2025
1277e78
Merge branch 'feat/sync-transaction-trace' of https://github.com/gets…
bitsandfoxes May 2, 2025
7365c5a
Updated CHANGELOG.md
bitsandfoxes May 2, 2025
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

## Unreleased

### Features
## Features

- When setting a transaction on the scope, the SDK will attempt to sync the transaction's trace context with the SDK on the native layer. Finishing a transaction will now also start a new trace ([#4153](https://github.com/getsentry/sentry-dotnet/pull/4153))
- Added `CaptureFeedback` overload with `configureScope` parameter ([#4073](https://github.com/getsentry/sentry-dotnet/pull/4073))
- Custom SessionReplay masks in MAUI Android apps ([#4121](https://github.com/getsentry/sentry-dotnet/pull/4121))

Expand Down
19 changes: 19 additions & 0 deletions src/Sentry/Scope.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,20 @@ public ITransactionTracer? Transaction
try
{
_transaction.Value = value;

if (Options.EnableScopeSync)
{
if (_transaction.Value != null)
{
// If there is a transaction set we propagate the trace to the native layer
Options.ScopeObserver?.SetTrace(_transaction.Value.TraceId, _transaction.Value.SpanId);
Copy link
Collaborator

@jamescrosswell jamescrosswell Apr 30, 2025

Choose a reason for hiding this comment

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

Is it just the TraceId and SpanId that need to be synced? Should the sample rate and sample rand also be synced (we normally put that on the DSC to ensure consistent sampling decisions)? Or will this just be used to create spans (not transactions) and so no sampling decisions need to be made?

Copy link
Contributor Author

@bitsandfoxes bitsandfoxes May 2, 2025

Choose a reason for hiding this comment

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

In the future, they should. But both the iOS and Android ScopeObservers are missing their implementation. See #4074

It needs bumping the native SDKs to specific versions and then mess around with the bindings (especially, since the API is in private/internal parts of the Cocoa/Java SDK)

Copy link
Member

@bruno-garcia bruno-garcia May 2, 2025

Choose a reason for hiding this comment

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

Please raise a ticket in the relevant repos so we don't forget about this

}
else
{
// If the transaction is being removed from the scope, reset and sync the trace as well
Options.ScopeObserver?.SetTrace(PropagationContext.TraceId, PropagationContext.SpanId);
}
}
}
finally
{
Expand Down Expand Up @@ -802,6 +816,11 @@ internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction)
if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction))
{
_transaction.Value = null;
if (Options.EnableScopeSync)
Copy link
Contributor Author

@bitsandfoxes bitsandfoxes Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Instead, this could also look like this

internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction)
{
    if (ReferenceEquals(Transaction, expectedCurrentTransaction))
    {
        Transaction = null;
    }
}

and have the property take care of locking and synching. But I'm not fully confident in this looking at the way it's set up right originally. cc @jamescrosswell

Copy link
Collaborator

@jamescrosswell jamescrosswell Apr 30, 2025

Choose a reason for hiding this comment

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

Hm, I wonder if we need the locking at all. It's an AsyncLocal<ITransactionTracer?> so in what scenarios would we see contention for reading/writing this? It has to be code from the same AsyncLocal context, which implicitly means it's on the same thread right? We shouldn't ever see multiple threads trying to access this concurrently?

@bruno-garcia sanity check?

Copy link
Member

@bruno-garcia bruno-garcia May 2, 2025

Choose a reason for hiding this comment

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

if it's always AsyncLocal we don't need any locking

{
// We have to restore the trace on the native layers to be in sync with the current scope
Options.ScopeObserver?.SetTrace(PropagationContext.TraceId, PropagationContext.SpanId);
}
}
}
finally
Expand Down
9 changes: 7 additions & 2 deletions src/Sentry/TransactionTracer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,13 @@ public void Finish()
EndTimestamp ??= _stopwatch.CurrentDateTimeOffset;
_options?.LogDebug("Finished Transaction {0}.", SpanId);

// Clear the transaction from the scope
_hub.ConfigureScope(scope => scope.ResetTransaction(this));
// Clear the transaction from the scope and regenerate the Propagation Context
// We do this so new events don't have a trace context that is "older" than the transaction that just finished
_hub.ConfigureScope(scope =>
{
scope.ResetTransaction(this);
scope.SetPropagationContext(new SentryPropagationContext());
});

// Client decides whether to discard this transaction based on sampling
_hub.CaptureTransaction(new SentryTransaction(this));
Expand Down
24 changes: 24 additions & 0 deletions test/Sentry.Tests/Protocol/SentryTransactionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,30 @@ public void Finish_ChildSpan_StatusSet_DoesNotOverride()
span.Status.Should().Be(SpanStatus.DataLoss);
}

[Fact]
public void Finish_ResetsScopeAndSetsNewPropagationContext()
{
// Arrange
var hub = Substitute.For<IHub>();
var transaction = new TransactionTracer(hub, "test name", "test op");

Action<Scope> capturedAction = null;
hub.ConfigureScope(Arg.Do<Action<Scope>>(action => capturedAction = action));

// Act
transaction.Finish();

// Assert
hub.Received(1).ConfigureScope(Arg.Any<Action<Scope>>());

capturedAction.Should().NotBeNull(); // Sanity Check
var mockScope = Substitute.For<Scope>();
capturedAction(mockScope);

mockScope.Received(1).ResetTransaction(transaction);
mockScope.Received(1).SetPropagationContext(Arg.Any<SentryPropagationContext>());
}

[Fact]
public void ISpan_GetTransaction_FromTransaction()
{
Expand Down
105 changes: 105 additions & 0 deletions test/Sentry.Tests/ScopeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,111 @@ public void Span_SetSpanThenCloseIt_ReturnsLatestOpen()
scope.Span.Should().Be(secondSpan);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void Transaction_Set_ObserverSetsTraceIfEnabled(bool enableScopeSync)
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = enableScopeSync
});
var transaction = new TransactionTracer(DisabledHub.Instance, "test-transaction", "op");
var expectedTraceId = transaction.TraceId;
var expectedSpanId = transaction.SpanId;
var expectedCount = enableScopeSync ? 1 : 0;

// Act
scope.Transaction = transaction;

// Assert
observer.Received(expectedCount).SetTrace(Arg.Is(expectedTraceId), Arg.Is(expectedSpanId));
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void Transaction_SetToNull_ObserverSetsTraceFromPropagationContextIfEnabled(bool enableScopeSync)
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = enableScopeSync
});
var transaction = new TransactionTracer(DisabledHub.Instance, "test-transaction", "op");
scope.Transaction = transaction;

var expectedTraceId = scope.PropagationContext.TraceId;
var expectedSpanId = scope.PropagationContext.SpanId;
var expectedCount = enableScopeSync ? 1 : 0;

observer.ClearReceivedCalls();

// Act
scope.Transaction = null;

// Assert
observer.Received(expectedCount).SetTrace(Arg.Is(expectedTraceId), Arg.Is(expectedSpanId));
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void ResetTransaction_MatchingTransaction_ObserverSetsTraceFromPropagationContextIfEnabled(bool enableScopeSync)
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = enableScopeSync
});
var transaction = new TransactionTracer(DisabledHub.Instance, "test-transaction", "op");
scope.Transaction = transaction;

var expectedTraceId = scope.PropagationContext.TraceId;
var expectedSpanId = scope.PropagationContext.SpanId;
var expectedCount = enableScopeSync ? 1 : 0;

observer.ClearReceivedCalls();

// Act
scope.ResetTransaction(transaction);

// Assert
observer.Received(expectedCount).SetTrace(Arg.Is(expectedTraceId), Arg.Is(expectedSpanId));
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public void ResetTransaction_NonMatchingTransaction_ObserverNotCalled(bool enableScopeSync)
{
// Arrange
var observer = Substitute.For<IScopeObserver>();
var scope = new Scope(new SentryOptions
{
ScopeObserver = observer,
EnableScopeSync = enableScopeSync
});
var transaction = new TransactionTracer(DisabledHub.Instance, "test-transaction", "op");
var differentTransaction = new TransactionTracer(DisabledHub.Instance, "different", "op");
scope.Transaction = transaction;

observer.ClearReceivedCalls();

// Act
scope.ResetTransaction(differentTransaction);

// Assert
observer.DidNotReceive().SetTrace(Arg.Any<SentryId>(), Arg.Any<SpanId>());
}

[Fact]
public void AddAttachment_AddAttachments()
{
Expand Down
Loading