fix: Add comprehensive Sentry logging for flowsheet mirror behavior#243
Open
JacksonMeade wants to merge 5 commits intomainfrom
Open
fix: Add comprehensive Sentry logging for flowsheet mirror behavior#243JacksonMeade wants to merge 5 commits intomainfrom
JacksonMeade wants to merge 5 commits intomainfrom
Conversation
jakebromberg
requested changes
Mar 20, 2026
| const maxSecondaryReports = parseInt(process.env.MIRROR_SECONDARY_REPORTS_MAX ?? '10', 10); | ||
| const secondaryIntervalMs = parseInt(process.env.MIRROR_SECONDARY_REPORTS_INTERVAL_MS ?? String(10 * 60 * 1000), 10); | ||
| if (cmd.attempts === secondaryAttempt && maxSecondaryReports > 0) { | ||
| void this.persistSecondaryReport({ |
Member
There was a problem hiding this comment.
If this.persistSecondaryReport throws an error, the catch block isn't hit. Should we await this?
| return { | ||
| sqlLength: sql.length, | ||
| sqlHash: hashSha256Hex(sql), | ||
| sqlPreview: truncateString(sql, MAX_SQL_PREVIEW_LEN), |
Comment on lines
+93
to
+109
| if ('cmd' in payload) { | ||
| const ctx = payload.cmd.context ?? {}; | ||
| addMirrorBreadcrumb( | ||
| 'Mirror sync: retry/failure attempt', | ||
| { | ||
| eventType, | ||
| mirror_cmd_id: payload.cmd.id, | ||
| attempt: payload.attempt, | ||
| last_error: truncateForMirrorPayload(payload.error?.message, 256), | ||
| }, | ||
| ctx, | ||
| payload.attempt >= 2 ? 'warning' : 'info' | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if ('id' in payload && 'status' in payload) { |
Member
There was a problem hiding this comment.
This works today because the shapes (MirrorCommand, { cmd, error, attempt }, FatalInfo) happen to have distinct keys. But if we add an id to FatalInfo or a cmd to MirrorCommand, the wrong branch runs silently with no compiler warning.
You might want to think about using a discriminated union:
type TriggerPayload =
| { kind: 'command'; command: MirrorCommand }
| { kind: 'retry'; cmd: MirrorCommand; error: Error; attempt: number }
| { kind: 'fatal'; info: FatalInfo };
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens legacy flowsheet mirror observability by introducing structured mirror logging, Sentry integration, request-level correlation context, and bounded EC2-safe report persistence.
Why
Before this work, mirror failures were hard to debug: logs were sparse, the queue detached from the original HTTP request, and fatal dumps could grow large on disk. Mirror runs against legacy SQL over SSH, so when something breaks we need clear signals (what operation, which request, what step failed, retries vs. final failure) without filling the instance with huge files.
What we changed (implementation)
Tests
We added focused unit tests so we can trust the above: Sentry helper behavior (tags, breadcrumbs, no crash if Sentry acts up), middleware paths (skip vs. enqueue vs. errors vs. dead queue), and queue behavior (success, retry, fatal, ring file names, size cap, SQL summary without leaking the full statement). Tests mock I/O and Sentry so they stay fast and deterministic.
Ops / rollout notes
X-Request-Idbehavior are unchanged at the app level; mirror events now show up with richer tags and breadcrumbs.