Skip to content

fix(otel): only set Scope.Transaction in SentrySpanProcessor when null#5337

Open
tsushanth wants to merge 3 commits into
getsentry:mainfrom
tsushanth:fix/span-processor-overwrite-transaction
Open

fix(otel): only set Scope.Transaction in SentrySpanProcessor when null#5337
tsushanth wants to merge 3 commits into
getsentry:mainfrom
tsushanth:fix/span-processor-overwrite-transaction

Conversation

@tsushanth

Copy link
Copy Markdown

Fixes #5314.

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.

@github-actions github-actions Bot added the risk: low PR risk score: low label Jul 3, 2026
Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs
Comment thread src/Sentry.OpenTelemetry/SentrySpanProcessor.cs
Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs Outdated
Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs
Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs Outdated
Comment thread src/Sentry.OpenTelemetry/SentrySpanProcessor.cs Outdated
Comment thread src/Sentry.OpenTelemetry/SentrySpanProcessor.cs Outdated
@Flash0ver

Copy link
Copy Markdown
Member

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 😉

Comment thread src/Sentry.OpenTelemetry/SentrySpanProcessor.cs Outdated
Rewrite test to exercise CreateRootSpan directly via ScopeManager,
use correct types, and remove noisy inline comments per review feedback.
@tsushanth tsushanth force-pushed the fix/span-processor-overwrite-transaction branch from 8a3f87c to 8a20eba Compare July 6, 2026 14:30
@tsushanth

Copy link
Copy Markdown
Author

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 Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Fix All in Cursor

❌ 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.

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

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5337      +/-   ##
==========================================
+ Coverage   74.16%   74.20%   +0.03%     
==========================================
  Files         508      509       +1     
  Lines       18353    18389      +36     
  Branches     3586     3601      +15     
==========================================
+ Hits        13612    13645      +33     
- Misses       3869     3870       +1     
- Partials      872      874       +2     

☔ 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

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Member

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

@Flash0ver Flash0ver self-assigned this Jul 8, 2026

@Flash0ver Flash0ver left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One final suggestion ... to make the test actually cover the branch where scope.Transaction is already set AND _hub.ConfigureScope is invoked.
(I took the liberty to apply the change and re-run the checks/workflows)

Comment thread test/Sentry.OpenTelemetry.Tests/SentrySpanProcessorTests.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: low PR risk score: low

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenTelemetry SpanProcessor should only set Scope.Transaction when it is null

3 participants