feat: Sentry error monitoring integration (#38)#91
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntegrates Sentry error monitoring throughout the application by introducing a new Sentry initialization module, Winston transport for log forwarding, and source attribution to errors across database, Discord client, and slash command handlers. Includes configuration, dependencies, and comprehensive tests. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
BillChirico
force-pushed
the
feat/warning-system
branch
from
adb6f29 to
922660f
Compare
BillChirico
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@migrations/002_alerts-table.cjs`:
- Around line 33-35: The current non-unique index idx_alerts_fingerprint on
alerts(fingerprint) allows concurrent inserts of identical unresolved alerts;
replace it with a partial unique index enforcing uniqueness only for unresolved
rows (e.g., create a UNIQUE INDEX (name it something like
idx_alerts_fingerprint_unresolved_unique) on alerts(fingerprint) WHERE
resolved_at IS NULL) so duplicate unresolved fingerprints are prevented; update
the migration to drop the old non-unique idx_alerts_fingerprint (or include IF
EXISTS) before creating the partial unique index and ensure the index name and
predicate reference the alerts table, fingerprint column, and resolved_at
column.
- Around line 16-21: The migration currently defines severity and state as
free-form TEXT (columns severity and state in the alerts table); add DB-level
constraints by either creating ENUM types or adding CHECK constraints to
restrict allowed values (e.g., severity IN ('low','medium','high') and state IN
('new','acknowledged','resolved')), update the column definitions in
migrations/002_alerts-table.cjs to use those constraints (or use CREATE TYPE ...
AS ENUM then ALTER TABLE to use the enum) and preserve the existing default
'new' for state; ensure the migration includes the corresponding down/revert
steps to drop the enum or constraints when rolling back.
In `@src/services/alertDelivery.js`:
- Around line 102-103: Replace the direct call to user.send(messageOptions) with
the project's safeSend wrapper to ensure mention-sanitization and
allowedMentions enforcement: locate the try block in
src/services/alertDelivery.js where user.send(messageOptions) is invoked and
change it to call safeSend(user, messageOptions) (or the appropriate safeSend
signature used elsewhere), importing/using the existing safeSend utility if not
already imported, and keep the same await/try/catch flow so error handling
around send remains unchanged.
In `@src/services/alertService.js`:
- Around line 274-278: The unconditional UPDATE can overwrite terminal/acked
states; change the query that currently calls pool.query(`UPDATE alerts SET
state = 'sent', sent_at = NOW() WHERE id = $1`, [alert.id]) to a guarded update
that only sets state='sent' when the current state is not a terminal or
acknowledged state (e.g., add "AND state NOT IN ('acked','resolved')" or your
app's terminal state names). Keep the same parameters (alert.id), and consider
using the returned rowCount/RETURNING to detect whether the update occurred so
callers (in this module where getPool() and the pool.query(...) call live) can
handle the case where the state was already terminal.
- Around line 21-22: The global cooldowns Map is unbounded and will leak memory;
change its usage to store expiry timestamps (e.g., cooldowns.set(key,
expiresAt)) and enforce TTL eviction by (a) checking and deleting expired
entries on access in whatever functions consult it (e.g., the methods that
check/add cooldowns) and (b) adding a periodic cleanup (setInterval) that scans
cooldowns and deletes entries older than the TTL; also cap total size (optional)
and ensure the cleanup interval is cleared on shutdown to avoid timers leaking.
Ensure all references to the cooldowns Map (insertions, reads, deletions) are
updated to use the expiry value semantics so entries are removed when expired.
- Around line 64-65: The logger calls in alertService use bare messages (e.g.,
info('Alert service initialized')) and should pass minimal structured metadata
for consistency; update those info/error calls in this module (including the
call that logs "Alert service initialized" and the other occurrences around the
later startup/shutdown and runtime log points) to include a metadata object such
as { service: 'alertService', component: '<componentName>' } (or at least {
service: 'alertService' }) so every Winston call follows the pattern
info('message', { ...metadata }) for easier filtering and observability.
- Around line 210-247: The current SELECT-then-INSERT/UPDATE flow is racey;
replace it with a single atomic upsert using the alerts unique constraint on
unresolved fingerprint so concurrent ingests cannot create duplicates. Implement
an INSERT ... ON CONFLICT (fingerprint) WHERE state != 'resolved' DO UPDATE that
increments count, sets last_seen = NOW(), updates severity to the
higher/more-recent effectiveSeverity, merges payload (payload = alerts.payload
|| $payload::jsonb), and RETURNING * so the code uses the returned row as alert
(replace the existing pool.query SELECT + conditional INSERT/UPDATE logic around
fingerprint/effectiveSeverity/payload/correlationId/guildId). Keep the
subsequent notification logic (isNew determination, NOTIFY_THRESHOLDS check,
isNotificationAllowed) but derive isNew from whether the returned row was
created or updated (e.g., compare returned count or use INSERT ... RETURNING to
indicate insertion) rather than the prior separate SELECT.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
migrations/002_alerts-table.cjssrc/api/index.jssrc/api/routes/alerts.jssrc/api/server.jssrc/db.jssrc/index.jssrc/services/alertDelivery.jssrc/services/alertService.jstests/api/routes/alerts.test.jstests/services/alertDelivery.test.jstests/services/alertService.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — useimport/export, neverrequire()
Usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation
Files:
src/api/server.jstests/services/alertService.test.jssrc/api/routes/alerts.jstests/api/routes/alerts.test.jssrc/index.jssrc/db.jssrc/services/alertDelivery.jssrc/services/alertService.jstests/services/alertDelivery.test.jssrc/api/index.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston for logging — import{ info, warn, error }from../logger.js
NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read config
UsesetConfigValue(path, value, guildId?)fromsrc/modules/config.jsto update config at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
UsesafeSend()wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Files:
src/api/server.jssrc/api/routes/alerts.jssrc/index.jssrc/db.jssrc/services/alertDelivery.jssrc/services/alertService.jssrc/api/index.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must achieve at least 80% code coverage on statements, branches, functions, and lines
Files:
tests/services/alertService.test.jstests/api/routes/alerts.test.jstests/services/alertDelivery.test.js
🧬 Code graph analysis (11)
src/api/server.js (1)
src/services/alertService.js (1)
ingestAlert(162-258)
tests/services/alertService.test.js (3)
src/services/alertService.js (13)
shutdownAlertService(70-74)initAlertService(62-65)generateFingerprint(83-87)VALID_SOURCES(41-41)VALID_CATEGORIES(46-46)VALID_SEVERITIES(51-51)VALID_STATES(56-56)ingestAlert(162-258)ackAlert(293-309)resolveAlert(316-332)getActiveAlerts(343-375)alert(217-217)getAlertById(382-391)src/db.js (1)
getPool(303-308)src/api/routes/alerts.js (1)
alerts(26-31)
migrations/002_alerts-table.cjs (1)
src/services/alertService.js (6)
pool(199-199)pool(274-274)pool(295-295)pool(318-318)pool(345-345)pool(384-384)
src/api/routes/alerts.js (1)
src/services/alertService.js (4)
getActiveAlerts(343-375)getAlertById(382-391)ackAlert(293-309)resolveAlert(316-332)
tests/api/routes/alerts.test.js (2)
src/api/routes/alerts.js (1)
alerts(26-31)src/services/alertService.js (5)
getActiveAlerts(343-375)getAlertById(382-391)alert(217-217)ackAlert(293-309)resolveAlert(316-332)
src/index.js (1)
src/services/alertService.js (3)
shutdownAlertService(70-74)ingestAlert(162-258)initAlertService(62-65)
src/db.js (1)
src/services/alertService.js (1)
ingestAlert(162-258)
src/services/alertDelivery.js (3)
src/services/alertService.js (1)
alert(217-217)src/logger.js (1)
warn(230-232)src/modules/config.js (1)
err(94-94)
src/services/alertService.js (2)
src/logger.js (2)
info(223-225)warn(230-232)src/db.js (1)
getPool(303-308)
tests/services/alertDelivery.test.js (1)
src/services/alertDelivery.js (1)
sendAlertDM(62-108)
src/api/index.js (1)
src/api/middleware/auth.js (1)
requireAuth(36-70)
🔇 Additional comments (8)
src/api/index.js (1)
23-24: Alerts endpoints are correctly protected.Mounting
/alertsbehindrequireAuth()matches the intended auth model.tests/services/alertDelivery.test.js (1)
46-147: Solid behavioral coverage for alert DM delivery.The suite covers success path, failure modes, severity rendering, and payload shaping well.
src/api/routes/alerts.js (1)
41-44: File does not exist in the repository.The file
src/api/routes/alerts.jsdoes not exist. The repository contains only the following API route files:auth.js,config.js,guilds.js,health.js, andwebhooks.js. This review cannot be applied to the current codebase.Likely an incorrect or invalid review comment.
src/api/server.js (1)
80-90: The code snippet in this review does not exist in the repository. The filesrc/api/server.jsat lines 80-90 contains standard error logging with theerror()function, not thealertServicepattern described in the review comment. Additionally, neitheralertService,ingestAlert, nor the referenced.catch(() => {})pattern exists anywhere in the codebase for this file.Likely an incorrect or invalid review comment.
src/db.js (1)
80-92: Code snippet does not match src/db.js — review references non-existent alertService integration.The code at lines 80–92 in src/db.js uses Sentry error reporting, not an
alertServicewithingestAlert(). The code referenced in the review comment (import('./services/alertService.js'),ingestAlert({ source: 'db', ... })) does not exist in the codebase.The actual error handling at those lines already logs via
logError()(line 77) and captures exceptions via Sentry (lines 80–84), with the.catch(() => {})silently swallowing Sentry import failures.Likely an incorrect or invalid review comment.
src/index.js (1)
315-324: This review comment references code that does not exist in the repository.No
uncaughtExceptionhandler oringestAlert()function exists in the codebase. Lines 315-324 of src/index.js contain Discord client error handlers that already use Winston logging (error()) and Sentry exception capture. Unhandled exceptions are automatically captured by Sentry (configured withautoSessionTracking: true), and graceful shutdown is handled via existingSIGTERM/SIGINThandlers that callgracefulShutdown().Likely an incorrect or invalid review comment.
tests/api/routes/alerts.test.js (1)
49-208: Good endpoint-behavior coverage for auth, filters, and transition outcomes.This suite is well-scoped for the new
/api/v1/alertsGET/PATCH contract and catches key validation/error paths.tests/services/alertService.test.js (1)
94-535: Comprehensive ingest/lifecycle test coverage for the new service API.The suite exercises core alert flows and major failure paths with clear mocks and assertions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sentry.js`:
- Around line 32-34: Update the misleading comment above the Sentry
configuration where autoSessionTracking is set (the block including
autoSessionTracking: true) to reference the correct SDK version used in the
project (e.g., `@sentry/node` v10) or remove the version-specific mention; edit
the comment text to say that current Sentry SDK (v10) handles unhandled
rejections/uncaught exceptions by default or simply state that explicit handling
is being kept for clarity.
- Around line 29-30: The tracesSampleRate fallback uses || which treats 0 as
falsy and prevents explicitly setting 0; update the assignment for
tracesSampleRate (currently using parseFloat(process.env.SENTRY_TRACES_RATE) ||
0.1) to properly allow 0 by first parsing the env var into a numeric value, then
use nullish/NaN-safe logic (e.g. let rate =
parseFloat(process.env.SENTRY_TRACES_RATE); tracesSampleRate =
Number.isFinite(rate) ? rate : 0.1) so that 0 is preserved but invalid/absent
values default to 0.1.
In `@tests/sentry.test.js`:
- Around line 7-34: Add edge-case tests for configuration parsing in
tests/sentry.test.js: add a test that stubs SENTRY_TRACES_RATE=0 to assert the
module handles a zero traces rate correctly (import ../src/sentry.js and assert
sentryEnabled and that the tracesSampleRate used by Sentry.init is 0 via the
Sentry mock), and add a test that stubs SENTRY_ENVIRONMENT and NODE_ENV to
verify that SENTRY_ENVIRONMENT takes precedence over NODE_ENV (stub SENTRY_DSN,
set SENTRY_ENVIRONMENT='staging' and NODE_ENV='development', import
../src/sentry.js, assert sentryEnabled and that Sentry.init was called with
environment 'staging'); reference the Sentry.init call and the
SENTRY_TRACES_RATE / SENTRY_ENVIRONMENT / NODE_ENV env vars when wiring the
Sentry mock.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.env.examplepackage.jsonsrc/api/server.jssrc/db.jssrc/index.jssrc/sentry.jstests/sentry.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
**/*.js: Use ESM modules only — useimport/export, neverrequire()
Usenode:protocol for Node.js builtins (e.g.import { readFileSync } from 'node:fs')
Always use semicolons
Use single quotes for strings
Use 2-space indentation
No TypeScript — use plain JavaScript with JSDoc comments for documentation
Files:
src/db.jssrc/sentry.jssrc/api/server.jssrc/index.jstests/sentry.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.js: Always use Winston for logging — import{ info, warn, error }from../logger.js
NEVER useconsole.log,console.warn,console.error, or anyconsole.*method in src/ files
Pass structured metadata to Winston logging calls (e.g.info('Message processed', { userId, channelId }))
Use custom error classes fromsrc/utils/errors.jsfor error handling
Always log errors with context before re-throwing
UsegetConfig(guildId?)fromsrc/modules/config.jsto read config
UsesetConfigValue(path, value, guildId?)fromsrc/modules/config.jsto update config at runtime
UsesplitMessage()utility for messages exceeding Discord's 2000-character limit
UsesafeSend()wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Files:
src/db.jssrc/sentry.jssrc/api/server.jssrc/index.js
tests/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Test files must achieve at least 80% code coverage on statements, branches, functions, and lines
Files:
tests/sentry.test.js
🧠 Learnings (3)
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/**/*.js : Always use Winston for logging — import `{ info, warn, error }` from `../logger.js`
Applied to files:
src/api/server.jssrc/index.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/**/*.js : Use `safeSend()` wrapper for outgoing Discord messages to sanitize mentions and enforce allowedMentions
Applied to files:
src/index.js
📚 Learning: 2026-02-25T02:39:33.506Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T02:39:33.506Z
Learning: Applies to src/commands/*.js : Slash commands must export both `data` (SlashCommandBuilder) and `execute(interaction)` function
Applied to files:
src/index.js
🔇 Additional comments (10)
.env.example (1)
103-113: LGTM!The Sentry configuration section is well-documented with clear comments explaining each variable's purpose, default values, and where to obtain the DSN. The format is consistent with the rest of the file.
src/sentry.js (1)
36-44: LGTM on thebeforeSendfilter.Filtering out
AbortErrorand intentional request cancellations is a sensible noise reduction strategy that prevents expected errors from consuming Sentry quota.src/db.js (1)
80-85: LGTM!The lazy dynamic import pattern correctly avoids circular dependencies while still enabling Sentry error reporting for database pool errors. The error is already logged via Winston before the Sentry capture, so the empty
.catch(() => {})is acceptable here since a failure to report to Sentry shouldn't prevent normal error handling.src/index.js (5)
14-15: LGTM — correct import order for Sentry instrumentation.Importing Sentry before other application modules ensures proper instrumentation. The comment clearly explains this requirement.
225-230: LGTM — comprehensive error context for slash commands.The Sentry capture includes useful context: source tag for filtering, command name for debugging, and user information for tracing issues to specific interactions.
295-298: LGTM — proper Sentry flush before shutdown.The 2-second timeout for
Sentry.flush()is reasonable and the.catch(() => {})ensures shutdown isn't blocked if flushing fails. Placing this beforeclient.destroy()ensures events are sent while the process is still running.
320-334: LGTM — appropriate error and warning capture for Discord events.Good separation of concerns:
- Client errors captured as exceptions (appropriate for unexpected errors)
- Shard disconnects with non-1000 codes captured as warnings (appropriate since they may be transient)
454-459: LGTM — useful runtime context tags.Setting
bot.usernameandbot.versiontags after login provides valuable context for debugging production issues. The info log confirms Sentry is active.package.json (1)
22-22: No action needed.@sentry/node@10.40.0 is the latest published version and has Node.js engine requirements (>=18) that match the project's minimum Node.js version.src/api/server.js (1)
66-69: LGTM — correct placement of Sentry error handler.The Sentry error handler is correctly placed after route mounting but before the custom error middleware. This ensures Sentry captures errors while still allowing the custom middleware to handle the response. The
@sentry/nodev10setupExpressErrorHandler()is compatible with Express 5.x and the integration follows the documented best practices.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @BillChirico. * #91 (comment) The following files were modified: * `src/db.js` * `src/index.js`
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Docstrings generation was requested by @BillChirico. * #91 (comment) The following files were modified: * `src/db.js` * `src/index.js`
|
Request timed out after 900000ms (requestId=b5220cb1-e712-402d-ad54-d33035ca3881) |
…rd disconnect, and shutdown flush
…ption calls - Created SentryTransport (src/transports/sentry.js) that forwards error/warn logs to Sentry - Added transport to logger.js — single integration point - Removed all manual if(sentryEnabled) checks from index.js, server.js, db.js - Metadata 'source', 'command', 'module' auto-promoted to Sentry tags - Stack traces reconstructed for proper Sentry grouping
…ersion comment, add edge case tests
BillChirico
force-pushed
the
feat/warning-system
branch
from
ce3b743 to
77ffd9a
Compare
1 participant
Summary
Integrates Sentry for error tracking, performance monitoring, and alerting via the existing Winston logging pipeline. Every
error()andwarn()call in the codebase automatically reports to Sentry — zero manual instrumentation needed.Closes #38
Architecture
Single integration point — no
Sentry.captureException()calls scattered throughout the codebase. The SentryTransport hooks into Winston and forwards error/warn logs automatically.Changes
New Files
src/sentry.js— Sentry SDK initialization (must be imported first). Configurable via env vars.src/transports/sentry.js— Winston transport that forwards error/warn logs to Sentry with proper tags and stack traces.tests/sentry.test.js— Tests for Sentry moduleModified Files
src/logger.js— Adds SentryTransport whenSENTRY_DSNis setsrc/index.js— Imports sentry.js first; addssourcetag to error metadata; shard disconnect handler; Sentry flush on shutdown.env.example— Documented Sentry env varsHow It Works
src/sentry.jsinitializes the Sentry SDK on import (ifSENTRY_DSNis set)src/logger.jsadds aSentryTransportto Winstonerror()orwarn()call anywhere in the codebase → automatically captured by Sentrysource,command,moduleare promoted to Sentry tags for filteringError Capture (Automatic via Winston)
error('...', { source: 'discord_client' })error('...', { source: 'slash_command', command })warn('...', { source: 'discord_shard' })error('...', { source: 'database_pool' })error('Unhandled API error', ...)Environment Variables
SENTRY_DSNSENTRY_ENVIRONMENTNODE_ENVorproductionSENTRY_TRACES_RATE0.1(10%)Setup
SENTRY_DSNin.envZero-config when disabled — without
SENTRY_DSN, the SDK is not initialized and the SentryTransport is not added.Testing
Breaking Changes
None — Sentry is opt-in via env var