fix(otel): prevent spans with negative durations #2582
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
🧹 Nitpick comments (3)
apps/webapp/app/v3/runEngineHandlers.server.ts (1)
410-412: Retry label could render “#0” — verify semanticsIf
run.attemptNumber === 1, this prints “#0”. If the intent is “previous attempt,” consider clamping or hiding when result < 1.Example fix:
- let retryMessage = `Retry ${ - typeof run.attemptNumber === "number" ? `#${run.attemptNumber - 1}` : "" - } delay`; + const previous = typeof run.attemptNumber === "number" ? run.attemptNumber - 1 : undefined; + let retryMessage = `Retry ${previous && previous > 0 ? `#${previous}` : ""} delay`;apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
435-438: Mixed minimums: endTime path uses 100ms, non‑endTime path uses 100 (ns)Confirm this asymmetry is intentional. If not, consider standardizing the lower bound (e.g., 100ms) for consistency in analytics and UI bars.
Also applies to: 465-466
apps/webapp/app/v3/eventRepository/common.server.ts (1)
31-45: Clamp logic OK; improve precision and guard checks
- Use precise BigInt math to avoid number precision loss.
- Treat
minimumDuration = 0correctly.- Optional: either remove the string branch or widen the type.
Suggested change:
-export function calculateDurationFromStart( - startTime: bigint, - endTime: Date = new Date(), - minimumDuration?: number -) { - const $endtime = typeof endTime === "string" ? new Date(endTime) : endTime; - - const duration = Number(BigInt($endtime.getTime() * 1_000_000) - startTime); - - if (minimumDuration && duration < minimumDuration) { - return minimumDuration; - } - - return duration; -} +export function calculateDurationFromStart( + startTime: bigint, + endTime: Date /* | string */ = new Date(), + minimumDuration?: number +) { + // If you intend to accept strings, widen the type; otherwise drop this branch. + const $endtime = typeof endTime === "string" ? new Date(endTime) : endTime; + // Precise BigInt math, then convert to number + const endNs = BigInt($endtime.getTime()) * 1_000_000n; + const duration = Number(endNs - startTime); + if (minimumDuration != null && duration < minimumDuration) { + return minimumDuration; + } + return duration; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts(11 hunks)apps/webapp/app/v3/eventRepository/common.server.ts(1 hunks)apps/webapp/app/v3/eventRepository/index.server.ts(2 hunks)apps/webapp/app/v3/runEngineHandlers.server.ts(1 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/v3/eventRepository/index.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/common.server.tsapps/webapp/app/v3/runEngineHandlers.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/v3/eventRepository/index.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/common.server.tsapps/webapp/app/v3/runEngineHandlers.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/v3/eventRepository/index.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/common.server.tsapps/webapp/app/v3/runEngineHandlers.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/v3/eventRepository/index.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/common.server.tsapps/webapp/app/v3/runEngineHandlers.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/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/common.server.tsapps/webapp/app/v3/runEngineHandlers.server.ts
🧠 Learnings (1)
📚 Learning: 2025-07-12T18:06:04.133Z
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.
Applied to files:
apps/webapp/app/v3/eventRepository/index.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (2)
apps/webapp/app/v3/eventRepository/eventRepository.server.ts (2)
event(118-149)event(1088-1107)apps/webapp/app/v3/eventRepository/common.server.ts (1)
calculateDurationFromStart(31-45)
apps/webapp/app/v3/runEngineHandlers.server.ts (1)
apps/webapp/app/v3/services/completeAttempt.server.ts (1)
run(419-517)
⏰ 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 / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/v3/eventRepository/index.server.ts (1)
5-5: Switching to primary (prisma) for run lookup looks goodUsing the primary for event-creation reads should reduce replica‑lag “RUN_NOT_FOUND” issues. Please confirm this is intentional for this hot path and acceptable from a read‑load perspective.
Also applies to: 148-171
apps/webapp/app/v3/runEngineHandlers.server.ts (1)
406-409: Early exit prevents negative-duration spansGood guard. This avoids recording a span when now ≥ retryAt.
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
159-160: LGTM: sanitize and normalize duration writesReplacing raw
toString()withformatClickhouseUnsignedIntegerString(...)across all write paths is the right move to prevent negative UInt values in ClickHouse. The changes here look consistent.Also applies to: 465-466, 566-567, 600-601, 649-652, 699-702, 741-744, 789-792, 881-884
| function formatClickhouseUnsignedIntegerString(value: number | bigint): string { | ||
| if (value < 0) { | ||
| return "0"; | ||
| } | ||
|
|
||
| if (typeof value === "bigint") { | ||
| return value.toString(); | ||
| } | ||
|
|
||
| return Math.floor(value).toString(); | ||
| } |
There was a problem hiding this comment.
Harden formatter against NaN/Infinity and negative BigInt
Currently, NaN/Infinity will serialize as "NaN"/"Infinity", causing ClickHouse insert failures. Also guard negative BigInts.
Apply:
-function formatClickhouseUnsignedIntegerString(value: number | bigint): string {
- if (value < 0) {
- return "0";
- }
-
- if (typeof value === "bigint") {
- return value.toString();
- }
-
- return Math.floor(value).toString();
-}
+function formatClickhouseUnsignedIntegerString(value: number | bigint): string {
+ if (typeof value === "bigint") {
+ return value < 0n ? "0" : value.toString();
+ }
+ if (!Number.isFinite(value) || value <= 0) {
+ return "0";
+ }
+ const floored = Math.floor(value);
+ return floored <= 0 ? "0" : String(floored);
+}📝 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.
| function formatClickhouseUnsignedIntegerString(value: number | bigint): string { | |
| if (value < 0) { | |
| return "0"; | |
| } | |
| if (typeof value === "bigint") { | |
| return value.toString(); | |
| } | |
| return Math.floor(value).toString(); | |
| } | |
| function formatClickhouseUnsignedIntegerString(value: number | bigint): string { | |
| if (typeof value === "bigint") { | |
| return value < 0n ? "0" : value.toString(); | |
| } | |
| if (!Number.isFinite(value) || value <= 0) { | |
| return "0"; | |
| } | |
| const floored = Math.floor(value); | |
| return floored <= 0 ? "0" : String(floored); | |
| } |
🤖 Prompt for AI Agents
In apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts around
lines 1857 to 1867, the formatter currently allows NaN/Infinity and negative
BigInts to be serialized which breaks ClickHouse inserts; update the function to
first detect and return "0" for non-finite numbers (Number.isFinite(value) or
Number.isNaN checks) and for numbers that are finite use
Math.floor(value).toString(); for bigints, explicitly guard negative values
(value < 0n) and return "0" for negatives, otherwise return value.toString();
ensure the checks are ordered so typeof value === "bigint" is used for
BigInt-specific comparison and numbers are validated with isFinite/NaN before
flooring.
No description provided.