Skip to content

fix(sentry): discard timed-out onboarding traces instead of sending fake ~5-min durations#33318

Open
smgv wants to merge 4 commits into
mainfrom
fix/sentry-discard-timed-out-onboarding-traces
Open

fix(sentry): discard timed-out onboarding traces instead of sending fake ~5-min durations#33318
smgv wants to merge 4 commits into
mainfrom
fix/sentry-discard-timed-out-onboarding-traces

Conversation

@smgv

@smgv smgv commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Description

When a span is still open after TRACES_CLEANUP_INTERVAL (5 min), the cleanup timeout now marks it with SpanStatus ERROR / deadline_exceeded and sets trace.timed_out = true before calling end(). The excludeEvents beforeSendTransaction hook drops any event carrying that flag, so artificially-clamped spans (p50 ≈ 300,014 ms for "Onboarding - Overall Journey") are never sent to Sentry. Legitimate long-but-real spans are unaffected because the drop is keyed strictly on the flag, not on duration.

JIRA Link: https://consensyssoftware.atlassian.net/browse/TO-898

Changelog

CHANGELOG entry: discard timed-out onboarding traces instead of sending fake ~5-min durations

Related issues

Fixes:

Manual testing steps

Feature: my feature name

  Scenario: user [verb for user action]
    Given [describe expected initial app state]

    When user [verb for user action]
    Then [describe expected outcome]

Screenshots/Recordings

Before

Screenshot 2026-07-15 at 1 36 07 PM

After

  1. Unit tests pass. trace.test.ts proves that when the cleanup timer fires, the span is marked deadline_exceeded + trace.timed_out. utils.test.ts proves excludeEvents drops any event carrying that flag. These test the exact changed lines.
  2. Code logic is airtight. excludeEvents only drops an event in two cases: it's a "Route Change", or it has trace.timed_out === true. The journey span isn't a Route Change — so the only way it gets dropped is via the flag our fix sets. A dropped journey = a timed-out one, by construction.
  3. Sentry baseline (before fix): Onboarding - Overall Journey p50 = 300,014 ms (≈ the 5-min timeout), max = 50.8 hours. These are the fake values the fix removes.

Nothing to "see" in Sentry

The fix works by dropping the bad trace before it's ever sent to Sentry. A dropped trace leaves no record — so a working fix shows up as the bad ~5-minute spans disappearing, not as a new event. There is nothing to screenshot; success = absence.

The only Sentry signal is therefore negative and only meaningful at scale: after release, Onboarding - Overall Journey p50 should drop away from ~300,000 ms. That's the post-ship confirmation (production dashboard), not something visible from one test run.

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Low Risk
Observability-only filtering keyed on an explicit timeout flag; no app logic, auth, or data paths change beyond what Sentry receives.

Overview
Fixes inflated onboarding journey durations (~5 min p50) when spans are never closed by not sending those transactions to Sentry.

When TRACES_CLEANUP_INTERVAL fires on an still-open manual span, cleanup now sets span status to deadline_exceeded and attribute trace.timed_out: true before ending the span. excludeEvents (beforeSendTransaction) returns null for any transaction whose trace context has trace.timed_out === true, same pattern as dropping Route Change. Real spans that end normally never get the flag and are unchanged.

Unit tests cover timeout cleanup marking the span and excludeEvents drop vs pass-through.

Reviewed by Cursor Bugbot for commit dee892a. Bugbot is set up for automated code reviews on this repo. Configure here.

…ake ~5-min durations

When a span is still open after TRACES_CLEANUP_INTERVAL (5 min), the
cleanup timeout now marks it with SpanStatus ERROR / deadline_exceeded
and sets `trace.timed_out = true` before calling end(). The
`excludeEvents` beforeSendTransaction hook drops any event carrying that
flag, so artificially-clamped spans (p50 ≈ 300,014 ms for
"Onboarding - Overall Journey") are never sent to Sentry. Legitimate
long-but-real spans are unaffected because the drop is keyed strictly
on the flag, not on duration.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamask-ci metamask-ci Bot added the team-onboarding Onboarding team label Jul 15, 2026
@smgv smgv self-assigned this Jul 15, 2026
@metamask-ci

metamask-ci Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Related issues section is empty. Add Fixes: #123 / Closes: <URL> / Refs: <Jira key>, or write a short rationale after the colon.
  • Manual testing steps still contain template content (the Gherkin example title or a [...] placeholder). Replace with real steps, or write N/A — <reason>.
  • Pre-merge author checklist has unchecked items (e.g. "I've documented my code using JSDoc format if applicable"). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@github-actions github-actions Bot added size-S risk:medium AI analysis: medium risk labels Jul 15, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

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, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 971548d. Configure here.

Comment thread app/util/sentry/utils.ts
@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 97%
click to see 🤖 AI reasoning details

E2E Test Selection:
The PR modifies two utility files related to Sentry error reporting and OpenTelemetry tracing:

  1. app/util/trace.ts: When a trace cleanup timeout fires (for long-running/abandoned traces), the span is now marked with deadline_exceeded status and a trace.timed_out=true attribute before being ended. This is purely internal telemetry behavior.

  2. app/util/sentry/utils.ts: The excludeEvents filter now drops Sentry events where trace.timed_out === true, preventing timed-out trace spans from being reported as errors to Sentry.

Both changes are purely observability/monitoring infrastructure changes:

  • No user-facing UI changes
  • No controller logic modifications
  • No navigation or transaction flow changes
  • No shared component modifications (TabBar, modals, confirmations, etc.)
  • No impact on any E2E-testable user flows

The accompanying test files (utils.test.ts, trace.test.ts) add unit tests for the new behavior, confirming the scope is limited to internal telemetry.

These changes cannot break any E2E test flows and have no performance impact. Zero E2E tags and zero performance tags are appropriate.

Performance Test Selection:
The changes only affect Sentry event filtering and trace span status marking on timeout. These are internal telemetry utilities with no impact on app performance characteristics, rendering times, or any user-facing flows measured by performance tests.

View GitHub Actions results

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/util/trace.test.ts 0/98 0/173 0/366

AI-detected flaky patterns

app/util/trace.test.ts

  • J2 — Real timers where fake timers are needed (high)
    • The nested describe for trace timeout cleanup (which contains the newly added test using jest.advanceTimersByTime to test the deadline_exceeded status and trace.timed_out attribute) sets up fake timers in beforeEach but uses an incorrect afterEach that reactivates fake timers (with legacy option) instead of restoring real timers. This can leak fake timer state into later tests like those in flushBufferedTraces describe (also modified in this PR), causing non-deterministic timing, hanging polls, or incorrect mock behavior under CI load. Matches J2 symptoms exactly; no historical flaky signal but the added test increases blast radius for timer-related races.
    • Suggested fix in app/util/trace.test.ts:
      -    beforeEach(() => {
      -      jest.useFakeTimers();
      -    });
      -
      -    afterEach(() => {
      -      jest.useFakeTimers({ legacyFakeTimers: true });
      -    });
      +    beforeEach(() => {
      +      jest.useFakeTimers();
      +    });
      +
      +    afterEach(() => {
      +      jest.useRealTimers();
      +    });

This check is informational only and does not block merging.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:medium AI analysis: medium risk size-S team-onboarding Onboarding team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant