Skip to content

fix(tracing): Unsubscribe spanEnd listeners after they fire to prevent accumulation#5840

Open
antonis wants to merge 4 commits intomainfrom
antonis/cleanup-listeners
Open

fix(tracing): Unsubscribe spanEnd listeners after they fire to prevent accumulation#5840
antonis wants to merge 4 commits intomainfrom
antonis/cleanup-listeners

Conversation

@antonis
Copy link
Contributor

@antonis antonis commented Mar 18, 2026

📢 Type of change

  • Bugfix

📜 Description

Each client.on('spanEnd') registered in onSpanEndUtils is a one-shot listener — it only cares about one specific span ending. Previously the unsubscribe function returned by client.on() was discarded, leaving listeners in the client's hook set indefinitely.

The fix captures and calls the unsubscribe function as the first action once the target span is identified, making every listener self-removing. All five functions in the file are updated: onThisSpanEnd, adjustTransactionDuration, discardEmptyNavigationSpan (shared by ignoreEmptyBackNavigation and ignoreEmptyRouteChangeTransactions), onlySampleIfChildSpans, and cancelInBackground.

💡 Motivation and Context

Over the lifetime of an app with frequent navigation, these listeners accumulated: each navigation span left 3–4 zombie listeners that fired on every subsequent spanEnd event system-wide and returned early via an identity check. On low-end devices this contributes measurable overhead, especially when many child spans (HTTP requests) end during active navigation transactions.

Related issue: #5665

💚 How did you test it?

Added a dedicated test file test/tracing/onSpanEndUtils.test.ts with 11 tests covering:

  • Correct callback behaviour for onThisSpanEnd
  • Unsubscribe is called for every function after its target span ends
  • AppState subscription is cleaned up in cancelInBackground
  • Regression test: listener count does not grow across multiple spans — after 5 spans are created and ended, ending a new span does not trigger any of the old callbacks

All existing reactnavigation tests (93) continue to pass.

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

Part of a broader investigation into reactNavigationIntegration performance on low-end devices (#5665). Other improvements being tracked in that issue.

…t accumulation

Each `client.on('spanEnd')` registered in `onSpanEndUtils` is a one-shot
listener — it cares about exactly one specific span ending. Previously the
unsubscribe function returned by `client.on()` was discarded, leaving all
listeners in the client's hook set indefinitely.

Over the lifetime of an app with frequent navigation, this caused listeners
to accumulate: each navigation span left 3–4 zombie listeners that fired on
every subsequent spanEnd event and returned early via an identity check. On
low-end devices this contributes measurable overhead, especially when many
child spans (HTTP requests) end during active navigation transactions.

The fix captures and calls the unsubscribe function as the first action once
the target span is identified, making every listener self-removing.

Also adds a dedicated test file for `onSpanEndUtils` covering unsubscribe
behaviour for all five functions, including a regression test that verifies
listener count does not grow across multiple spans.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2026

Semver Impact of This PR

None (no version bump detected)

📋 Changelog Preview

This is how your changes will appear in the changelog.
Entries from this PR are highlighted with a left border (blockquote style).


  • fix(tracing): Unsubscribe spanEnd listeners after they fire to prevent accumulation by antonis in #5840
  • fix(tracing): Recover app start data when first navigation transaction is discarded by antonis in #5833
  • chore(deps): update Android SDK to v8.36.0 by github-actions in #5812
  • Add expoUpdatesListenerIntegration that records breadcrumbs for Expo Updates lifecycle events by alwx in #5795
  • chore(deps): update Sentry Android Gradle Plugin to v6.2.0 by github-actions in #5836
  • fix(ci): Update Appium version to fix Sauce Labs metrics tests by antonis in #5835
  • chore(deps): update JavaScript SDK to v10.44.0 by github-actions in #5832
  • fix(tracing): Fix native frames measurements dropped for idle transactions by antonis in #5813
  • feat(core): Support SENTRY_ENVIRONMENT in bare React Native builds by antonis in #5823
  • chore(deps): bump tar to ^7.5.11 by antonis in #5824
  • chore(deps): bump actions/create-github-app-token from 2.2.1 to 3.0.0 by dependabot in #5822
  • chore(deps): bump dorny/paths-filter from 3.0.2 to 4.0.1 by dependabot in #5820
  • chore(deps): bump reactivecircus/android-emulator-runner from 2.35.0 to 2.37.0 by dependabot in #5818
  • chore(deps): bump getsentry/craft/.github/workflows/changelog-preview.yml from 2.23.2 to 2.24.1 by dependabot in #5821
  • chore(deps): bump getsentry/craft from 2.23.2 to 2.24.1 by dependabot in #5819
  • chore(deps): bump undici from 6.23.0 to 6.24.1 by dependabot in #5817
  • chore(deps): bump flatted from 3.3.1 to 3.4.1 by dependabot in #5816
  • Ref: remove yarn from stub update by lucas-zimerman in #5811
  • Ref(CI): Unify stub update with android update by lucas-zimerman in #5807

🤖 This preview updates automatically when you update the PR.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2026

Fails
🚫 Pull request is not ready for merge, please add the "ready-to-merge" label to the pull request

Generated by 🚫 dangerJS against b853a20

Copy link
Contributor Author

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Since this is more of an internal implementation fix with no user-facing behavior change I'll skip changelog. I would suggest to add a changelog entry when all the improvements of #5665 are delivered. Happy to add it if you feel it is needed.

@antonis
Copy link
Contributor Author

antonis commented Mar 18, 2026

@sentry review

@antonis
Copy link
Contributor Author

antonis commented Mar 18, 2026

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

@antonis antonis marked this pull request as ready for review March 18, 2026 15:02
antonis and others added 2 commits March 19, 2026 11:04
The NativeFrames and StallTracking integrations performed their full
work (native bridge calls, 50ms polling loop) regardless of whether the
span would be sampled, unlike the Profiling integration which already
checks spanIsSampled() correctly.

With tracesSampleRate: 0.2, this meant 80% of navigation spans triggered
unnecessary fetchNativeFrames() bridge calls and stall tracking loop
activations. On low-end devices these operations are significantly more
expensive and contribute to the performance overhead reported in #5665.

The fix adds a spanIsSampled() guard at the entry of fetchStartFramesForSpan
in NativeFrames and _onSpanStart in StallTracking. Since end-frame fetching
already bails when no start frames are found, the single guard in
fetchStartFramesForSpan covers both start and end for NativeFrames.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant