fix: sentry controlplane auto-instrumentation #2270
Conversation
miklosbarabas
requested review from
JivusAyrus,
StarpTech,
thisisnithin and
wilsonrivera
as code owners
WalkthroughSentry initialization was changed from an exported config/init API to environment-driven runtime initialization; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)controlplane/src/core/sentry.config.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
controlplane/Dockerfile (1)
46-51: Document SENTRY_ENABLED default in control plane docsThe Dockerfile defaults
SENTRY_ENABLED=false, disabling Sentry unless explicitly enabled.
- Add
SENTRY_ENABLED(defaultfalse) to the Control Plane configuration documentation.- Confirm that
falseis the intended default in your deployment strategy.controlplane/docker/patch-sentry.sh (1)
24-27: Refine sed pattern and add validation
Use a stricter regex to match only the app declaration and verify insertion succeeded:- # Insert setup line after 'const app' - sed -i '/const app/a\ - Sentry.setupFastifyErrorHandler(app); - ' "$file" + # Insert setup line after the app declaration, then verify + if ! sed -i '/^const[[:space:]]\+app[[:space:]]*=/a\ + Sentry.setupFastifyErrorHandler(app); + ' "$file" || ! grep -q 'Sentry.setupFastifyErrorHandler(app)' "$file"; then + echo "[!] Warning: Failed to insert Sentry handler into $file" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
controlplane/Dockerfile(1 hunks)controlplane/docker/docker-entrypoint.sh(1 hunks)controlplane/docker/patch-sentry.sh(1 hunks)controlplane/src/core/env.schema.ts(0 hunks)controlplane/src/core/sentry.config.ts(2 hunks)controlplane/src/index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- controlplane/src/core/env.schema.ts
- controlplane/src/index.ts
🧰 Additional context used
🪛 Shellcheck (0.11.0)
controlplane/docker/patch-sentry.sh
[warning] 2-2: In POSIX sh, set option pipefail is undefined.
(SC3040)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
controlplane/docker/patch-sentry.sh (2)
7-10: LGTM! The idempotency check is well-designed.Checking for the existing import before patching ensures the script can be safely re-run without causing duplicate imports or corruption.
14-22: LGTM! The temp file approach is safe and portable.Using a temp file with
mktempand atomicmvensures the operation is safe even if interrupted. The approach correctly preserves the original file content while prepending the imports.controlplane/docker/docker-entrypoint.sh (3)
5-13: LGTM! The conditional patching logic is clear and well-structured.The script properly checks the SENTRY_ENABLED flag and provides helpful logging at each step. The conditional logic is correct and the default value handling
${SENTRY_ENABLED:-false}ensures safe behavior when the variable is unset.
16-18: LGTM! Proper use ofexecfor signal handling.Using
exec "$@"replaces the shell process with the actual Node.js process, ensuring signals (like SIGTERM for graceful shutdown) are properly delivered to the main application rather than being caught by the shell wrapper.
10-10: Script availability confirmed:patch-sentry.shis copied into/usr/local/binand marked executable in the Dockerfile, so it will be in PATH.controlplane/src/core/sentry.config.ts (2)
11-11: LGTM! Making fields optional aligns with the new runtime approach.Making
dsnandenableLogsoptional provides flexibility for different deployment configurations where some Sentry features may not be needed.Also applies to: 17-17
34-37: Ensureinit()is invoked and its returned Sentry client is used
No matching call sites found forinit(); confirm you’re calling this function and consuming the returnedSentryobject (or remove thereturn Sentryif it’s never used).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/docker/patch-sentry.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
controlplane/src/core/sentry.config.ts (1)
38-47: Critical: Past review concerns remain unaddressed.The module-level initialization still has the issues flagged in the previous review:
Missing
SENTRY_ENABLEDcheck: Init runs unconditionally when this file is imported, regardless of whether Sentry should be enabled. While the Docker patch script may control imports, this hardcodes the assumption and removes flexibility.No DSN validation: If
SENTRY_DSNis undefined,Sentry.init()is called withdsn: undefined(line 40), which can fail silently or behave unpredictably. There's no validation or warning.Module-level side effects: Importing this file immediately triggers initialization, making testing harder and creating implicit dependencies on environment state.
Apply the suggested fix from the previous review:
-init({ - sentry: { - dsn: process.env.SENTRY_DSN, - tracesSampleRate: Number(process.env.SENTRY_TRACES_SAMPLE_RATE ?? 1), - profileSessionSampleRate: Number(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE ?? 1), - sendDefaultPii: (process.env.SENTRY_SEND_DEFAULT_PII ?? 'true') === 'true', - eventLoopBlockIntegrationThresholdMs: Number(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS ?? 100), - enableLogs: (process.env.SENTRY_EVENT_ENABLE_LOGS ?? 'true') === 'true', - }, -}); +// Only initialize if explicitly enabled via environment +if (process.env.SENTRY_ENABLED === 'true') { + if (!process.env.SENTRY_DSN) { + console.warn('SENTRY_ENABLED is true but SENTRY_DSN is not set. Skipping Sentry initialization.'); + } else { + init({ + sentry: { + dsn: process.env.SENTRY_DSN, + tracesSampleRate: Number(process.env.SENTRY_TRACES_SAMPLE_RATE ?? 1), + profileSessionSampleRate: Number(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE ?? 1), + sendDefaultPii: (process.env.SENTRY_SEND_DEFAULT_PII ?? 'true') === 'true', + eventLoopBlockIntegrationThresholdMs: Number(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS ?? 100), + enableLogs: (process.env.SENTRY_EVENT_ENABLE_LOGS ?? 'true') === 'true', + }, + }); + } +}
🧹 Nitpick comments (1)
controlplane/src/core/sentry.config.ts (1)
34-34: Consider using a proper logger instead of console.log.While this informational message is helpful, production code should use a structured logging framework rather than console.log for consistency and proper log level management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/sentry.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
controlplane/src/core/sentry.config.ts (1)
38-47: Past review concerns remain: Module-level initialization without guards.The module-level
init()call still triggers Sentry initialization immediately when this file is imported, with no checks forSENTRY_ENABLEDor DSN presence. While the Docker entrypoint/patch script approach (mentioned in the PR summary) conditionally imports this file only whenSENTRY_ENABLED=true, this creates tight coupling between deployment configuration and code behavior.The past review comment (lines 38-47) recommended checking
process.env.SENTRY_ENABLEDwithin this file and validating DSN before initialization. This would provide better separation of concerns and make the code more testable and flexible.Consider implementing the guard suggested in the past review:
-init({ - sentry: { - dsn: process.env.SENTRY_DSN, - tracesSampleRate: Number(process.env.SENTRY_TRACES_SAMPLE_RATE ?? 1), - profileSessionSampleRate: Number(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE ?? 1), - sendDefaultPii: (process.env.SENTRY_SEND_DEFAULT_PII ?? 'true') === 'true', - eventLoopBlockIntegrationThresholdMs: Number(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS ?? 100), - enableLogs: (process.env.SENTRY_EVENT_ENABLE_LOGS ?? 'true') === 'true', - }, -}); +// Only initialize if explicitly enabled +if (process.env.SENTRY_ENABLED === 'true') { + init({ + sentry: { + dsn: process.env.SENTRY_DSN, + tracesSampleRate: Number(process.env.SENTRY_TRACES_SAMPLE_RATE ?? 1), + profileSessionSampleRate: Number(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE ?? 1), + sendDefaultPii: (process.env.SENTRY_SEND_DEFAULT_PII ?? 'true') === 'true', + eventLoopBlockIntegrationThresholdMs: Number(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS ?? 100), + enableLogs: (process.env.SENTRY_EVENT_ENABLE_LOGS ?? 'true') === 'true', + }, + }); +}This approach decouples the code from deployment scripts and makes behavior explicit within the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/sentry.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
miklosbarabas
force-pushed
the
miklos/fix-sentry-controlplane-autoinstrument
branch
from
0b732dd to
0dc04d4
Compare
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
controlplane/src/core/sentry.config.ts (2)
11-11: Consider validating numeric environment variables.The
Number()conversions forSENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS,SENTRY_PROFILE_SESSION_SAMPLE_RATE, andSENTRY_TRACES_SAMPLE_RATElack validation. If these environment variables contain non-numeric values,Number()returnsNaN, which Sentry may silently ignore or treat unexpectedly.Apply this diff to add validation:
+const parseEnvNumber = (value: string | undefined, defaultValue: number): number => { + if (!value) return defaultValue; + const parsed = Number(value); + return isNaN(parsed) ? defaultValue : parsed; +}; + if (process.env.SENTRY_ENABLED === 'true' && process.env.SENTRY_DSN) { Sentry.init({ dsn: process.env.SENTRY_DSN, integrations: [ fastifyIntegration(), - eventLoopBlockIntegration({ threshold: Number(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS ?? 100) }), + eventLoopBlockIntegration({ threshold: parseEnvNumber(process.env.SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS, 100) }), nodeProfilingIntegration(), pinoIntegration({ log: { levels: ['info', 'warn', 'error'] } }), ], - profileSessionSampleRate: Number(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE ?? 1), + profileSessionSampleRate: parseEnvNumber(process.env.SENTRY_PROFILE_SESSION_SAMPLE_RATE, 1), sendDefaultPii: (process.env.SENTRY_SEND_DEFAULT_PII ?? 'true') === 'true', - tracesSampleRate: Number(process.env.SENTRY_TRACES_SAMPLE_RATE ?? 1), + tracesSampleRate: parseEnvNumber(process.env.SENTRY_TRACES_SAMPLE_RATE, 1), profileLifecycle: (process.env.SENTRY_PROFILE_LIFECYCLE as "trace" | "manual") ?? "trace", enableLogs: (process.env.SENTRY_ENABLE_LOGS ?? 'false') === 'true', });Also applies to: 15-15, 17-17
18-18: Validate profileLifecycle value before type assertion.The type assertion
as "trace" | "manual"bypasses TypeScript's type checking. IfSENTRY_PROFILE_LIFECYCLEcontains an invalid value (e.g., "auto"), Sentry may behave unexpectedly or fail silently.Apply this diff to add runtime validation:
+ const validLifecycle = process.env.SENTRY_PROFILE_LIFECYCLE === 'manual' ? 'manual' : 'trace'; - profileLifecycle: (process.env.SENTRY_PROFILE_LIFECYCLE as "trace" | "manual") ?? "trace", + profileLifecycle: validLifecycle,controlplane/src/index.ts (1)
181-181: Consider: Redundant environment check.The check for
SENTRY_ENABLEDandSENTRY_DSNhere duplicates the guard condition insentry.config.ts(line 6). While this provides defense in depth, if initialization insentry.config.tsfails or is skipped, the error handler setup will also be skipped here. This is likely the intended behavior, but it's worth confirming this duplication is intentional.If the duplication is intentional for clarity, consider adding a comment:
+// Setup Sentry error handler if Sentry was initialized if (process.env.SENTRY_ENABLED === 'true' && process.env.SENTRY_DSN) { Sentry.setupFastifyErrorHandler(app); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
controlplane/package.json(1 hunks)controlplane/src/core/env.schema.ts(0 hunks)controlplane/src/core/sentry.config.ts(1 hunks)controlplane/src/index.ts(2 hunks)
💤 Files with no reviewable changes (1)
- controlplane/src/core/env.schema.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
🔇 Additional comments (5)
controlplane/package.json (1)
54-56: LGTM! Consistent Sentry package upgrades.All three Sentry packages are upgraded consistently to ^10.19.0, which is the latest stable version in the 10.x line. The version bump is well-aligned with the configuration changes in the other files.
Based on learnings
controlplane/src/core/sentry.config.ts (2)
6-6: Well done! Previous validation concerns addressed.The guard condition checking both
SENTRY_ENABLED === 'true'and the presence ofSENTRY_DSNaddresses the validation concerns raised in previous reviews. This ensures Sentry only initializes when explicitly enabled and properly configured.
13-13: Appropriate use of pinoIntegration for logging.The addition of
pinoIntegrationwith appropriate log levels ('info', 'warn', 'error') integrates well with Sentry's logging capabilities and provides structured log forwarding to Sentry.Based on learnings
controlplane/src/index.ts (2)
1-3: Correct initialization pattern for Sentry.The side-effect import of
sentry.config.jsat the top ensures Sentry initializes before application code runs, which is the recommended pattern for Sentry's auto-instrumentation features.Based on learnings
181-183: Appropriate placement of Fastify error handler.The
setupFastifyErrorHandleris correctly called after app creation and conditionally based on environment configuration. This pattern ensures Sentry error handling integrates properly with the Fastify request lifecycle.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/core/sentry.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: integration_test (./events)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
controlplane/src/core/sentry.config.ts (1)
7-34: LGTM! Type safety and DSN validation concerns addressed.The refactoring successfully addresses previous review concerns:
Type safety: Using
envVariables.parse(process.env)with the zod schema eliminates the unsafe type casts andNumber()calls flagged in earlier reviews. All configuration values are now validated and coerced by zod.DSN validation: The guard
if (SENTRY_ENABLED && SENTRY_DSN)ensures Sentry only initializes when both the flag and DSN are present, addressing the missing validation concern.Pino integration: The addition of
pinoIntegrationon line 25 aligns with @sentry/node v10.x's built-in pino support, replacing the previous separate transport package. Based on learnings.The module-level side-effect pattern (parsing and conditional initialization at import time) is pragmatic for this use case, though it does create implicit dependencies on environment state.
Optional: Add error handling for environment parsing.
Consider wrapping
envVariables.parse(process.env)in a try-catch to provide clearer error messages if environment validation fails:+try { const { SENTRY_ENABLED, SENTRY_DSN, SENTRY_SEND_DEFAULT_PII, SENTRY_TRACES_SAMPLE_RATE, SENTRY_PROFILE_SESSION_SAMPLE_RATE, SENTRY_PROFILE_LIFECYCLE, SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS, SENTRY_ENABLE_LOGS } = envVariables.parse(process.env); +} catch (error) { + console.error('Failed to parse Sentry environment variables:', error); + throw error; +}controlplane/src/index.ts (1)
181-183: LGTM! Fastify error handler correctly attached.The conditional setup of Sentry's Fastify error handler after app creation is the correct approach. This ensures Sentry captures unhandled errors in Fastify request handlers.
Minor: Consider reusing parsed environment variables.
The environment variables are parsed twice: once in
sentry.config.ts(via the side-effect import) and again here on line 75. While not harmful, you could export the parsedSENTRY_ENABLEDandSENTRY_DSNfromsentry.config.tsand import them here to avoid duplicate parsing:In
sentry.config.ts:export const { SENTRY_ENABLED, SENTRY_DSN } = envVariables.parse(process.env);In
index.ts:import { SENTRY_ENABLED, SENTRY_DSN } from './core/sentry.config.js';This is a minor optimization and not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controlplane/src/core/env.schema.ts(1 hunks)controlplane/src/core/sentry.config.ts(1 hunks)controlplane/src/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controlplane/src/core/sentry.config.ts (1)
controlplane/src/core/env.schema.ts (1)
envVariables(3-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_push_image
- GitHub Check: integration_test (./telemetry)
- GitHub Check: image_scan
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
🔇 Additional comments (2)
controlplane/src/core/env.schema.ts (1)
208-212: LGTM! Consistent environment variable pattern.The addition of
SENTRY_ENABLE_LOGSfollows the established pattern for boolean environment variables in this schema. The type transformation and default value are appropriate.controlplane/src/index.ts (1)
1-3: LGTM! Correct Sentry initialization pattern.The side-effect import on line 1 ensures Sentry initializes early in the application lifecycle, which is the recommended pattern for automatic instrumentation. The eslint-disable comment on line 3 indicates the import ordering is intentional.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controlplane/src/index.ts (1)
180-182: Consider wrapping Sentry.setupFastifyErrorHandler in try/catch
If this call throws, it will crash the app—wrap it in a try/catch and log errors to allow graceful degradation. No direct@opentelemetrydependencies were found, so OT v2 compatibility isn’t a concern here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controlplane/src/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: build_test
- GitHub Check: image_scan
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image (nonroot)
- GitHub Check: build_test
- GitHub Check: integration_test (./events)
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build_test
🔇 Additional comments (2)
controlplane/src/index.ts (2)
1-3: LGTM! Correct import placement for Sentry auto-instrumentation.The side-effect import on line 1 is correctly placed at the very top to ensure Sentry initializes before other modules are loaded, which is essential for proper auto-instrumentation. The namespace import on line 3 provides access to
setupFastifyErrorHandler.
72-73: LGTM!The environment variable extraction is correct and aligns with the conditional Sentry setup below.
miklosbarabas
deleted the
miklos/fix-sentry-controlplane-autoinstrument
branch
Summary by CodeRabbit
New Features
Refactor
Chores
Documentation
Checklist