chore: update Sentry integration - #2931
Conversation
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThe composition worker-pool system is enhanced with comprehensive Sentry instrumentation. The worker initializes Sentry with Fastify and Pino integrations, refactors its default export to guarantee event flushing, and the pool layer captures span context to record queue size metrics during task execution. ChangesComposition Worker Pool Sentry Instrumentation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/src/core/composition/composeGraphs.worker.ts (1)
81-81: ⚡ Quick win
fastifyIntegration()may be ineffective in worker threads.This worker runs in an isolated thread/child process and does not handle HTTP requests via Fastify. The
fastifyIntegration()instruments Fastify's request handling, which isn't applicable here. Consider removing it from the worker's Sentry config to avoid unnecessary overhead and align the integration list with the worker's actual runtime context.The shared
sentry.config.tsincludes it because the main process runs Fastify, but workers don't share that context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controlplane/src/core/composition/composeGraphs.worker.ts` at line 81, The worker is loading fastifyIntegration() which is irrelevant in worker threads; update the Sentry initialization used by composeGraphs.worker.ts to remove fastifyIntegration() from the integrations list. Locate where Sentry is initialized (or where sentry.config.ts is imported) and either filter out fastifyIntegration() from the integrations array before calling Sentry.init, or create a small worker-specific integrations array that excludes fastifyIntegration() (use the fastifyIntegration symbol to identify it). Ensure composeGraphs.worker.ts uses the filtered/worker-specific integrations when booting Sentry so the Fastify instrumentation is not registered in the worker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Line 81: The worker is loading fastifyIntegration() which is irrelevant in
worker threads; update the Sentry initialization used by composeGraphs.worker.ts
to remove fastifyIntegration() from the integrations list. Locate where Sentry
is initialized (or where sentry.config.ts is imported) and either filter out
fastifyIntegration() from the integrations array before calling Sentry.init, or
create a small worker-specific integrations array that excludes
fastifyIntegration() (use the fastifyIntegration symbol to identify it). Ensure
composeGraphs.worker.ts uses the filtered/worker-specific integrations when
booting Sentry so the Fastify instrumentation is not registered in the worker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06417edb-265f-4e92-8de7-cb34f6d46b7a
📒 Files selected for processing (2)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/composition/composeGraphs.worker.ts
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (25.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2931 +/- ##
==========================================
- Coverage 66.05% 64.95% -1.11%
==========================================
Files 258 327 +69
Lines 27309 47076 +19767
Branches 0 5218 +5218
==========================================
+ Hits 18040 30578 +12538
- Misses 7818 16474 +8656
+ Partials 1451 24 -1427
🚀 New features to boost your workflow:
|
| ); | ||
| } | ||
|
|
||
| export default async function composeGraphsInWorkerActual( |
There was a problem hiding this comment.
Just a nitpick on already merged PR 😅 but the name is kinda of confusing even though I understand the intention.
Could we rename it as part of subsequent PR? Maybe use composeGraphsInWorker for this actual exported function and rename the other one to composeGraphsInWorkerWithTraces
Summary by CodeRabbit
Refactor
Chores
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.