fix: Removed redundant scope sync after transaction finish

[bitsandfoxes]

Problem Description

When a transaction gets finished it also resets itself on the scope. This triggers a "manual" scope sync to the native layer. After finishing a transaction the PropagationContext get renewed to keep new events from having an old trace context.
The SDK syncs the trace in immediate succession, overwriting itself every time.

Context

The logs are taken from a Unity game but should apply to anywhere there is a transaction that gets finished

Sentry: (Info) Finishing 'app.start' transaction. 
Sentry: (Debug) Attempting to finish Transaction d84ad5b1214e4d44. 
Sentry: (Debug) Finished Transaction d84ad5b1214e4d44. 
Sentry: (Debug) macOS Scope Sync - Setting Trace traceId:d968c8c94a7241e8b9873018d1302e4a spanId:5eba0d6fe7261f58 
Sentry: (Debug) macOS Scope Sync - Setting Trace traceId:bd2de9eb0db946819f59251e313c6471 spanId:e0b7994a40178a9c 

After a transaction gets finished it gets reset on the Scope and afterwards the PropagationContext gets regenerated so that new events don't have a trace context older than the just finished transaction. We even point this out in the comments.

There are two places the transaction gets reset on the scope:

• UnsampledTransaction

  sentry-dotnet/src/Sentry/Internal/UnsampledTransaction.cs

  Lines 65 to 71 in 6eedb71

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

• TransactionTracer

  sentry-dotnet/src/Sentry/TransactionTracer.cs

  Lines 386 to 392 in 6eedb71

  	// 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(static (scope, transactionTracer) =>
  	{
  	scope.ResetTransaction(transactionTracer);
  	scope.SetPropagationContext(new SentryPropagationContext());
  	}, this);

Proposal

The SDK does not need to manually restore the trace context on the native layer, as it gets overwritten basically in the next line.

[jamescrosswell]

Nice - thanks @bitsandfoxes !

My only question is about how we want to approach the fix. What you've done will work... it does depend on us always doing this:

            scope.ResetTransaction(transactionTracer);
            scope.SetPropagationContext(new SentryPropagationContext());

What's more, if there's a race and this line returns false:

sentry-dotnet/src/Sentry/Scope.cs

Line 818 in 34d8490

if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction))

Then we could end up running this multiple times:

scope.SetPropagationContext(new SentryPropagationContext());

Alternative solution

We could possibly rewrite ResetTransaction to do this:

            if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction))
            {
                _transaction.Value = null;
                SetPropagationContext(new SentryPropagationContext());
            }

So both the scope sync and the resetting of the propagation context always happen if you call RestTransaction (but only once, since it's in a lock that will prevent races).

[cursor]

Bug: Trace Context Sync Issue in ResetTransaction

Removing the scope synchronization from ResetTransaction creates an implicit dependency on the calling code to synchronize the native layer's trace context. Without a subsequent call to SetPropagationContext, the native layer can become out of sync with the managed scope, potentially leading to incorrect trace context in native-generated events.

src/Sentry/Scope.cs#L812-L827

sentry-dotnet/src/Sentry/Scope.cs

Lines 812 to 827 in 7336a87

	internal void ResetTransaction(ITransactionTracer? expectedCurrentTransaction)
	{
	_transactionLock.EnterWriteLock();
	try
	{
	if (ReferenceEquals(_transaction.Value, expectedCurrentTransaction))
	{
	_transaction.Value = null;
	}
	}
	finally
	{
	_transactionLock.ExitWriteLock();
	}
	}

 

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.40%. Comparing base (4a2bf5f) to head (7336a87).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4479      +/-   ##
==========================================
+ Coverage   73.38%   73.40%   +0.01%     
==========================================
  Files         479      479              
  Lines       17506    17504       -2     
  Branches     3479     3477       -2     
==========================================
+ Hits        12846    12848       +2     
+ Misses       3780     3778       -2     
+ Partials      880      878       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.