fix(selfhost): move Sentry init before emptyConfigDirAdvisory so it can alert too - #6359
Merged
Merged
Conversation
…an alert too Follow-up to #6325, which fixed sqliteBackupAdvisory and publicOriginReachabilityAdvisory the same way -- both switched from console.warn to console.error, since installStructuredLogForwarding only ever intercepts console.log (level:error/fatal only) and console.error (always forwarded), never console.warn. emptyConfigDirAdvisory has the identical bug, but the same one-line swap wasn't enough for it: it fires before initSentry runs, so the forwarding hook doesn't exist yet at that point in the boot sequence -- a console.error there would have reached nothing regardless. Moves the Sentry/OpenTelemetry init block to run immediately after loadFileSecrets()/assertSelfHostPreflight() (before every boot-time advisory, not just this one), then applies the same console.warn -> console.error swap to emptyConfigDirAdvisory. Kept right after those two specifically: a self-host SENTRY_DSN is commonly supplied via a mounted secret file loadFileSecrets() reads into process.env, and preflight is a fatal-exit gate that should run before anything else regardless of Sentry's own state. Everything else that used to sit between the old and new positions (metrics mode flag, the LOOPOVER_REPO_CONFIG_DIR manifest/ review-context readers, the selfhost_config_dir info log) has no ordering dependency on Sentry either way -- confirmed by reading through all of it, and that info log in particular has no `level` field, so the forwarder would ignore it even with the hook active.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6359 +/- ##
=======================================
Coverage 95.60% 95.60%
=======================================
Files 598 598
Lines 47201 47201
Branches 15022 15022
=======================================
Hits 45127 45127
Misses 1290 1290
Partials 784 784
Flags with carried forward coverage won't be shown. Click here to find out more. |
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
Follow-up to #6325 (#6350), which fixed
sqliteBackupAdvisoryandpublicOriginReachabilityAdvisorythe same way — both switched fromconsole.warntoconsole.error, sinceinstallStructuredLogForwardingonly ever interceptsconsole.log(with an explicitlevel:error/fatal) andconsole.error(always forwarded), neverconsole.warn.emptyConfigDirAdvisoryhas the identical bug, but the same one-line swap wasn't enough for it: it fires beforeinitSentryruns, so the forwarding hook doesn't exist yet at that point in the boot sequence — aconsole.errorthere would have reached nothing regardless.loadFileSecrets()/assertSelfHostPreflight()— before every boot-time advisory, not just this one — then applies the sameconsole.warn→console.errorswap toemptyConfigDirAdvisory.SENTRY_DSNis commonly supplied via a mounted secret fileloadFileSecrets()reads intoprocess.env, and preflight is a fatal-exit gate that should run before anything else regardless of Sentry's own state.LOOPOVER_REPO_CONFIG_DIRmanifest/review-context readers, theselfhost_config_dirinfo log) has no ordering dependency on Sentry either way — confirmed by reading through all of it. The info log in particular has nolevelfield, so the forwarder would ignore it even with the hook now active earlier.Advances #6325 (doesn't close it — the live
PUBLIC_API_ORIGINmisconfiguration on the self-hosted instance is still the remaining piece, pending your infra decision).Test plan
test/unit/selfhost-health.test.ts(the pureemptyConfigDirAdvisoryfunction tests, untouched by this wiring-only change) — 36 tests pass unchangedtest/unit/docs-selfhost-sentry-observability.test.ts— 4 tests pass unchangedsrc/server.tsis Codecov-exempt (seecodecov.yml'signore:list), so this doesn't need new coveragebuild + boot smoke testCI workflow (.github/workflows/selfhost.yml) actually boots the real container and checks/health,/ready,/metrics, and the migration log line — none of which depend on this reorder, andSENTRY_DSNisn't set in that smoke test's env anyway, soinitSentryno-ops there regardless of positionnpm run typecheckcleannpm run test:cigate green