feat(spx-gui): report SPX runtime panics and lifecycle spans to Sentry - #3383
feat(spx-gui): report SPX runtime panics and lifecycle spans to Sentry#33831034674309 wants to merge 3 commits into
Conversation
Add Sentry integration to ProjectRunner to capture and report runtime errors from tools/ispx with structured context including file, line, function, and column information. Key features: - Parse panic JSON logs from tools/ispx console output - Report runtime panics to Sentry with SpxRuntimePanic exception type - Add structured context (file, line, column, function) to events - Instrument lifecycle spans for engine init, build, start, and stop - Deduplicate errors within 5-second window to prevent duplicate reports - Graceful degradation when Sentry is unavailable - Async reporting to avoid interfering with WASM panic unwinding Closes goplus#2419 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1034674309
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
This change adds Sentry tracing spans around SPX runner lifecycle operations and structured SPX runtime-panic reporting. The overall structure is clean, parseRuntimePanic is strict and defensive at the iframe trust boundary, and the WASM-unwinding deferral (setTimeout(0)) is a subtle correctness detail handled well. The four review passes surfaced a few concrete concerns worth addressing, noted inline. No blocking issues; submitting as a non-approving comment review.
Highlights:
- Performance:
JSON.parseruns on every single-stringconsole.logfrom the game loop — worth a cheap prefilter. - Correctness: grace-period (1000ms) vs recent-panic window (5000ms) mismatch, and panic state is not reset on
stop(), so a stale panic can suppress a legitimate error in a subsequent run. - Security (low): attacker-controlled panic strings reach Sentry unbounded.
|
|
||
| let value: unknown | ||
| try { | ||
| value = JSON.parse(args[0]) |
There was a problem hiding this comment.
Performance: parseRuntimePanic runs on every single-string console.log forwarded from the game iframe, and the only guard before JSON.parse is the arity/typeof check on line 244. A game doing console.log("score: 42") once per frame will trigger a JSON.parse + throw/catch at up to 60x/sec. Consider a cheap prefilter before parsing, e.g. skip unless the string starts with { and contains "panic" — the payload requires record.msg === 'panic' anyway, so this is behavior-preserving.
|
|
||
| function hasRecentRuntimePanic() { | ||
| const panic = lastRuntimePanic | ||
| return panic != null && Date.now() - lastRuntimePanicAt <= 5000 |
There was a problem hiding this comment.
Magic-number mismatch / correctness: this bare 5000 recent-panic window is inconsistent with runtimePanicGracePeriod = 1000 (line 185). The deferral only waits 1s for a future panic, but a panic that arrived up to 5s earlier suppresses the generic capture. Recommend extracting both into named constants and documenting why they differ (or unifying them). A raw literal here is easy to drift out of sync with the grace period.
| } | ||
|
|
||
| function deferRuntimeErrorCapture(err: unknown, ctx: string) { | ||
| if (hasRecentRuntimePanic()) return |
There was a problem hiding this comment.
Cross-run stale-panic suppression: lastRuntimePanic/lastRuntimePanicAt are reset only at the start of runInternal (lines 429-430), not in stop(). During rerun() (which calls stop() then run()), and for up to 5s after any run ends, hasRecentRuntimePanic() can still return true from a previous run's panic. That would cause this early return to silently drop a legitimately different error from the new run. Consider resetting panic state on stop() as well, or keying "recent" to the current run/AbortController.
| line, | ||
| column | ||
| }) | ||
| Sentry.captureException(exception) |
There was a problem hiding this comment.
Security (low) — unbounded attacker-controlled data to Sentry: error, functionName, and file originate from the iframe's console.log (user/game WASM) and are forwarded verbatim into Sentry as the exception message, name, and context with no length cap or sanitization. A malicious project can emit arbitrary/large panic strings (log-poisoning) and could embed runtime-harvested data into your Sentry stream. Consider truncating these strings to a bounded length and confirming Sentry PII scrubbing covers this path.
| // eslint-disable-next-line no-console | ||
| console.log(...args) | ||
| const panic = parseRuntimePanic(args) | ||
| if (panic != null) handleRuntimePanic(panic.error, panic.functionName, panic.file, panic.line, panic.column) |
There was a problem hiding this comment.
Minor: parseRuntimePanic already returns a RuntimePanic object, but it's destructured into 5 positional args here only for handleRuntimePanic to immediately reassemble the identical object (line 215). Passing panic directly (handleRuntimePanic(panic)) is simpler and removes a class of positional-arg ordering bugs.
| }) | ||
| try { | ||
| const result = await operation() | ||
| span.setStatus({ code: 1 }) |
There was a problem hiding this comment.
Minor (readability): span.setStatus({ code: 1 }) / { code: 2 } use bare numeric codes for OK/ERROR. A short comment or named constants (// 1 = OK, 2 = ERROR) would match how logLevels is spelled out elsewhere in this file.
1 participant
Summary
Closes #2419 by connecting the structured panic logs already emitted by
tools/ispxto the existing browser-side Sentry client.This PR intentionally does not add a separate Go/WASM Sentry SDK or configuration.
tools/ispxemits a location-rich JSON panic record to its console;ProjectRunnerrecognizes that record and reports it through the app's existing@sentry/vuesetup.Runtime panic flow
tools/ispxemits a JSON console record containingerror,function,file,line, andcolumn.ProjectRunnervalidates and parses that record from the runner iframe.SpxRuntimePanicevent to Sentry with:spx.runtime=panicspx.function,spx.file,spx.line, andspx.columnThis lets Sentry point to the SPX source location instead of only showing a generic game error.
Changes
msg: "panic"JSON logs emitted by the WASM runner.SpxRuntimePanicexception with structured SPX source context.spx.engine.initspx.buildspx.startspx.stopDuplicate-event handling
A single runtime panic can also produce the runner's existing generic
onGameErrororonEngineCrashcallback.Those generic captures are now deferred for one second. If the structured panic log arrives during that grace period, the pending generic capture is cancelled, leaving the source-aware
SpxRuntimePanicas the single Sentry error event. If no panic log arrives, the normal error is still reported.Verification
pnpm type-checkpnpm lintpnpm format-check./build-wasm.shpnpm buildgit diff --checkSpxRuntimePanicSentry event withNiuXiaoQi.spx, line/column, function name,environment=production, andspx.runtime=panic.Scope
No backend APIs, user-code semantics, project save/publish flow, or editor error UI are changed.
Closes #2419