perf(clickhouse): prepare task event attribute inserts - #4860
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (30)
🧰 Additional context used📓 Path-based instructions (7)We use vitest exclusively.📄 CodeRabbit inference engine (AGENTS.md) Files:
**Prefer static imports over dynamic imports.**📄 CodeRabbit inference engine (AGENTS.md) Files:
Add crumbs as you write code — not just when debugging.📄 CodeRabbit inference engine (AGENTS.md) Files:
Use vitest for all tests in the Trigger.dev repository📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use function declarations instead of default exports📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs Do not use high-cardinality attributes in OTEL metr...📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc) Files:
🔇 Additional comments (1)
WalkthroughThe ClickHouse writer now accepts explicit insert columns. The task events V2 path supplies all 18 target columns. Integration tests verify nested attribute serialization and automatic Merge Risk: 🔵 Low · up to The PR makes task-event inserts explicit and changes selected administrative reads to extract JSON from the persisted representation without changing the schema. Normal behavior remains covered, but merge should proceed with owner awareness of the bounded rollback-safety concern and the required implementation-documentation follow-up. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description accurately explains the ClickHouse insert and attribute query changes, but it omits the required issue reference, checklist, testing details, changelog, and screenshots sections from the repository template.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 30ed0661-455b-487e-bf67-ab7f7e087936
📒 Files selected for processing (7)
apps/webapp/app/services/admin/missingLlmModels.server.tsapps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sqlinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.test.tsinternal-packages/clickhouse/src/taskEvents.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (29)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
🧰 Additional context used
📓 Path-based instructions (14)
Never use `request.signal` to detect client disconnects. Use `getRequestAbortSignal()` from `app/services/httpAsyncStorage.server.ts`, which is wired to Express response close events.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/services/admin/missingLlmModels.server.ts
We use vitest exclusively. **Never mock anything** - use testcontainers instead.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/taskEvents.test.ts
Test files must not import `app/env.server.ts`; pass configuration as options instead.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.ts
For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
Files:
apps/webapp/app/services/admin/missingLlmModels.server.ts
**Prefer static imports over dynamic imports.** Only use dynamic `import()` when:
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/services/admin/missingLlmModels.server.tsinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/taskEvents.test.ts
Add crumbs as you write code — not just when debugging. Mark lines with
📄 CodeRabbit inference engine (AGENTS.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sqlapps/webapp/app/services/admin/missingLlmModels.server.tsinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/taskEvents.test.ts
Migration file numbering: name files as `0(N+1)_descriptive_name.sql` where N is the largest existing migration number in `schema/`; rebase and renumber if main adds migrations before opening a PR
📄 CodeRabbit inference engine (internal-packages/clickhouse/CLAUDE.md)
Files:
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql
Use zod for validation in packages/core and apps/webapp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsapps/webapp/app/services/admin/missingLlmModels.server.ts
Do not import `env.server.ts` directly or indirectly into test files; instead pass environment-dependent values through options/parameters to make code testable
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.ts
Access environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env`
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsapps/webapp/app/services/admin/missingLlmModels.server.ts
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/taskEvents.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/services/admin/missingLlmModels.server.tsinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/taskEvents.test.ts
Use types over interfaces for TypeScript
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/services/admin/missingLlmModels.server.tsinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/taskEvents.test.ts
When creating or editing OTEL metrics (counters, histograms, gauges), ensure metric attributes have low cardinality by using only enums, booleans, bounded error codes, or bounded shard IDs
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
apps/webapp/test/clickhouseEventRepositoryJsonRecovery.test.tsinternal-packages/clickhouse/src/client/types.tsinternal-packages/clickhouse/src/taskEvents.tsapps/webapp/app/services/admin/missingLlmModels.server.tsinternal-packages/clickhouse/src/client/client.tsinternal-packages/clickhouse/src/taskEvents.test.ts
🧠 Learnings (1)
📚 Learning: 2026-06-16T09:19:47.637Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3960
File: apps/webapp/test/prismaInfrastructureErrorCapture.test.ts:0-0
Timestamp: 2026-06-16T09:19:47.637Z
Learning: In this repo’s Vitest setup, `vitest.config.ts` uses `globals: true`, so identifiers like `vi`, `describe`, `it`, and `expect` are available as globals in Vitest test files. During code review, do not flag missing `vi`/`describe`/`it`/`expect` imports as a runtime error or correctness issue when they’re used in `*.test.ts/tsx` or `*.spec.ts/tsx` files. Explicit imports are still preferred for consistency, but they’re not required for runtime behavior.
Applied to files:
internal-packages/clickhouse/src/taskEvents.test.ts
carderne
changed the title
9fc88e0 to
d6b02fa
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
Summary
Prepare task event writes for a later storage schema change without changing the table itself. Event attributes continue to be stored and read exactly as before.
Design
The event writer now names every insert column explicitly, ensuring
attributesremains part of the input when the column becomes input-only. The remaining native attribute-path queries read the existingattributes_textrepresentation through JSON extraction functions.This PR does not alter the
task_events_v2schema.