fix(clickhouse): correctly format datetime64(9) input format #2580
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
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: 0
🧹 Nitpick comments (1)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
1831-1843: LGTM!The helper functions correctly format and parse ClickHouse DateTime64(9) nanoseconds epoch strings. The logic is straightforward and serves its purpose well.
Optional: Consider adding input validation to improve robustness:
function formatClickhouseDate64NanosecondsEpochString(date: string): string { if (date.length !== 19) { return date; } + + // Validate that the string contains only digits + if (!/^\d{19}$/.test(date)) { + return date; + } return date.substring(0, 10) + "." + date.substring(10); } function convertClickhouseDate64NanosecondsEpochStringToBigInt(date: string): bigint { + // Validate format: digits.digits + if (!/^\d+\.\d+$/.test(date)) { + throw new Error(`Invalid ClickHouse DateTime64 nanoseconds epoch string: "${date}"`); + } + const parts = date.split("."); return BigInt(parts.join("")); }However, given that these functions are internal and their inputs are controlled (always from bigint conversions or ClickHouse responses), the current implementation is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/presenters/v3/RunPresenter.server.ts(1 hunks)apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts(17 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.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/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.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/app/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.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/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.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/presenters/v3/RunPresenter.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: matt-aitken
PR: triggerdotdev/trigger.dev#2264
File: apps/webapp/app/services/runsRepository.server.ts:172-174
Timestamp: 2025-07-12T18:06:04.133Z
Learning: In apps/webapp/app/services/runsRepository.server.ts, the in-memory status filtering after fetching runs from Prisma is intentionally used as a workaround for ClickHouse data delays. This approach is acceptable because the result set is limited to a maximum of 100 runs due to pagination, making the performance impact negligible.
🧬 Code graph analysis (2)
apps/webapp/app/presenters/v3/RunPresenter.server.ts (2)
apps/webapp/app/presenters/v3/SpanPresenter.server.ts (3)
run(559-565)run(567-632)run(634-636)apps/webapp/app/v3/services/cancelTaskRunV1.server.ts (2)
run(132-191)run(193-206)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (2)
apps/webapp/app/v3/otlpExporter.server.ts (1)
events(115-121)apps/webapp/app/v3/eventRepository/common.server.ts (1)
convertDateToNanoseconds(43-45)
⏰ 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). (22)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (8)
114-120: LGTM!The debug log for the first event in a flush batch will help with troubleshooting event formatting issues.
158-158: LGTM!The new formatting helper correctly converts nanoseconds epoch to ClickHouse DateTime64(9) format (e.g., "1759427319.944999936").
188-192: LGTM!The filtering logic correctly converts the formatted ClickHouse DateTime64 string back to bigint for numeric comparison.
224-226: LGTM!Consistent formatting applied to span event timestamps.
260-262: LGTM!Consistent formatting applied across all span event types (Cancellation, AttemptFailed, Other).
Also applies to: 290-292, 324-326
462-462: LGTM!Consistent formatting applied to recordEvent method.
563-563: LGTM!Consistent formatting applied in traceEvent method for both main event and exception event.
Also applies to: 597-597
646-646: LGTM!Consistent formatting applied across all run completion and cancellation methods.
Also applies to: 694-694, 734-734, 780-780, 826-826, 870-870
apps/webapp/app/presenters/v3/RunPresenter.server.ts (1)
183-184: LGTM!Correctly treating WAITING_TO_RESUME status as partial. This ensures that runs waiting to resume are displayed as "executing" in the UI and have null duration in the timeline, which accurately reflects that they are still in progress.
No description provided.