perf(clickhouse): stop storing native event attributes - #4866
Conversation
|
WalkthroughThe pull request adds a ClickHouse migration for Merge Risk: ⚪ Minimal · up to The migration stops storing native event attributes while preserving the materialized text representation and removes unused indexes; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the migration, design, dependency on Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches📝 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e6f1300 to
9b86819
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
9b86819 to
c0fbe91
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal-packages/clickhouse/src/taskEvents.test.ts (1)
80-93: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd an insert/read assertion for
attributes_text.These metadata assertions can pass even when
TASK_EVENT_V2_INSERT_COLUMNSomitsattributes. Insert a representative event throughinsertTaskEventsV2, readattributes_text, and assert that it contains the input attributes. This prevents silent data loss when the migration is applied before#4860.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: e5316a18-a3b3-448f-b438-198775333395
📒 Files selected for processing (2)
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sqlinternal-packages/clickhouse/src/taskEvents.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (33)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (23, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (16, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (24, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (20, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (22, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (17, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (19, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (21, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (1, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (15, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (13, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (18, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (4, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (3, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (14, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (10, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (6, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (7, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (12, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (11, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (9, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (5, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (8, 24)
- GitHub Check: webapp / 🧪 Unit Tests: Webapp (2, 24)
- GitHub Check: runops-guard / runops-guard
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (2, 2)
- GitHub Check: fk-cascade-guard / fk-cascade-guard
- GitHub Check: internal / 🧪 Unit Tests: Internal
- GitHub Check: e2e-webapp / 🧪 E2E Tests: Webapp (1, 2)
- GitHub Check: typecheck / typecheck
- GitHub Check: code-quality / code-quality
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build and publish previews
🧰 Additional context used
📓 Path-based instructions (8)
We use vitest exclusively.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
**Prefer static imports over dynamic imports.**
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Add crumbs as you write code — not just when debugging.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sqlinternal-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 DDL in migrations must be idempotent: use `...
📄 CodeRabbit inference engine (internal-packages/clickhouse/CLAUDE.md)
Files:
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql
Use vitest for all tests in the Trigger.dev repository
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Use function declarations instead of default exports
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
Use types over interfaces for TypeScript Avoid using enums; prefer string unions or const objects instead
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
internal-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 Do not use high-cardinality attributes in OTEL metr...
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
Files:
internal-packages/clickhouse/src/taskEvents.test.ts
🔇 Additional comments (4)
internal-packages/clickhouse/schema/042_reduce_task_events_v2_storage_overhead.sql (3)
17-21: LGTM!
8-9: 🩺 Stability & AvailabilityNo change required for replicated ALTER ordering.
trigger_dev.task_events_v2usesMergeTree, and the migration runner executes Goose directly against the configured ClickHouse URL. The repository defines no replicated engine orON CLUSTERmigration path, so thisCANNOT_ASSIGN_ALTERscenario does not apply.
15-15: 🗄️ Data Integrity & IntegrationNo change needed.
insertTaskEventsV2passesTASK_EVENT_V2_INSERT_COLUMNStoClickhouseWriter.insertUnsafe, and that list includesattributes. The writer therefore supplies theEPHEMERALinput used byattributes_text.internal-packages/clickhouse/src/taskEvents.test.ts (1)
95-107: LGTM!
2 participants
Summary
Stop storing native JSON event attributes while retaining the existing materialized
attributes_textrepresentation. Also remove the two unused full-text indexes from the source event table.Design
This depends on #4860. Its explicit-column writer must be deployed before this migration is applied so attributes continue to feed the materialized string.
The JSON input becomes EPHEMERAL, eliminating its stored subcolumns and merge overhead for new parts. Existing parts retain their JSON files until normal merges or TTL removal.
The migration is intentionally not reversible automatically. Restoring native JSON storage safely requires inspecting the current schema and coordinating the writer change, so it should be done with a new forward migration rather than a retry-sensitive Down migration.