chore: add tracing to all events #672
Conversation
WalkthroughAdds OpenTelemetry-style tracing to many event-queue handlers by wrapping handlers with per-event span utilities. Each wrapped handler sets span attributes (event.type and relevant IDs including workspace.id) then performs existing workspace loading and OperationPipeline dispatch inside the span context. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor E as Event Source
participant H as Handler (wrapped)
participant T as Tracer/withSpan
participant W as WorkspaceManager
participant P as OperationPipeline
participant D as Dispatcher
E->>H: Emit Created/Updated/Deleted event
H->>T: create span (named per-event)
activate T
T-->>H: span context
H->>T: setAttributes(event.type, ids..., workspace.id)
H->>W: getOrLoad(workspaceId)
alt workspace found
H->>P: build payload + update/delete(...)
P-->>D: dispatch operation
D-->>H: ack
T-->>T: end span (success)
else workspace missing
T-->>T: end span (workspace not found)
end
deactivate T
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (1)
apps/event-queue/src/events/policy.ts (1)
32-70: Use a single tracer for all policy event spans.Grabbing three different tracers (
"new-policy","updated-policy","deleted-policy") splits these spans across separate instrumentation libraries, which makes it harder to aggregate policy traces coherently in OTel backends. The span names can still stay event-specific while the tracer name remains stable for the policy event domain.-const newPolicyTracer = trace.getTracer("new-policy"); -const withNewPolicySpan = makeWithSpan(newPolicyTracer); +const policyTracer = trace.getTracer("event-queue/policy"); +const withPolicySpan = makeWithSpan(policyTracer); -export const newPolicy: Handler<Event.PolicyCreated> = withNewPolicySpan( +export const newPolicy: Handler<Event.PolicyCreated> = withPolicySpan( "new-policy", async (span, event) => { span.setAttribute("policy.id", event.payload.id); span.setAttribute("workspace.id", event.workspaceId); const ws = await WorkspaceManager.getOrLoad(event.workspaceId); if (ws == null) return; const policy = getPolicyWithDates(event.payload); await OperationPipeline.update(ws).policy(policy).dispatch(); }, ); -const updatedPolicyTracer = trace.getTracer("updated-policy"); -const withUpdatedPolicySpan = makeWithSpan(updatedPolicyTracer); - export const updatedPolicy: Handler<Event.PolicyUpdated> = - withUpdatedPolicySpan("updated-policy", async (span, event) => { + withPolicySpan("updated-policy", async (span, event) => { span.setAttribute("policy.id", event.payload.current.id); span.setAttribute("workspace.id", event.workspaceId); const ws = await WorkspaceManager.getOrLoad(event.workspaceId); if (ws == null) return; const policy = getPolicyWithDates(event.payload.current); await OperationPipeline.update(ws).policy(policy).dispatch(); }); -const deletedPolicyTracer = trace.getTracer("deleted-policy"); -const withDeletedPolicySpan = makeWithSpan(deletedPolicyTracer); - export const deletedPolicy: Handler<Event.PolicyDeleted> = - withDeletedPolicySpan("deleted-policy", async (span, event) => { + withPolicySpan("deleted-policy", async (span, event) => { span.setAttribute("policy.id", event.payload.id); span.setAttribute("workspace.id", event.workspaceId); const ws = await WorkspaceManager.getOrLoad(event.workspaceId); if (ws == null) return; await OperationPipeline.delete(ws).policy(event.payload).dispatch(); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/event-queue/src/events/deployment-variables.ts(1 hunks)apps/event-queue/src/events/deployment-versions.ts(1 hunks)apps/event-queue/src/events/deployments.ts(1 hunks)apps/event-queue/src/events/environments.ts(1 hunks)apps/event-queue/src/events/job.ts(2 hunks)apps/event-queue/src/events/policy.ts(2 hunks)apps/event-queue/src/events/release-targets.ts(1 hunks)apps/event-queue/src/events/resources.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/event-queue/src/events/release-targets.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/environments.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/deployment-variables.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/event-queue/src/events/release-targets.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/environments.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/deployment-variables.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/src/events/release-targets.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/environments.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/deployment-variables.ts
🧬 Code graph analysis (8)
apps/event-queue/src/events/release-targets.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
OperationPipeline(39-637)
apps/event-queue/src/events/policy.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
policy(93-96)
apps/event-queue/src/events/environments.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)
apps/event-queue/src/events/deployments.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)
apps/event-queue/src/events/job.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
job(98-101)
apps/event-queue/src/events/deployment-versions.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/pipeline.ts (1)
deploymentVersion(76-79)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)
apps/event-queue/src/events/resources.ts (4)
packages/logger/src/index.ts (2)
makeWithSpan(74-95)trace(7-7)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (2)
resource(54-57)OperationPipeline(39-637)
apps/event-queue/src/events/deployment-variables.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
OperationPipeline(39-637)
⏰ 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). (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (4)
apps/event-queue/src/events/job.ts (1)
19-32: Instrumentation wraps handler without altering flowNice job threading the span through the handler—attributes are set up front and the existing workspace/pipeline logic runs unchanged inside the traced context.
apps/event-queue/src/events/release-targets.ts (1)
12-21: Span wrapper fits the handler contract.The
withSpanwrapper cleanly preserves the original control flow while adding the attributes we need for observability; looks solid.apps/event-queue/src/events/deployment-versions.ts (1)
20-34: Tracing wrapper preserves handler semantics.The span wrapper keeps the existing load/dispatch flow intact while adding useful IDs as attributes and leveraging
makeWithSpan's error status propagation. No further changes needed.apps/event-queue/src/events/resources.ts (1)
24-33: Clear span instrumentation for resource creationWrapping the handler with
withNewResourceSpanand annotating both resource and workspace identifiers gives us actionable trace data while keeping the existing workspace guard untouched. Nice upgrade.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/event-queue/src/events/resources.ts (1)
24-58: Consider adding the event type attribute to resource spans. Resource-variable spans now recordevent.type, but the resource spans don’t. Ifevent.eventTypeis available on these messages, setting it as a span attribute here would keep the telemetry consistent and make querying simpler.apps/event-queue/src/events/deployment-versions.ts (1)
10-12: Use a single tracer scope for these handlers.Creating a distinct tracer for every event type inflates the instrumentation scope cardinality and makes it harder to aggregate traces consistently. OpenTelemetry guidance is to keep the tracer (instrumentation scope) aligned with the producing library/module and vary only the span names. Please collapse these into a single tracer and reuse one
withSpanwrapper across the three handlers.-const newDeploymentVersionTracer = trace.getTracer("new-deployment-version"); -const withNewDeploymentVersionSpan = makeWithSpan(newDeploymentVersionTracer); +const deploymentVersionTracer = trace.getTracer( + "@ctrlplane/event-queue/deployment-versions", +); +const withDeploymentVersionSpan = makeWithSpan(deploymentVersionTracer); @@ -const updatedDeploymentVersionTracer = trace.getTracer( - "updated-deployment-version", -); -const withUpdatedDeploymentVersionSpan = makeWithSpan( - updatedDeploymentVersionTracer, -); - -export const updatedDeploymentVersion: Handler<Event.DeploymentVersionUpdated> = - withUpdatedDeploymentVersionSpan( +export const updatedDeploymentVersion: Handler<Event.DeploymentVersionUpdated> = + withDeploymentVersionSpan( @@ -const deletedDeploymentVersionTracer = trace.getTracer( - "deleted-deployment-version", -); -const withDeletedDeploymentVersionSpan = makeWithSpan( - deletedDeploymentVersionTracer, -); - -export const deletedDeploymentVersion: Handler<Event.DeploymentVersionDeleted> = - withDeletedDeploymentVersionSpan( +export const deletedDeploymentVersion: Handler<Event.DeploymentVersionDeleted> = + withDeploymentVersionSpan(Also applies to: 37-42, 63-68
apps/event-queue/src/events/policy.ts (2)
32-34: Use a single module-level tracer, not one per handler.Multiple tracers fragment instrumentation scope. Prefer one tracer for this module and vary the span name per event.
Apply:
-const newPolicyTracer = trace.getTracer("new-policy"); -const withNewPolicySpan = makeWithSpan(newPolicyTracer); +const policyTracer = trace.getTracer("event-queue.policy"); +const withPolicySpan = makeWithSpan(policyTracer);Then update call sites in this file to use
withPolicySpan(see other comments).
65-73: Drop incorrect type concern; payload is FullPolicy
Event.PolicyDeleted.payloadis defined asFullPolicy, andOperationPipeline.delete(ws).policy(event.payload)expectsFullPolicy, so the original type/contract risk does not apply.Optional: surface workspace‐load failures instead of silently returning:
const ws = await WorkspaceManager.getOrLoad(event.workspaceId); -if (!ws) return; +if (!ws) { + span.addEvent("workspace.load_miss", { workspaceId: event.workspaceId }); + throw new Error(`Workspace ${event.workspaceId} not loaded`); +} await OperationPipeline.delete(ws).policy(event.payload).dispatch();If tracers have been consolidated, rename
withDeletedPolicySpan→withPolicySpan.apps/event-queue/src/events/deployments.ts (1)
9-11: Consolidate tracers to a single module tracer to reduce duplication and improve grouping.Use one tracer for this module and reuse a single
withSpanhelper; keep span names specific per handler.-const newDeploymentTracer = trace.getTracer("new-deployment"); -const withNewDeploymentSpan = makeWithSpan(newDeploymentTracer); +const tracer = trace.getTracer("event-queue.deployments"); +const withSpan = makeWithSpan(tracer);Then replace usages:
withNewDeploymentSpan("new-deployment", …)→withSpan("DeploymentCreated", …)withUpdatedDeploymentSpan("updated-deployment", …)→withSpan("DeploymentUpdated", …)withDeletedDeploymentSpan("deleted-deployment", …)→withSpan("DeploymentDeleted", …)This keeps spans readable and attributes already provide ids.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/event-queue/src/events/deployment-variables.ts(1 hunks)apps/event-queue/src/events/deployment-versions.ts(1 hunks)apps/event-queue/src/events/deployments.ts(1 hunks)apps/event-queue/src/events/environments.ts(1 hunks)apps/event-queue/src/events/job.ts(2 hunks)apps/event-queue/src/events/policy.ts(2 hunks)apps/event-queue/src/events/release-targets.ts(1 hunks)apps/event-queue/src/events/resources.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/event-queue/src/events/environments.ts
- apps/event-queue/src/events/deployment-variables.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/release-targets.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/release-targets.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/src/events/deployment-versions.tsapps/event-queue/src/events/deployments.tsapps/event-queue/src/events/job.tsapps/event-queue/src/events/policy.tsapps/event-queue/src/events/resources.tsapps/event-queue/src/events/release-targets.ts
🧬 Code graph analysis (6)
apps/event-queue/src/events/deployment-versions.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/pipeline.ts (2)
deploymentVersion(76-79)OperationPipeline(39-637)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)
apps/event-queue/src/events/deployments.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
OperationPipeline(39-637)
apps/event-queue/src/events/job.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (2)
job(98-101)OperationPipeline(39-637)
apps/event-queue/src/events/policy.ts (3)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (2)
policy(93-96)OperationPipeline(39-637)
apps/event-queue/src/events/resources.ts (4)
packages/logger/src/index.ts (2)
makeWithSpan(74-95)trace(7-7)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (2)
resource(54-57)OperationPipeline(39-637)
apps/event-queue/src/events/release-targets.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
OperationPipeline(39-637)
⏰ 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). (2)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (9)
apps/event-queue/src/events/job.ts (1)
22-33: Span wrapper integrates cleanlyHappy to see the handler wrapped with
withUpdatedJobSpan; attributes are set up front and the existing workspace + pipeline flow remains unchanged. 👍apps/event-queue/src/events/release-targets.ts (1)
12-22: Tracing wrapper aligns with Handler expectations.The span wrapper cleanly preserves the Handler signature while capturing the key identifiers we care about. Nice work keeping the workspace pipeline flow unchanged.
apps/event-queue/src/events/policy.ts (4)
3-4: Imports look good.Brings in tracing utilities cleanly and follows external→internal import grouping.
48-50: Same tracer consolidation applies here.If you adopt the single tracer, remove this tracer/init block and reuse the module-level one.
62-64: Same tracer consolidation applies here.Remove per-event tracer/init and reuse the module-level tracer.
35-46: Don’t drop events silently – throw on missing workspace
Add a span event and throw instead of returning whengetOrLoadyields null:- const ws = await WorkspaceManager.getOrLoad(event.workspaceId); - if (ws == null) return; + const ws = await WorkspaceManager.getOrLoad(event.workspaceId); + if (ws == null) { + span.addEvent("workspace.load_miss", { workspaceId: event.workspaceId }); + throw new Error(`Workspace ${event.workspaceId} not loaded`); + }Confirm that using
OperationPipeline.updatein thePolicyCreatedhandler is intentional (vs..evaluate). After the single-tracer refactor, you can also replacewithNewPolicySpanwithwithPolicySpan.apps/event-queue/src/events/deployments.ts (3)
22-24: Same tracer consolidation applies here.Fold these into the single module tracer as suggested above.
37-38: Same tracer consolidation applies here.Fold these into the single module tracer as suggested above.
12-21: LGTM on span instrumentation and attribute coverage.Attributes are informative and the
withSpanwrapper ensures error status on thrown exceptions. Once the missing-workspace path sets span status (see above), this handler will be observability-complete.
| const ws = await WorkspaceManager.getOrLoad(event.workspaceId); | ||
| if (ws == null) return; | ||
| await OperationPipeline.update(ws).deployment(event.payload).dispatch(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Do not silently drop events; fix WorkspaceManager.getOrLoad return bug and set span status on missing workspace.
Right now, the first event for any workspace will be dropped: WorkspaceManager.getOrLoad() returns the pre-load value instead of the loaded instance. Your if (ws == null) return; then exits, skipping the pipeline. This is a functional bug. Also, missing workspaces should mark the span as an error, not silently return. (See apps/event-queue/src/workspace/workspace.ts snippet.)
Apply this in-file safeguard and error status, and separately fix getOrLoad:
@@
- const ws = await WorkspaceManager.getOrLoad(event.workspaceId);
- if (ws == null) return;
- await OperationPipeline.update(ws).deployment(event.payload).dispatch();
+ const ws0 = await WorkspaceManager.getOrLoad(event.workspaceId);
+ const ws = ws0 ?? WorkspaceManager.get(event.workspaceId);
+ if (ws == null) {
+ span.setAttribute("workspace.missing", true);
+ span.setStatus({ code: SpanStatusCode.ERROR, message: `workspace not found: ${event.workspaceId}` });
+ return;
+ }
+ await OperationPipeline.update(ws).deployment(event.payload).dispatch();
@@
- const ws = await WorkspaceManager.getOrLoad(event.workspaceId);
- if (ws == null) return;
+ const ws0 = await WorkspaceManager.getOrLoad(event.workspaceId);
+ const ws = ws0 ?? WorkspaceManager.get(event.workspaceId);
+ if (ws == null) {
+ span.setAttribute("workspace.missing", true);
+ span.setStatus({ code: SpanStatusCode.ERROR, message: `workspace not found: ${event.workspaceId}` });
+ return;
+ }
await OperationPipeline.update(ws)
.deployment(event.payload.current)
.dispatch();
@@
- const ws = await WorkspaceManager.getOrLoad(event.workspaceId);
- if (ws == null) return;
- await OperationPipeline.delete(ws).deployment(event.payload).dispatch();
+ const ws0 = await WorkspaceManager.getOrLoad(event.workspaceId);
+ const ws = ws0 ?? WorkspaceManager.get(event.workspaceId);
+ if (ws == null) {
+ span.setAttribute("workspace.missing", true);
+ span.setStatus({ code: SpanStatusCode.ERROR, message: `workspace not found: ${event.workspaceId}` });
+ return;
+ }
+ await OperationPipeline.delete(ws).deployment(event.payload).dispatch();And add the missing import to support setStatus:
@@
-import { makeWithSpan, trace } from "@ctrlplane/logger";
+import { makeWithSpan, trace } from "@ctrlplane/logger";
+import { SpanStatusCode } from "@opentelemetry/api";Fix WorkspaceManager.getOrLoad to actually return the loaded instance:
- static async getOrLoad(id: string) {
- const workspace = WorkspaceManager.get(id);
- if (!workspace) {
- const ws = await Workspace.load(id);
- WorkspaceManager.set(id, ws);
- }
-
- return workspace;
- }
+ static async getOrLoad(id: string) {
+ let workspace = WorkspaceManager.get(id);
+ if (!workspace) {
+ const ws = await Workspace.load(id);
+ WorkspaceManager.set(id, ws);
+ workspace = ws;
+ }
+ return workspace;
+ }Run to confirm other callers aren’t affected and that getOrLoad is fixed everywhere:
Also applies to: 30-34, 45-48
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check getOrLoad definition:"
rg -n "static\s+async\s+getOrLoad" -C2
echo -e "\nCall sites that might rely on the buggy behavior:"
rg -n --type ts -C2 '\bgetOrLoad\s*\(' apps/event-queue/srcLength of output: 14398
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show full getOrLoad implementation
rg -n -C5 "static async getOrLoad" apps/event-queue/src/workspace/workspace.ts
# Show imports and initial handler logic in deployments.ts
sed -n '1,30p' apps/event-queue/src/events/deployments.tsLength of output: 1738
Fix getOrLoad return and enforce error spans in event handlers
- In apps/event-queue/src/workspace/workspace.ts, amend getOrLoad to return the loaded workspace instance:
static async getOrLoad(id: string) { - const workspace = WorkspaceManager.get(id);
- if (!workspace) {
-
const ws = await Workspace.load(id); -
WorkspaceManager.set(id, ws); - }
- //← missing return
- let workspace = WorkspaceManager.get(id);
- if (!workspace) {
-
workspace = await Workspace.load(id); -
WorkspaceManager.set(id, workspace); - }
- return workspace;
}
- In apps/event-queue/src/events/deployments.ts, replace the silent‐drop null‐check with a fallback and error‐span logic as originally diffed (including `import { SpanStatusCode } from "@opentelemetry/api";`).
- Propagate that same pattern (fallback against WorkspaceManager.get + `span.setStatus` on null) to every handler in apps/event-queue/src/events/** that uses `getOrLoad` to prevent dropping the first event.
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In apps/event-queue/src/events/deployments.ts around lines 17-20, the handler
silently returns when WorkspaceManager.getOrLoad yields null which drops the
first event; update WorkspaceManager.getOrLoad in
apps/event-queue/src/workspace/workspace.ts to actually return the loaded
Workspace instance, then in this file replace the simple null-check with the
original fallback + error-span logic: import { SpanStatusCode } from
"@opentelemetry/api", create/obtain the span, if getOrLoad returns null call
span.setStatus({ code: SpanStatusCode.ERROR, message: "workspace not found" })
(or similar) and proceed to use a fallback WorkspaceManager.get before deciding
to stop; apply the same pattern (fallback against WorkspaceManager.get and
span.setStatus on null) to every event handler file under
apps/event-queue/src/events/** that calls getOrLoad so events aren’t silently
dropped.
</details>
<!-- fingerprinting:phantom:poseidon:chinchilla -->
<!-- This is an auto-generated comment by CodeRabbit -->
| export const updatedPolicy: Handler<Event.PolicyUpdated> = | ||
| withUpdatedPolicySpan("updated-policy", async (span, event) => { | ||
| span.setAttribute("event.type", event.eventType); | ||
| span.setAttribute("policy.id", event.payload.current.id); | ||
| span.setAttribute("workspace.id", event.workspaceId); | ||
| const ws = await WorkspaceManager.getOrLoad(event.workspaceId); | ||
| if (ws == null) return; | ||
| const policy = getPolicyWithDates(event.payload.current); | ||
| await OperationPipeline.update(ws).policy(policy).dispatch(); | ||
| }); |
There was a problem hiding this comment.
Repeat: surface workspace-miss as an error, not a silent return.
Apply:
withUpdatedPolicySpan("updated-policy", async (span, event) => {
@@
- const ws = await WorkspaceManager.getOrLoad(event.workspaceId);
- if (ws == null) return;
+ const ws = await WorkspaceManager.getOrLoad(event.workspaceId);
+ if (ws == null) {
+ span.addEvent("workspace.load_miss", { workspaceId: event.workspaceId });
+ throw new Error(`Workspace ${event.workspaceId} not loaded`);
+ }
@@
});If you consolidated tracers, change withUpdatedPolicySpan → withPolicySpan.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const updatedPolicy: Handler<Event.PolicyUpdated> = | |
| withUpdatedPolicySpan("updated-policy", async (span, event) => { | |
| span.setAttribute("event.type", event.eventType); | |
| span.setAttribute("policy.id", event.payload.current.id); | |
| span.setAttribute("workspace.id", event.workspaceId); | |
| const ws = await WorkspaceManager.getOrLoad(event.workspaceId); | |
| if (ws == null) return; | |
| const policy = getPolicyWithDates(event.payload.current); | |
| await OperationPipeline.update(ws).policy(policy).dispatch(); | |
| }); | |
| export const updatedPolicy: Handler<Event.PolicyUpdated> = | |
| withUpdatedPolicySpan("updated-policy", async (span, event) => { | |
| span.setAttribute("event.type", event.eventType); | |
| span.setAttribute("policy.id", event.payload.current.id); | |
| span.setAttribute("workspace.id", event.workspaceId); | |
| const ws = await WorkspaceManager.getOrLoad(event.workspaceId); | |
| if (ws == null) { | |
| span.addEvent("workspace.load_miss", { workspaceId: event.workspaceId }); | |
| throw new Error(`Workspace ${event.workspaceId} not loaded`); | |
| } | |
| const policy = getPolicyWithDates(event.payload.current); | |
| await OperationPipeline.update(ws).policy(policy).dispatch(); | |
| }); |
🤖 Prompt for AI Agents
In apps/event-queue/src/events/policy.ts around lines 51 to 60, the handler
silently returns when WorkspaceManager.getOrLoad(event.workspaceId) yields null;
change this to surface the workspace-miss as an error by recording/setting the
span error and throwing an Error (e.g. record the exception on the span and
throw new Error(`Workspace not found: ${event.workspaceId}`)) so the failure is
visible in logs/telemetry; also, if tracers were consolidated, switch the
wrapper from withUpdatedPolicySpan to withPolicySpan.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/event-queue/src/events/deployment-variables.ts (1)
85-92: Add event.type span attribute for creation events.All of the other handlers record
event.type, which keeps span queries and dashboards consistent. Adding it here prevents the creation spans from being filtered out when analysts slice by event type.Apply this diff to keep the attribute set uniform:
async (span, event) => { + span.setAttribute("event.type", event.eventType); span.setAttribute("deployment-variable-value.id", event.payload.id); span.setAttribute("deployment-variable.id", event.payload.variableId); span.setAttribute("workspace.id", event.workspaceId);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/event-queue/src/events/deployment-variables.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript with explicit types (prefer interfaces for public APIs)
Import styles: Use named imports, group imports by source (std lib > external > internal)
Consistent type imports:import type { Type } from "module"
Prefer async/await over raw promises
Handle errors explicitly (use try/catch and typed error responses)
Files:
apps/event-queue/src/events/deployment-variables.ts
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
Files:
apps/event-queue/src/events/deployment-variables.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Formatting: Prettier is used with
@ctrlplane/prettier-config
Files:
apps/event-queue/src/events/deployment-variables.ts
🧠 Learnings (1)
📚 Learning: 2024-10-30T23:10:58.869Z
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#187
File: apps/jobs/src/ephemeral-env-checker/index.ts:57-0
Timestamp: 2024-10-30T23:10:58.869Z
Learning: In the codebase, deployments are decoupled from environments. When deleting environments (e.g., in `apps/jobs/src/ephemeral-env-checker/index.ts`), associated deployments should not be deleted.
Applied to files:
apps/event-queue/src/events/deployment-variables.ts
🧬 Code graph analysis (1)
apps/event-queue/src/events/deployment-variables.ts (4)
packages/logger/src/index.ts (2)
trace(7-7)makeWithSpan(74-95)apps/event-queue/src/events/index.ts (1)
Handler(71-73)apps/event-queue/src/workspace/workspace.ts (1)
WorkspaceManager(203-223)apps/event-queue/src/workspace/pipeline.ts (1)
OperationPipeline(39-637)
🔇 Additional comments (1)
apps/event-queue/src/events/deployment-variables.ts (1)
68-70: Good call switching to the delete pipeline.This ensures the deletion path now triggers
removeDeploymentVariablerather than re-upserting the record.
Summary by CodeRabbit