fix(otel): propagate the task event store to run descendants #2583
Conversation
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (8)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts(2 hunks)apps/webapp/app/runEngine/concerns/traceEvents.server.ts(4 hunks)apps/webapp/app/runEngine/services/triggerTask.server.ts(2 hunks)apps/webapp/app/runEngine/types.ts(1 hunks)apps/webapp/app/v3/eventRepository/index.server.ts(1 hunks)apps/webapp/app/v3/services/triggerTask.server.ts(2 hunks)apps/webapp/app/v3/services/triggerTaskV1.server.ts(4 hunks)apps/webapp/test/engine/triggerTask.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/test/engine/triggerTask.test.tsapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/traceEvents.server.tsapps/webapp/app/runEngine/types.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Our tests are all vitest
Files:
apps/webapp/test/engine/triggerTask.test.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/test/engine/triggerTask.test.tsapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/traceEvents.server.tsapps/webapp/app/runEngine/types.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/test/engine/triggerTask.test.tsapps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/traceEvents.server.tsapps/webapp/app/runEngine/types.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
{apps/webapp/**/__tests__/**/*.{ts,tsx},apps/webapp/**/*.{test,spec}.{ts,tsx}}: Do not import app/env.server.ts into tests, either directly or indirectly
Tests should only import classes/functions from files under apps/webapp/app/**/*.ts
Files:
apps/webapp/test/engine/triggerTask.test.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests must use Vitest
Tests should avoid mocks or stubs and use helpers from @internal/testcontainers when Redis or Postgres are needed
Test files live beside the files under test and should use descriptive describe and it blocks
Files:
apps/webapp/test/engine/triggerTask.test.ts
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/traceEvents.server.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/v3/eventRepository/index.server.tsapps/webapp/app/runEngine/concerns/idempotencyKeys.server.tsapps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/concerns/traceEvents.server.tsapps/webapp/app/runEngine/types.tsapps/webapp/app/v3/services/triggerTaskV1.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
🧠 Learnings (4)
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Do not use client.defineJob or any deprecated v2 patterns (e.g., eventTrigger) when defining tasks
Applied to files:
apps/webapp/app/v3/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
apps/webapp/app/v3/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Define tasks using task({ id, run, ... }) with a unique id per project
Applied to files:
apps/webapp/app/v3/services/triggerTask.server.tsapps/webapp/app/runEngine/services/triggerTask.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : For idempotent child-task invocations, create and pass idempotencyKey (and optional TTL) when calling trigger()/batchTrigger() from tasks
Applied to files:
apps/webapp/app/runEngine/services/triggerTask.server.ts
🧬 Code graph analysis (6)
apps/webapp/app/v3/eventRepository/index.server.ts (4)
apps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.server.ts (1)
clickhouseEventRepository(6-9)apps/webapp/app/v3/eventRepository/eventRepository.server.ts (1)
eventRepository(1475-1475)apps/webapp/app/presenters/v3/SpanPresenter.server.ts (1)
eventRepository(440-557)apps/webapp/app/v3/taskEventStore.server.ts (1)
getTaskEventStore(53-55)
apps/webapp/app/runEngine/concerns/idempotencyKeys.server.ts (2)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (1)
request(8-16)apps/webapp/app/runEngine/types.ts (1)
TriggerTaskRequest(24-30)
apps/webapp/app/v3/services/triggerTask.server.ts (1)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (1)
DefaultTraceEventsConcern(7-125)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (3)
apps/webapp/app/runEngine/types.ts (3)
TraceEventConcern(143-160)TriggerTaskRequest(24-30)TracedEventSpan(131-141)apps/webapp/app/v3/eventRepository/eventRepository.types.ts (1)
IEventRepository(311-417)apps/webapp/app/v3/eventRepository/index.server.ts (1)
getEventRepository(20-39)
apps/webapp/app/runEngine/types.ts (1)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (1)
request(8-16)
apps/webapp/app/v3/services/triggerTaskV1.server.ts (1)
apps/webapp/app/v3/marqs/sharedQueueConsumer.server.ts (1)
environment(2187-2219)
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/runEngine/concerns/traceEvents.server.ts (3)
8-16: LGTM! Runtime repository resolution works correctly.The method properly delegates to
getEventRepositorywith the appropriate parameters, enabling store-aware behavior based onparentStore.
18-59: LGTM! TheparentStoreparameter propagates correctly.The method signature matches the
TraceEventConcerninterface, and theparentStoreis properly threaded through to repository resolution. The resolvedstoreis correctly passed to the callback.
61-124: LGTM! Idempotent run tracing correctly uses the resolved repository.The method properly:
- Accepts the
parentStoreparameter matching the interface- Resolves the repository at runtime via
#getEventRepository- Uses the resolved
repositoryfor recording events (line 98)- Passes the resolved
storeto the callback (line 120)
| if (typeof parentStore === "string") { | ||
| if (parentStore === "clickhouse") { | ||
| return { repository: clickhouseEventRepository, store: "clickhouse" }; | ||
| } else { | ||
| return { repository: eventRepository, store: getTaskEventStore() }; | ||
| } |
There was a problem hiding this comment.
Preserve the parent event store selection
When parentStore is provided (Line 24), we intend to keep the descendant run in the same event store as its parent. The current branch discards the supplied store and falls back to getTaskEventStore(), so a parent run stored in "taskEvent" could spawn children in whatever the current default is (e.g. "taskEventPartitioned"). That breaks trace/event continuity and can move data into the wrong backend. Please return the passed-in store instead of recomputing it.
if (typeof parentStore === "string") {
if (parentStore === "clickhouse") {
return { repository: clickhouseEventRepository, store: "clickhouse" };
- } else {
- return { repository: eventRepository, store: getTaskEventStore() };
- }
+ }
+
+ return { repository: eventRepository, store: parentStore };
}📝 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.
| if (typeof parentStore === "string") { | |
| if (parentStore === "clickhouse") { | |
| return { repository: clickhouseEventRepository, store: "clickhouse" }; | |
| } else { | |
| return { repository: eventRepository, store: getTaskEventStore() }; | |
| } | |
| if (typeof parentStore === "string") { | |
| if (parentStore === "clickhouse") { | |
| return { repository: clickhouseEventRepository, store: "clickhouse" }; | |
| } | |
| return { repository: eventRepository, store: parentStore }; | |
| } |
🤖 Prompt for AI Agents
In apps/webapp/app/v3/eventRepository/index.server.ts around lines 24-29, the
branch that handles a supplied parentStore currently returns repository:
eventRepository but sets store: getTaskEventStore(), discarding the passed-in
parentStore; change that branch to return the repository appropriate for
non-clickhouse parents (eventRepository) but preserve and return the passed-in
parentStore value (store: parentStore) instead of recomputing it.
No description provided.