Skip to content

fix(P13): route transient Segment send-loop errors off Sentry#9420

Closed
eordano wants to merge 1 commit into
devfrom
chore/clean-segment-noise
Closed

fix(P13): route transient Segment send-loop errors off Sentry#9420
eordano wants to merge 1 commit into
devfrom
chore/clean-segment-noise

Conversation

@eordano

@eordano eordano commented Jul 17, 2026

Copy link
Copy Markdown
Member

UNITY-EXPLORER-P13 (637 evts/242 users — widest reach): "Segment error: Error executing send loop (will retry): ClientError {...}". The Rust Segment SDK's background send loop reports transient, self-recovering upload failures via ErrorCallback, which called ReportHub.LogException → Sentry (error level). The existing once-per-session guard still emitted one Sentry error per user. Route "(will retry)" messages to LogWarning on ReportHandler.DebugLog (local log, never Sentry); genuine non-transient Segment errors keep Sentry reporting. Removed the now-unused ONCE_PATTERN_ALREADY_CAUGHT field. Report-noise reduction. Unverified.

fixes #8574


Supersedes #9406: moved from the fork into the org repo so CI workflows receive repository secrets (fork PRs do not).

UNITY-EXPLORER-P13 (637 evts/242 users — widest reach): "Segment error: Error
executing send loop (will retry): ClientError {...}". The Rust Segment SDK's
background send loop reports transient, self-recovering upload failures via
ErrorCallback, which called ReportHub.LogException → Sentry (error level). The
existing once-per-session guard still emitted one Sentry error per user. Route
"(will retry)" messages to LogWarning on ReportHandler.DebugLog (local log, never
Sentry); genuine non-transient Segment errors keep Sentry reporting. Removed the
now-unused ONCE_PATTERN_ALREADY_CAUGHT field. Report-noise reduction. Unverified.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



Production evidence (decentraland Sentry, archive snapshot 2026-07-17):
- UNITY-EXPLORER-P13: 29 events / 15 users, last seen 2026-07-16
- UNITY-EXPLORER-JVR: 8 events / 1 users, last seen 2026-02-22
@eordano
eordano requested review from a team as code owners July 17, 2026 13:35
@github-actions
github-actions Bot requested a review from DafGreco July 17, 2026 13:35
@decentraland-bot decentraland-bot added the ext-contribution Identifies a contribution which was not initiated by a Unity Developer label Jul 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

badge

New build in progress, come back later!

@github-actions

Copy link
Copy Markdown
Contributor

Slack notification sent to #explorer-ext-contributions for external review.
To re-send, delete this comment and re-add the ext-contribution label.

@claude

claude Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Claude finished @eordano's task in 1m 18s —— View job


Review: PR #9420 — route transient Segment send-loop errors off Sentry

  • Read PR diff and changed file
  • Review code correctness & standards
  • Security/dependency check (labels: new-dependency, ext-contribution)
  • Post review

Verdict: LGTM. The change does exactly what it says and is a clean, targeted noise reduction. No blocking issues.

Behavioral change (correct)

Before After
First (will retry) msg LogExceptionSentry (1 error/session/user) LogWarningDebugLog only, never Sentry
Subsequent (will retry) silently dropped LogWarning → DebugLog only
Non-transient errors LogException → Sentry unchanged (still Sentry)

Verified against the source:

  • ReportHub.LogWarning(ReportData, object, ReportHandler) exists (ReportHub.cs:57) and ReportCategory.ANALYTICS converts to ReportData (same conversion the untouched LogException call on line 244 relies on). ✅
  • ReportHandler.DebugLog = 1 and All = DebugLog | Sentry (ReportHandler.cs:16,26), so passing DebugLog genuinely bypasses the Sentry handler. ✅
  • Removing the now-unused ONCE_PATTERN_ALREADY_CAUGHT static field and its typo'd comment is correct dead-code cleanup — no other references. ✅

Minor observations (non-blocking, optional)

  1. Log volume trade-off. The old code suppressed all but the first (will retry) message; the new code writes a local warning for every transient failure. That's fine (DebugLog is cheap and never leaves the machine, and it's arguably better for local diagnostics), but if the Rust send loop retries tightly during an outage this will be chattier in the local log. Acceptable given the Sentry-noise goal.
  2. Stale identifier names. ONCE_PATTERN / isCaught were named for the old "log once" semantics, which no longer exist — the pattern now simply classifies transient vs. genuine errors. A rename like TRANSIENT_PATTERN / isTransient would read truer, but it's cosmetic.

Security / dependency review (per new-dependency + ext-contribution labels)

Despite the labels, this diff contains no actual dependency, binary, or workflow changes — it is a single .cs edit (+6/-10) to RustSegmentAnalyticsService.cs.

  • No changes to Packages/manifest.json or packages-lock.json.
  • No new/modified native or managed binaries under Assets/** (pre-computed binary list: None).
  • No .asmdef / build-script / editor-hook changes.
  • No .github/workflows/ or .github/prompts/ changes (pre-computed: None).

No new runtime capability, network, filesystem, or reflection surface is introduced — the code strictly reduces what gets reported externally.

DEPENDENCY_REVIEW: PASS

@eordano

eordano commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

Closed in favor of the compound PR #9430 (team decision — all fixes in this set land and iterate there; each fix remains individually reviewable as its own merge commit in #9430).

@eordano eordano closed this Jul 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ext-contribution Identifies a contribution which was not initiated by a Unity Developer new-dependency

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Exception: Segment error: Error executing send loop (will retry): ClientError {

2 participants