Skip to content

fix: Add comprehensive Sentry logging for flowsheet mirror behavior#243

Open
JacksonMeade wants to merge 5 commits intomainfrom
fix/better-mirror-logs
Open

fix: Add comprehensive Sentry logging for flowsheet mirror behavior#243
JacksonMeade wants to merge 5 commits intomainfrom
fix/better-mirror-logs

Conversation

@JacksonMeade
Copy link
Copy Markdown
Contributor

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)

  • One place for mirror telemetry – Helper code centralizes Sentry breadcrumbs, exception capture, truncation, and SQL “fingerprints” (length + hash) so behavior stays consistent.
  • Tie logs to the HTTP request – Middleware passes context into the queue: operation name, correlation id, show/DJ when available, route/method/status. That survives after the response finishes.
  • Explicit branches, not guesswork – We log when mirror work is skipped (bad status, no body to mirror, PostHog flag off) vs. when work is queued vs. when the queue is dead.
  • Queue lifecycle is visible – Enqueue, start, success, failed attempt, planned retry, and fatal each emit breadcrumbs; fatals also send a Sentry error with attempt counts.
  • Disk reports are safe on EC2 – Secondary and fatal reports use rotating filenames and capped JSON; if a payload would be huge, we write a smaller summary instead. We do not persist full raw SQL in those files.

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

  • Sentry and X-Request-Id behavior are unchanged at the app level; mirror events now show up with richer tags and breadcrumbs.
  • Optional env vars tune report rotation and max JSON size for secondary/fatal files; defaults aim for a small, fixed footprint on disk.
  • No change required for clients unless you were parsing mirror SSE payloads specifically for raw SQL (reports on disk intentionally avoid full SQL).

@JacksonMeade JacksonMeade self-assigned this 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({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where is sqlPreview used?

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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 };                                 

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants