feat(otel): allow clickhouse task events to be inserted using async_insert via env vars #2608
Conversation
…nsert via env vars
|
WalkthroughAdds four new ClickHouse-related environment variables to env.server.ts: EVENTS_CLICKHOUSE_INSERT_STRATEGY, EVENTS_CLICKHOUSE_WAIT_FOR_ASYNC_INSERT, EVENTS_CLICKHOUSE_ASYNC_INSERT_MAX_DATA_SIZE, and EVENTS_CLICKHOUSE_ASYNC_INSERT_BUSY_TIMEOUT_MS, with defaults. Extends ClickhouseEventRepositoryConfig to include insertStrategy, waitForAsyncInsert, asyncInsertMaxDataSize, and asyncInsertBusyTimeoutMs. In the repository, a helper builds clickhouse_settings based on the insert strategy and options, conditionally enabling async insert parameters. Batch insert calls now pass these settings. The repository instance wires the new env values into the repository config, converting the wait flag from string to boolean. 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: 0
🧹 Nitpick comments (2)
apps/webapp/app/env.server.ts (1)
1126-1126: Use BoolEnv for consistency with other boolean environment variables.The file uses
BoolEnvfor other boolean environment variables (e.g., lines 62, 82, 391-393). Usingz.string().default("1")here requires manual conversion to boolean at the consumption site (line 47 in clickhouseEventRepositoryInstance.server.ts), which is less idiomatic.Apply this diff to use BoolEnv:
- EVENTS_CLICKHOUSE_WAIT_FOR_ASYNC_INSERT: z.string().default("1"), + EVENTS_CLICKHOUSE_WAIT_FOR_ASYNC_INSERT: BoolEnv.default(true),Then update line 47 in clickhouseEventRepositoryInstance.server.ts:
- waitForAsyncInsert: env.EVENTS_CLICKHOUSE_WAIT_FOR_ASYNC_INSERT === "1", + waitForAsyncInsert: env.EVENTS_CLICKHOUSE_WAIT_FOR_ASYNC_INSERT,apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
145-156: Consider explicit check for "insert_async" to avoid unexpected behavior.The current logic enables async inserts for any
insertStrategyvalue other than"insert"(line 146). If an unexpected value orundefinedis passed, async inserts would be enabled. While the environment default is"insert"so this shouldn't happen in practice, it would be safer to explicitly check for"insert_async"in the condition.Apply this diff for more explicit logic:
#getClickhouseInsertSettings() { - if (this._config.insertStrategy === "insert") { + if (this._config.insertStrategy !== "insert_async") { return {}; } else { return {This makes it clear that async inserts are only enabled when explicitly requested with
"insert_async", and any other value (includingundefinedor unexpected values) will use the default synchronous behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts(3 hunks)apps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.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/env.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.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/env.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.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/env.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.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/env.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.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/env.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.tsapps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
apps/webapp/app/v3/eventRepository/eventRepository.server.ts (1)
events(1468-1472)
apps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1207-1207)
⏰ 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 (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/webapp/app/v3/eventRepository/clickhouseEventRepositoryInstance.server.ts (1)
46-49: LGTM!The new configuration options are properly wired from environment variables to the repository. The boolean conversion on line 47 is correct, though it would be simplified if the suggestion in env.server.ts is adopted.
apps/webapp/app/v3/eventRepository/clickhouseEventRepository.server.ts (1)
126-130: LGTM!The insert call correctly passes the ClickHouse settings based on the configured insert strategy. The settings are built dynamically via the helper method and properly integrated into the insert parameters.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
No description provided.