feat: add more tracing instrumentation to the composition worker pool - #2905
Merged
Aenimus merged 7 commits intoJun 2, 2026
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR extracts Sentry env parsing into a separate schema, instruments WorkerPool creation and deserialization with Sentry spans, adds an optional trace field to composition tasks, injects trace context into pool.run, and continues the trace inside the composition worker with nested spans. ChangesSentry Instrumentation for Composition Worker
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@controlplane/src/core/composition/composeGraphs.pool.ts`:
- Around line 77-80: The code is truncating Sentry numeric settings by calling
toFixed(2) on SENTRY_TRACES_SAMPLE_RATE, SENTRY_PROFILE_SESSION_SAMPLE_RATE, and
SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS in composeGraphs.pool.ts; remove the
toFixed(2) calls and forward the original numeric values (or explicitly
stringify them with String(...) only if the workers require strings) so sampling
rates and thresholds keep full precision; update the SENTRY_PROFILE_LIFECYCLE
entry only if it needs conversion, but do not alter numeric values via toFixed
in any of the referenced env fields.
In `@controlplane/src/core/env.schema.ts`:
- Around line 233-236: Sentry numeric env values currently accept any number;
tighten validation in env.schema.ts by constraining SENTRY_TRACES_SAMPLE_RATE
and SENTRY_PROFILE_SESSION_SAMPLE_RATE to the valid sampling range 0–1 (use
z.coerce.number().min(0).max(1).optional().default(...)) and constrain
SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS to non‑negative (e.g.,
z.coerce.number().min(0).optional().default(100) and optionally a sensible upper
bound like .max(60000)); update the validations for the symbols
SENTRY_TRACES_SAMPLE_RATE, SENTRY_PROFILE_SESSION_SAMPLE_RATE, and
SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d134d64b-7609-4472-bb19-1a38e3ada0ff
📒 Files selected for processing (6)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/composition/composeGraphs.types.tscontrolplane/src/core/composition/composeGraphs.worker.tscontrolplane/src/core/env.schema.tscontrolplane/src/core/sentry.config.tscontrolplane/src/index.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controlplane/src/core/env.schema.ts (1)
222-241: ⚡ Quick winZod is resolved to v3 (zod@3.25.76), so
z.string().optional().transform((val) => val === 'true').default('false')parses the default through the transform and produces booleanfalse—the truthy-string concern is Zod-4-specific. If upgrading to Zod 4 later, switch these.default('false')values to.default(false)to match the output type.🤖 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/env.schema.ts` around lines 222 - 241, The boolean-like env fields (SENTRY_ENABLED, SENTRY_SEND_DEFAULT_PII, SENTRY_ENABLE_LOGS) currently use .string().optional().transform(...).default('false') which relies on Zod v3 behavior; when you upgrade to Zod v4 change each .default('false') to .default(false) so the default value matches the transformed boolean output (update SENTRY_ENABLED, SENTRY_SEND_DEFAULT_PII, SENTRY_ENABLE_LOGS to use .default(false)).
🤖 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/env.schema.ts`:
- Around line 222-241: The boolean-like env fields (SENTRY_ENABLED,
SENTRY_SEND_DEFAULT_PII, SENTRY_ENABLE_LOGS) currently use
.string().optional().transform(...).default('false') which relies on Zod v3
behavior; when you upgrade to Zod v4 change each .default('false') to
.default(false) so the default value matches the transformed boolean output
(update SENTRY_ENABLED, SENTRY_SEND_DEFAULT_PII, SENTRY_ENABLE_LOGS to use
.default(false)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 113b4acd-cab6-48f6-b94a-9195fd7ca11d
📒 Files selected for processing (2)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/env.schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/composition/composeGraphs.pool.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
controlplane/src/core/composition/composeGraphs.worker.ts (1)
190-201: 💤 Low valueConsider differentiating span names for better trace clarity.
Both
printSchemaWithDirectivesspans have identical names, which could make it harder to distinguish them in traces. Consider naming themprintSchemaWithDirectives.federatedGraphSchemaandprintSchemaWithDirectives.federatedGraphClientSchemarespectively.♻️ Proposed fix
const composedSchema = result.success - ? Sentry.startSpan({ name: 'printSchemaWithDirectives' }, () => + ? Sentry.startSpan({ name: 'printSchemaWithDirectives.federatedGraphSchema' }, () => printSchemaWithDirectives(result.federatedGraphSchema), ) : undefined; const federatedClientSchema = result.success - ? Sentry.startSpan({ name: 'printSchemaWithDirectives' }, () => + ? Sentry.startSpan({ name: 'printSchemaWithDirectives.federatedGraphClientSchema' }, () => printSchemaWithDirectives(result.federatedGraphClientSchema), ) : undefined;🤖 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` around lines 190 - 201, The two Sentry.startSpan calls that wrap printSchemaWithDirectives (for result.federatedGraphSchema and result.federatedGraphClientSchema) use the same name and should be differentiated; update the span names used in the Sentry.startSpan invocations (the ones around printSchemaWithDirectives(result.federatedGraphSchema) and printSchemaWithDirectives(result.federatedGraphClientSchema)) to distinct identifiers such as "printSchemaWithDirectives.federatedGraphSchema" and "printSchemaWithDirectives.federatedGraphClientSchema" so traces clearly show which schema is being printed.
🤖 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.
Inline comments:
In `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Around line 42-63: The Sentry env schema uses
z.string().transform(...).default('false') which yields a type mismatch under
Zod v4: the transform returns boolean but the default is the string 'false' so
the default must match the transformed type; update SENTRY_ENABLED,
SENTRY_SEND_DEFAULT_PII, and SENTRY_ENABLE_LOGS in the sentryEnvVariables object
to use .default(false) (or alternatively make the schema a
z.string().default('false').transform(...) so the default is a string that gets
transformed) so the default value type aligns with the transform output in the
sentryEnvVariables definition.
---
Nitpick comments:
In `@controlplane/src/core/composition/composeGraphs.worker.ts`:
- Around line 190-201: The two Sentry.startSpan calls that wrap
printSchemaWithDirectives (for result.federatedGraphSchema and
result.federatedGraphClientSchema) use the same name and should be
differentiated; update the span names used in the Sentry.startSpan invocations
(the ones around printSchemaWithDirectives(result.federatedGraphSchema) and
printSchemaWithDirectives(result.federatedGraphClientSchema)) to distinct
identifiers such as "printSchemaWithDirectives.federatedGraphSchema" and
"printSchemaWithDirectives.federatedGraphClientSchema" so traces clearly show
which schema is being printed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92964c62-263c-47c8-8921-1c712dd69528
📒 Files selected for processing (4)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/composition/composeGraphs.worker.tscontrolplane/src/core/env.schema.tscontrolplane/src/core/sentry.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- controlplane/src/core/composition/composeGraphs.pool.ts
- controlplane/src/core/env.schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@controlplane/src/core/env.schema.ts`:
- Around line 4-8: Update the Sentry env schema entries to add the missing
numeric validations: for SENTRY_TRACES_SAMPLE_RATE and
SENTRY_PROFILE_SESSION_SAMPLE_RATE apply .min(0).max(1) (keeping them
numeric/floating validations), and for SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS
apply .int().min(0); keep the existing SENTRY_ENABLED string->boolean transform
and 'false' default as-is (Zod v3.25.x compatible) and locate these changes
around the SENTRY_* schema definitions (symbols: SENTRY_TRACES_SAMPLE_RATE,
SENTRY_PROFILE_SESSION_SAMPLE_RATE, SENTRY_EVENT_LOOP_BLOCK_THRESHOLD_MS,
SENTRY_ENABLED).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b3e8792-cf0e-44db-a416-ad353bb63325
📒 Files selected for processing (3)
controlplane/src/core/composition/composeGraphs.pool.tscontrolplane/src/core/env.schema.tscontrolplane/src/core/sentry.config.ts
✅ Files skipped from review due to trivial changes (1)
- controlplane/src/core/sentry.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- controlplane/src/core/composition/composeGraphs.pool.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2905 +/- ##
==========================================
+ Coverage 64.61% 64.81% +0.19%
==========================================
Files 326 326
Lines 46629 46746 +117
Branches 5145 5153 +8
==========================================
+ Hits 30131 30299 +168
+ Misses 16473 16423 -50
+ Partials 25 24 -1
🚀 New features to boost your workflow:
|
thisisnithin
approved these changes
Jun 2, 2026
Aenimus
enabled auto-merge (squash)
June 2, 2026 14:33
Aenimus
deleted the
wilson/eng-9673-controlplane-add-more-instrumentation-for-composition
branch
June 2, 2026 14:39
This was referenced Jun 8, 2026
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
3 participants
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 by CodeRabbit
New Features
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.