fix(otel): only set Scope.Transaction in SentrySpanProcessor when null by tsushanth · Pull Request #5337 · getsentry/sentry-dotnet

tsushanth · 2026-07-03T01:03:56Z

Problem

SentrySpanProcessor.OnStart calls ConfigureScope unconditionally:

_hub.ConfigureScope(static (scope, transaction) => scope.Transaction = transaction, transaction);

When two root OTel activities overlap and neither is a child of the other, this silently overwrites whatever transaction was already on the scope with the new one. As the issue notes, the intuitive default is to not overwrite an existing transaction.

Fix

Add a null-check guard inside the ConfigureScope lambda:

_hub.ConfigureScope(static (scope, transaction) =>
{
    if (scope.Transaction is null)
    {
        scope.Transaction = transaction;
    }
}, transaction);

Scope.Transaction is only set when it is currently null; an already-present transaction is left unchanged.

Test

Added OnStart_WithExistingTransactionOnScope_DoesNotOverwriteExistingTransaction to SentrySpanProcessorTests: sets a transaction on the scope, starts a new root activity, calls OnStart, and asserts the scope still holds the original transaction.

This is intentionally not bundled with #2399 (the AutoSetScopeTransactions opt-in), per the issue's notes.

Flash0ver · 2026-07-03T14:16:36Z

+        sut.OnStart(data!);
+
+        // Assert - the pre-existing transaction must still be on the scope
+        ITransaction? scopeTransaction = null;


issue: test was neither run nor built

there is no ITransaction type

this project does not have Nullable Reference Types enabled

Fixed — rewrote the test to use the real Hub instead of a mock IInternalScopeManager. The mock was eating ConfigureScope calls so the guard never ran. The test now captures the first transaction from sut._map and verifies scope.Transaction still points to it after the second OnStart.

Flash0ver · 2026-07-03T14:18:29Z

+    [Fact]
+    public void OnStart_WithExistingTransactionOnScope_DoesNotOverwriteExistingTransaction()


issue: test is ineffective

Currently, this test does not actually cover the modification, and does not cover the modified method SentrySpanProcessor.CreateRootSpan.

@jamescrosswell I wonder if Mutation Testing could cover such issues early ... e.g. if we only run mutation tests on the changes per PR, and only via the test cases that cover the /src/ changes of the PR.

That might be nice. It has to save us more effort than we spend maintaining CI to automate it. In the case of mutation tests that seems likely. Let's add an issue for prioritisation.

Run mutation tests for any new tests added by each PR in CI #5343

Addressed in the same rewrite — the test now goes through the real Hub's ConfigureScope, so the null guard in CreateRootSpan actually runs.

Flash0ver · 2026-07-03T14:23:10Z

+        // Arrange - simulate a root OTel activity starting while another transaction is
+        // already on the scope (e.g. two overlapping root activities). The processor should
+        // not overwrite the existing transaction (see GH-5314).


thought: comments don't add additional value

IMO, the comments in this test apart from Arrange/Assert/Assert don't really add any additional value:

Assert: the existing transaction is called "Existing"

Act: the New Root Activity that is started is called "NewRootActivity"

also: actually, Act is missing for the Act portion of the test

Assert: the because string parameter to the assertion describes

To me, the combination of Test-Name + Test-Logic + String-Parameters already describe what this test is covering and asserting.

Plus the test itself is called OnStart_WithExistingTransactionOnScope_DoesNotOverwriteExistingTransaction...

Removed the inline comments from the test.

Flash0ver · 2026-07-03T14:26:44Z

+        // Only set Scope.Transaction when it is currently null. Overwriting an existing
+        // transaction (which can happen when two root OTel activities overlap and neither is
+        // a child of the other) silently drops the previous transaction's scope context.
+        // See https://github.com/getsentry/sentry-dotnet/issues/5314


suggestion: shorten comment

Although this comment does add value ... I find it a bit lengthy.
Perhaps we can shorten it a bit: for example: Only set Scope.Transaction when it is currently null is just describing the C# syntax in use.

I think we could probably remove the comment entirely in this case... it's pretty apparent from the code what's going on.

Flash0ver · 2026-07-03T14:32:08Z

+        // Only set Scope.Transaction when it is currently null. Overwriting an existing
+        // transaction (which can happen when two root OTel activities overlap and neither is
+        // a child of the other) silently drops the previous transaction's scope context.
+        // See https://github.com/getsentry/sentry-dotnet/issues/5314


question: do we want to add links to issues more aggressively? @jamescrosswell

Previously, we only added links to the respective GitHub issue, when the code isn't too obvious what it's trying to achieve in a rather complex flow ... or when the solution in place should be reviewed/revisited at some point in time when it is a workaround.

But it seems that Coding Agents like to add both a summary from the GitHub issue, as well as a link to the GitHub issue, and additionally describe the code semantics ahead.

In this case, I would prefer to either have a one-liner, or two/three words + link to the GitHub issue ... but both - to me - is a bit lengthy. To be fair, I might exaggerate "the problem" here a bit. What do you think?

We get this for free from git blame (will identify the specific commit - relatively trivial to find the PR from there with a squash and merge as we're doing). When the code is particularly unintuitive and a full explanation in code comments isn't practical, it's worth linking to a PR description that provides a full explanation (make it clear we expect people will have to read the PR description to fully 'get it' in those cases).

For this piece of code, I don't think it's worth it though. We just don't want to overwrite previous/manually set values on the scope, which is obvious from the code.

Agreed, removed.

Flash0ver · 2026-07-03T14:36:37Z

Thank you very much, @tsushanth, for your contribution to the Sentry .NET SDK!
I left some comments about the test-case, and commented on the comments 😉

jamescrosswell · 2026-07-05T22:17:29Z

+            if (scope.Transaction is null)
+            {
+                scope.Transaction = transaction;
+            }


Suggested change

if (scope.Transaction is null)

{

scope.Transaction = transaction;

}

scope.Transaction ??= transaction;

Thanks @tsushanth - did you forget to push your changes to remote maybe? I don't see them here...

Rewrite test to exercise CreateRootSpan directly via ScopeManager, use correct types, and remove noisy inline comments per review feedback.

tsushanth · 2026-07-06T14:30:15Z

Thanks for the detailed review @Flash0ver! Updated:

Source comment: shortened to a two-liner with issue link
Test: rewritten to use ScopeManager/Scope directly (same pattern as OnStart_SampledWithoutParentSpanId_StartsNewTransaction), so it actually exercises CreateRootSpan. First activity sets scope.Transaction via the processor; second root activity is the act; assertion checks scope.Transaction is still the first
Types: removed ITransaction (doesn't exist here); scope.Transaction is tested via BeSameAs on the captured reference
Inline comments: removed the narrative comments, kept only Arrange/Act/Assert markers

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 8a20eba. Configure here.}

codecov · 2026-07-07T05:15:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.20%. Comparing base (04a9854) to head (c9f0b84).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5337      +/-   ##
==========================================
+ Coverage   74.16%   74.20%   +0.04%     
==========================================
  Files         508      509       +1     
  Lines       18353    18386      +33     
  Branches     3586     3600      +14     
==========================================
+ Hits        13612    13644      +32     
  Misses       3869     3869              
- Partials      872      873       +1

☔ View full report in Codecov by Harness.
📢 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.

tsushanth · 2026-07-07T19:33:20Z

Sorry about the delay — was pushing to the wrong local branch name. Commits are now on the correct branch: ??= operator in the source, and the test rewritten to use the real Hub (not a mock IInternalScopeManager) so ConfigureScope actually executes the null guard.

Flash0ver · 2026-07-07T21:50:10Z

thanks ... I'll take another look in the morning (CEST)

tsushanth requested review from Flash0ver and jamescrosswell as code owners July 3, 2026 01:03

github-actions Bot added the risk: low PR risk score: low label Jul 3, 2026

cursor Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs

sentry Bot reviewed Jul 3, 2026

View reviewed changes

Comment thread src/Sentry.OpenTelemetry/SentrySpanProcessor.cs

Flash0ver requested changes Jul 3, 2026

View reviewed changes

jamescrosswell reviewed Jul 5, 2026

View reviewed changes

fix(otel): only set Scope.Transaction in CreateRootSpan when null

8a20eba

Rewrite test to exercise CreateRootSpan directly via ScopeManager, use correct types, and remove noisy inline comments per review feedback.

tsushanth force-pushed the fix/span-processor-overwrite-transaction branch from 8a3f87c to 8a20eba Compare July 6, 2026 14:30

cursor Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs

fix(otel): use ??= operator and fix test to exercise the actual guard

c9f0b84

		[Fact]
		public void OnStart_WithExistingTransactionOnScope_DoesNotOverwriteExistingTransaction()

Uh oh!

Uh oh!

Conversation

tsushanth commented Jul 3, 2026

Problem

Fix

Test

Uh oh!

Uh oh!

Uh oh!

Flash0ver Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flash0ver Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamescrosswell Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flash0ver commented Jul 3, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsushanth commented Jul 6, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsushanth commented Jul 7, 2026

Uh oh!

Flash0ver commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Flash0ver Jul 3, 2026 •

edited

Loading

Flash0ver Jul 3, 2026 •

edited

Loading

jamescrosswell Jul 5, 2026 •

edited

Loading

codecov Bot commented Jul 7, 2026 •

edited

Loading