Conversation
The only way to get a ClickHouse client now is through the factory. Refactored all existing code to use that and pass in an org. The runReplication and otlpExporter are the hot paths here which need special attention in reviews.
🦋 Changeset detectedLatest commit: ffc9e6e The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (70)
WalkthroughThis pull request implements organization-scoped ClickHouse database instance routing. It introduces a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes ✨ 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const taskEventStore = parentStore ?? (await resolveTaskEventRepositoryFlag(featureFlags)); | ||
| const { repository: resolvedRepository } = await clickhouseFactory.getEventRepositoryForOrganization( | ||
| taskEventStore, | ||
| organizationId | ||
| ); |
There was a problem hiding this comment.
🔴 Factory throws on "postgres" store, crashing all PostgreSQL event storage paths
getEventRepository(), getConfiguredEventRepository(), and getV3EventRepository() now unconditionally call clickhouseFactory.getEventRepositoryForOrganization(taskEventStore, organizationId) before checking whether the store is "postgres". When taskEventStore resolves to "postgres" (via resolveTaskEventRepositoryFlag or the TaskRun.taskEventStore column, which defaults to "postgres"), the factory calls buildEventRepository("postgres", ...) at clickhouseFactory.server.ts:385, which throws Unknown ClickHouse event repository store: postgres. The old code never called the factory for the postgres path — it returned the postgres eventRepository directly. This will crash any org or run configured to use PostgreSQL event storage.
Affected call sites
index.server.ts:66—getEventRepositorycalls factory before checking storeindex.server.ts:44—getConfiguredEventRepositorycalls factory before checking storeindex.server.ts:87—getV3EventRepositorypassesparentStore(which can be"postgres") directly to factory
Prompt for agents
The bug is that getEventRepository (and similarly getConfiguredEventRepository and getV3EventRepository) calls clickhouseFactory.getEventRepositoryForOrganization unconditionally, even when the store is 'postgres'. The factory's buildEventRepository function throws for unknown stores including 'postgres'.
The fix: in each of these functions, check if the resolved store is 'postgres' BEFORE calling the factory, and return the postgres eventRepository directly. Only call the factory for 'clickhouse' or 'clickhouse_v2' stores.
Affected functions in apps/webapp/app/v3/eventRepository/index.server.ts:
1. getEventRepository (line 60) — move the factory call inside the clickhouse branches
2. getConfiguredEventRepository (line 19) — move the factory call inside the clickhouse branches
3. getV3EventRepository (line 82) — guard the parentStore === 'postgres' case before calling factory
Alternatively, buildEventRepository in clickhouseFactory.server.ts could handle 'postgres' by returning a dummy or by not throwing, but the cleanest fix is to not call the factory at all for postgres paths.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const clickhouseClient = await clickhouseFactory.getClickhouseForOrganization( | ||
| environment.organizationId, | ||
| "query" | ||
| ); | ||
|
|
||
| const presenter = new ErrorGroupPresenter($replica, clickhouseClient, clickhouseClient); |
There was a problem hiding this comment.
🔴 ErrorGroupPresenter receives query ClickHouse client instead of logs-specific client
The ErrorGroupPresenter constructor takes (replica, logsClickhouse, clickhouse) — two distinct ClickHouse clients. The logsClickhouse parameter was previously the dedicated logs client (from LOGS_CLICKHOUSE_URL) with specific clickhouseSettings like max_memory_usage, max_bytes_before_external_sort, and max_threads configured in the old initializeLogsClickhouseClient(). The new code at errors.$fingerprint/route.tsx:253 passes the same query client for both parameters: new ErrorGroupPresenter($replica, clickhouseClient, clickhouseClient). This means error occurrence queries (this.logsClickhouse.errors.*) will use the query client's settings instead of the logs-specific memory/sort limits, potentially causing query failures or different performance characteristics under load.
| const clickhouseClient = await clickhouseFactory.getClickhouseForOrganization( | |
| environment.organizationId, | |
| "query" | |
| ); | |
| const presenter = new ErrorGroupPresenter($replica, clickhouseClient, clickhouseClient); | |
| const clickhouseClient = await clickhouseFactory.getClickhouseForOrganization( | |
| environment.organizationId, | |
| "query" | |
| ); | |
| const logsClickhouseClient = await clickhouseFactory.getClickhouseForOrganization( | |
| environment.organizationId, | |
| "logs" | |
| ); | |
| const presenter = new ErrorGroupPresenter($replica, logsClickhouseClient, clickhouseClient); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const queryClickhouse = await clickhouseFactory.getClickhouseForOrganization( | ||
| project.organizationId, | ||
| "query" | ||
| ); | ||
| const presenter = new ErrorsListPresenter($replica, queryClickhouse); |
There was a problem hiding this comment.
🚩 ErrorsListPresenter uses query client instead of logs client — matches old pattern
In the errors index route (errors._index/route.tsx:126-130), the ErrorsListPresenter is constructed with a query type ClickHouse client. The ErrorsListPresenter constructor takes (replica, clickhouse) — a single ClickHouse parameter. Looking at the old code, it used logsClickhouseClient for this. So this is also a change from logs → query client, but since ErrorsListPresenter only has one ClickHouse parameter (unlike ErrorGroupPresenter which has two), this is a deliberate consolidation. The query client may have different settings than the logs client. This is worth verifying but is a design decision rather than a clear bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Closes #
✅ Checklist
Testing
[Describe the steps you took to test this change]
Changelog
[Short description of what has changed]
Screenshots
[Screenshots]
💯