improv(webapp): Add new table for optimized logs search #3036
Conversation
…zed materialized view that filters out debug events, partial spans, and empty span events at ingestion time. * ClickHouse client updates: New getLogsSearchListQueryBuilder and taskEventsSearch accessor on the ClickHouse class. * LogsListPresenter: Switches to the new search table, uses triggered_timestamp for cursor pagination instead of unixTimestamp. * Spans route: Also switches to the new search query builder. * Seed spanSpammer: Adds a 10s trace with events and metadata operations for testing.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSwitches log listing to a new ClickHouse search table and query builder (task_events_search_v1 via taskEventsSearch.logsListQueryBuilder). Adds a migration creating task_events_search_v1 and a materialized view that computes triggered_timestamp and attributes_text. Removes includeDebugLogs and all debug UI/route toggles. Changes presenter, query builders, and ClickHouse interfaces to use a new cursor shape: organizationId, environmentId, triggeredTimestamp, and spanId; updates cursor encoding/decoding, lexicographic ORDER BY, pagination logic, and result fields (including message truncation and renamed result types). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts (1)
267-276:⚠️ Potential issue | 🟡 MinorMinor: inconsistent indentation on Line 270.
Line 270 has extra leading spaces compared to the surrounding
queryBuilder.where(...)calls.Proposed fix
if (effectiveFrom) { const fromNs = convertDateToNanoseconds(effectiveFrom); - queryBuilder.where("inserted_at >= {insertedAtStart: DateTime64(3)}", { - insertedAtStart: convertDateToClickhouseDateTime(effectiveFrom), - }); + queryBuilder.where("inserted_at >= {insertedAtStart: DateTime64(3)}", { + insertedAtStart: convertDateToClickhouseDateTime(effectiveFrom), + });
🤖 Fix all issues with AI agents
In `@apps/webapp/app/presenters/v3/LogsListPresenter.server.ts`:
- Around line 366-373: The tuple comparison using
queryBuilder.where("(environment_id, triggered_timestamp, trace_id) < (...)", {
cursorEnvId: ..., cursorTriggeredTimestamp: ..., cursorTraceId: ... }) is unsafe
because trace_id is not part of the table ORDER BY and can cause
skipped/duplicated rows; change the pagination predicate to mirror the actual
ORDER BY columns (use the exact ordering keys used by the query—e.g.,
environment_id and triggered_timestamp plus a deterministic tiebreaker such as
the primary key/id) and implement lexicographic comparisons like (environment_id
= :env AND triggered_timestamp < :ts) OR (environment_id < :env) with a
deterministic fallback using the primary key (id) instead of trace_id,
referencing decodedCursor fields accordingly so the WHERE clause and ORDER BY
are consistent.
- Around line 352-361: Remove the dead commented-out debug-filter block and
eliminate the now-unused includeDebugLogs parameter: delete the commented lines
in LogsListPresenter.server.ts and then remove includeDebugLogs from the
LogsListOptions type, LogsListOptionsSchema, and the destructured parameters in
the call method so it is no longer declared or referenced; ensure any
validation/schema generation and any remaining references to includeDebugLogs in
the file are cleaned up (e.g., remove from LogsListOptions,
LogsListOptionsSchema, and the call(...) parameter list).
In `@internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql`:
- Around line 25-27: The table's MergeTree ORDER BY lacks the cursor tiebreaker
trace_id used by LogsListPresenter, causing non-deterministic pagination; update
the table definition (the ENGINE = MergeTree block and its ORDER BY clause) to
include trace_id appended to the existing ORDER BY tuple (organization_id,
environment_id, triggered_timestamp) so ordering becomes (organization_id,
environment_id, triggered_timestamp, trace_id), then rebuild or ALTER the table
accordingly to ensure deterministic cursor pagination.
🧹 Nitpick comments (1)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx (1)
66-66: Consider whether the search table is optimal for this trace-scoped query.This route fetches all events for a specific
trace_id+run_idwithin an environment. The search table's ORDER BY(organization_id, environment_id, triggered_timestamp)doesn't includetrace_id, and there's no skip index on it — so ClickHouse must scan all granules within the environment's partitions. Theidx_run_idbloom filter helps narrow byrun_id, but for large environments this may regress compared to queryingtask_events_v2directly (which may have different ordering). If trace-level lookups are frequent, consider adding a bloom filter index ontrace_idto the search table.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/seed/src/trigger/spanSpammer.tsis excluded by!references/**
📒 Files selected for processing (5)
apps/webapp/app/presenters/v3/LogsListPresenter.server.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxinternal-packages/clickhouse/schema/016_add_task_events_search_v1.sqlinternal-packages/clickhouse/src/index.tsinternal-packages/clickhouse/src/taskEvents.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Always import tasks from@trigger.dev/sdk, never use@trigger.dev/sdk/v3or deprecatedclient.defineJobpattern
Every Trigger.dev task must be exported and have a uniqueidproperty with no timeouts in the run function
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxinternal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Import from
@trigger.dev/coreusing subpaths only, never import from root
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxinternal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webappAccess environment variables via
envexport fromapps/webapp/app/env.server.ts, never useprocess.envdirectly
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
**/*.{js,ts,jsx,tsx,json,md,yaml,yml}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier before committing
Files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxinternal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/otel-metrics.mdc)
**/*.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 metrics such as UUIDs/IDs (envId, userId, runId, projectId, organizationId), unbounded integers (itemCount, batchSize, retryCount), timestamps (createdAt, startTime), or free-form strings (errorMessage, taskName, queueName)
When exporting OTEL metrics via OTLP to Prometheus, be aware that the exporter automatically adds unit suffixes to metric names (e.g., 'my_duration_ms' becomes 'my_duration_ms_milliseconds', 'my_counter' becomes 'my_counter_total'). Account for these transformations when writing Grafana dashboards or Prometheus queries
Files:
internal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.tsapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
internal-packages/clickhouse/schema/**/*.sql
📄 CodeRabbit inference engine (CLAUDE.md)
internal-packages/clickhouse/schema/**/*.sql: ClickHouse migrations must use Goose format with-- +goose Upand-- +goose Downmarkers
Follow ClickHouse naming conventions:raw_prefix for input tables,_v1,_v2suffixes for versioning,_mv_v1suffix for materialized views
Files:
internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql
🧠 Learnings (18)
📚 Learning: 2026-02-11T16:50:07.201Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx:126-131
Timestamp: 2026-02-11T16:50:07.201Z
Learning: In apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.dashboards.$dashboardId.widgets.tsx, MetricsDashboard entities are intentionally scoped to the organization level, not the project level. The dashboard lookup should filter by organizationId only (not projectId), allowing dashboards to be accessed across projects within the same organization. The optional projectId field on MetricsDashboard serves other purposes and should not be used as an authorization constraint.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
📚 Learning: 2025-12-08T15:19:56.823Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2760
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx:278-281
Timestamp: 2025-12-08T15:19:56.823Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx, the tableState search parameter uses intentional double-encoding: the parameter value contains a URL-encoded URLSearchParams string, so decodeURIComponent(value("tableState") ?? "") is required to fully decode it before parsing with new URLSearchParams(). This pattern allows bundling multiple filter/pagination params as a single search parameter.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
📚 Learning: 2025-07-12T18:06:04.133Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 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/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsxapps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2025-06-14T08:07:46.625Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 2175
File: apps/webapp/app/services/environmentMetricsRepository.server.ts:202-207
Timestamp: 2025-06-14T08:07:46.625Z
Learning: In apps/webapp/app/services/environmentMetricsRepository.server.ts, the ClickHouse methods (getTaskActivity, getCurrentRunningStats, getAverageDurations) intentionally do not filter by the `tasks` parameter at the ClickHouse level, even though the tasks parameter is accepted by the public methods. This is done on purpose as there is not much benefit from adding that filtering at the ClickHouse layer.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
📚 Learning: 2026-02-03T18:27:40.429Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx:553-555
Timestamp: 2026-02-03T18:27:40.429Z
Learning: In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables/route.tsx, the menu buttons (e.g., Edit with PencilSquareIcon) in the TableCellMenu are intentionally icon-only with no text labels as a compact UI pattern. This is a deliberate design choice for this route; preserve the icon-only behavior for consistency in this file.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `schedules.task()` for scheduled/cron tasks instead of regular `task()`
Applied to files:
internal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use logger methods (debug, log, info, warn, error) from `trigger.dev/sdk/v3` for structured logging in tasks
Applied to files:
internal-packages/clickhouse/src/taskEvents.tsinternal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Export tasks with unique IDs within the project to enable proper task discovery and execution
Applied to files:
internal-packages/clickhouse/src/taskEvents.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `.withStreams()` to subscribe to realtime streams from task metadata in addition to run changes
Applied to files:
internal-packages/clickhouse/src/taskEvents.ts
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/clickhouse/schema/**/*.sql : Follow ClickHouse naming conventions: `raw_` prefix for input tables, `_v1`, `_v2` suffixes for versioning, `_mv_v1` suffix for materialized views
Applied to files:
internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes
Applied to files:
internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql
📚 Learning: 2026-01-15T11:50:06.067Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/clickhouse/schema/**/*.sql : ClickHouse migrations must use Goose format with `-- +goose Up` and `-- +goose Down` markers
Applied to files:
internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use the `task()` function from `trigger.dev/sdk/v3` to define tasks with id and run properties
Applied to files:
internal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use `tasks.trigger()` with type-only imports to trigger tasks from backend code without importing the task implementation
Applied to files:
internal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Use lifecycle functions (init, cleanup, onStart, onSuccess, onFailure, handleError) for task event handling
Applied to files:
internal-packages/clickhouse/src/index.ts
📚 Learning: 2025-11-27T16:27:35.304Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-11-27T16:27:35.304Z
Learning: Applies to **/trigger.config.ts : Configure OpenTelemetry instrumentations and exporters in trigger.config.ts for enhanced logging
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
📚 Learning: 2026-02-06T19:53:38.843Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 2994
File: apps/webapp/app/presenters/v3/DeploymentListPresenter.server.ts:233-237
Timestamp: 2026-02-06T19:53:38.843Z
Learning: When constructing Vercel dashboard URLs from deployment IDs, always strip the dpl_ prefix from the ID. Implement this by transforming the ID with .replace(/^dpl_/, "") before concatenating into the URL: https://vercel.com/${teamSlug}/${projectName}/${cleanedDeploymentId}. Consider centralizing this logic in a small helper (e.g., getVercelDeploymentId(id) or a URL builder) and add tests to verify both prefixed and non-prefixed inputs.
Applied to files:
apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
🧬 Code graph analysis (3)
apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx (1)
apps/webapp/app/services/clickhouseInstance.server.ts (1)
clickhouseClient(5-5)
internal-packages/clickhouse/src/taskEvents.ts (2)
internal-packages/clickhouse/src/client/types.ts (1)
ClickhouseReader(60-199)internal-packages/clickhouse/src/index.ts (1)
ClickHouseSettings(2-2)
internal-packages/clickhouse/src/index.ts (1)
internal-packages/clickhouse/src/taskEvents.ts (1)
getLogsSearchListQueryBuilder(259-285)
⏰ 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 (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: sdk-compat / Deno Runtime
- 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 / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: sdk-compat / Node.js 22.12 (ubuntu-latest)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: sdk-compat / Cloudflare Workers
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
🔇 Additional comments (5)
internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql (2)
1-62: Migration structure and naming conventions look good.Goose markers are present, naming follows
_v1/_mv_v1conventions, down migration drops MV before the table, TTL and partition strategy are reasonable.
51-58: No action required — the WHERE filter is semantically correct.The source table
task_events_v2definesattributes_textas a MATERIALIZED column with the formulatoJSONString(attributes)(line 31 of 010_add_task_events_v2.sql). This means the WHERE filter onattributes_text = '{}'directly references the source's pre-computed materialized column, which is guaranteed to be identical totoJSONString(attributes)in the SELECT. No semantic inconsistency exists.Likely an incorrect or invalid review comment.
internal-packages/clickhouse/src/taskEvents.ts (1)
239-285: LGTM — clean migration of the query builder to the new search table.The renamed type, function, and table target are consistent. Column list matches the new schema including
triggered_timestamp.internal-packages/clickhouse/src/index.ts (1)
227-231: LGTM — newtaskEventsSearchaccessor follows existing patterns.apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx (1)
34-99: Route logic and migration to the new query builder look correct.Auth checks, parameter validation, query construction, and response mapping are all sound.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ENGINE = MergeTree | ||
| PARTITION BY toDate(triggered_timestamp) | ||
| ORDER BY (organization_id, environment_id, triggered_timestamp) |
There was a problem hiding this comment.
Cursor pagination tiebreaker (trace_id) is not in the table's ORDER BY.
The table ORDER BY is (organization_id, environment_id, triggered_timestamp), but LogsListPresenter paginates with the tuple (environment_id, triggered_timestamp, trace_id). Since trace_id is absent from the ordering key, rows sharing the same triggered_timestamp have no deterministic order — the cursor comparison can skip or duplicate rows across pages.
Consider adding trace_id to the ORDER BY:
Proposed fix
-ORDER BY (organization_id, environment_id, triggered_timestamp)
+ORDER BY (organization_id, environment_id, triggered_timestamp, trace_id)📝 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.
| ENGINE = MergeTree | |
| PARTITION BY toDate(triggered_timestamp) | |
| ORDER BY (organization_id, environment_id, triggered_timestamp) | |
| ENGINE = MergeTree | |
| PARTITION BY toDate(triggered_timestamp) | |
| ORDER BY (organization_id, environment_id, triggered_timestamp, trace_id) |
🤖 Prompt for AI Agents
In `@internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql` around
lines 25 - 27, The table's MergeTree ORDER BY lacks the cursor tiebreaker
trace_id used by LogsListPresenter, causing non-deterministic pagination; update
the table definition (the ENGINE = MergeTree block and its ORDER BY clause) to
include trace_id appended to the existing ORDER BY tuple (organization_id,
environment_id, triggered_timestamp) so ordering becomes (organization_id,
environment_id, triggered_timestamp, trace_id), then rebuild or ALTER the table
accordingly to ensure deterministic cursor pagination.
There was a problem hiding this comment.
they will also have the same trace_id, duplication will be handled at span level
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the LogsListPresenter implementation to understand cursor pagination logic
cat apps/webapp/app/presenters/v3/LogsListPresenter.server.ts
# Check the materialized view definition to understand triggered_timestamp computation
cat internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql | grep -A 5 "triggered_timestamp"
# Search for any code that groups or filters by trace_id in relation to triggered_timestamp
rg -n -C3 "triggered_timestamp.*trace_id|trace_id.*triggered_timestamp" --type=ts
Length of output: 16875
@mpcgrid I don't think that's necessarily the case. The triggered_timestamp is computed as start_time + duration for each individual event/span in the materialized view:
fromUnixTimestamp64Nano(toUnixTimestamp64Nano(start_time) + toInt64(duration)) AS triggered_timestampThis means triggered_timestamp represents the end time of each event. Multiple events from different traces can theoretically have the same end time, even at nanosecond precision:
- Event A from Trace X: starts at T, duration 100ms → ends at T+100ms
- Event B from Trace Y: starts at T+50ms, duration 50ms → ends at T+100ms
Both events would have identical triggered_timestamp values but different trace_id values.
Since the cursor pagination in LogsListPresenter uses the tuple (environment_id, triggered_timestamp, trace_id) for ordering and comparison (line 367, 376), but the table's ORDER BY only includes (organization_id, environment_id, triggered_timestamp), ClickHouse won't guarantee a deterministic order for rows sharing the same triggered_timestamp. This could lead to pagination inconsistencies.
Adding trace_id to the table's ORDER BY ensures the physical row order matches the cursor's comparison semantics, making pagination deterministic.
🧠 Learnings used
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/clickhouse/schema/**/*.sql : ClickHouse migrations must use Goose format with `-- +goose Up` and `-- +goose Down` markers
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/clickhouse/schema/**/*.sql : Follow ClickHouse naming conventions: `raw_` prefix for input tables, `_v1`, `_v2` suffixes for versioning, `_mv_v1` suffix for materialized views
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-15T11:50:06.067Z
Learning: Applies to internal-packages/database/prisma/migrations/**/*.sql : When editing the Prisma schema, remove extraneous migration lines related to specific tables: `_BackgroundWorkerToBackgroundWorkerFile`, `_BackgroundWorkerToTaskQueue`, `_TaskRunToTaskRunTag`, `_WaitpointRunConnections`, `_completedWaitpoints`, `SecretStore_key_idx`, and unrelated `TaskRun` indexes
mpcgrid
changed the title
mpcgrid
changed the title
mpcgrid
changed the title
mpcgrid
changed the title
mpcgrid
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
.../resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
Outdated
Show resolved
Hide resolved
Improved cursor Removed debug from logs
…vents_search_v1' into feature/tri-7365-research-task_events_search_v1
.../resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
Outdated
Show resolved
Hide resolved
.../resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
Outdated
Show resolved
Hide resolved
.../resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🚩 Partition pruning with inserted_at is ineffective on the new search table
The LogsListPresenter applies inserted_at >= ... and inserted_at <= ... filters (lines 269-271, 282-284). On the old task_events_v2 table, these were effective for partition pruning since it used PARTITION BY toDate(inserted_at) (010_add_task_events_v2.sql:48). However, the new task_events_search_v1 table uses PARTITION BY toDate(triggered_timestamp) (016_add_task_events_search_v1.sql:26). The inserted_at filter will still execute correctly but won't help prune partitions, making it dead weight. For effective partition pruning, the time range filter should target triggered_timestamp instead. The start_time filters that are also applied are close to triggered_timestamp (since triggered_timestamp = start_time + duration) but won't trigger partition pruning either since they reference a different column. This won't cause incorrect results but may degrade query performance for large datasets.
(Refers to lines 266-289)
Was this helpful? React with 👍 or 👎 to provide feedback.
| duration, | ||
| parent_span_id, | ||
| toJSONString(attributes) AS attributes_text, | ||
| fromUnixTimestamp64Nano(toUnixTimestamp64Nano(start_time) + toInt64(duration)) AS triggered_timestamp |
There was a problem hiding this comment.
🚩 triggered_timestamp = start_time + duration means log events and span events sort differently than before
The MV computes triggered_timestamp as fromUnixTimestamp64Nano(toUnixTimestamp64Nano(start_time) + toInt64(duration)) at internal-packages/clickhouse/schema/016_add_task_events_search_v1.sql:52. For log events (duration=0), triggered_timestamp = start_time. For spans (duration>0), triggered_timestamp = end_time. The old ordering used start_time directly (toUnixTimestamp(start_time) DESC). This means spans now sort by their completion time rather than start time. This is a deliberate semantic change — logs appear in the order they "completed" rather than started — but could surprise users who expect chronological start-time ordering, especially for long-running spans that may appear far from their actual start in the log timeline.
Was this helpful? React with 👍 or 👎 to provide feedback.
removed unused code
There was a problem hiding this comment.
🚩 CANCELLED status SPANs are unreachable by any level filter
The excluded_statuses parameter at LogsListPresenter.server.ts:300 always excludes ['ERROR', 'CANCELLED'] from the kinds branch of every level. This means:
- INFO filter:
kind IN ('LOG_INFO', 'LOG_LOG', 'SPAN') AND status NOT IN ('ERROR', 'CANCELLED')— excludes CANCELLED SPANs - ERROR filter:
status IN ('ERROR')— doesn't match CANCELLED - No other level claims them
kindToLevel('SPAN', 'CANCELLED') at logUtils.ts:73-94 returns INFO (since CANCELLED isn't checked before the switch). So a SPAN with CANCELLED status is classified as INFO but excluded from the INFO filter. These rows would appear in the unfiltered view but become invisible once any level filter is applied. This may be acceptable since CANCELLED is a terminal state not typically surfaced in logs, but it's an inconsistency worth being aware of.
(Refers to lines 298-301)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Will be figured out in the future after some alpha testing in prod. This status is not that used and we might not want to filter by it.
.../resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.logs.$logId.spans.tsx
Outdated
Show resolved
Hide resolved
| FROM trigger_dev.task_events_v2 | ||
| WHERE | ||
| kind != 'DEBUG_EVENT' | ||
| AND status != 'PARTIAL' |
There was a problem hiding this comment.
🚩 MV's status != 'PARTIAL' is broader than the old SPAN-only PARTIAL filter
The old code had queryBuilder.where("NOT (kind = 'SPAN' AND status = 'PARTIAL')") which only excluded PARTIAL status for SPAN kind events. The new MV at 016_add_task_events_search_v1.sql:56 uses AND status != 'PARTIAL' which excludes ALL event kinds with PARTIAL status. If there are non-SPAN events that legitimately have PARTIAL status and should appear in logs, they would now be silently excluded at the MV ingestion level with no way to recover them without backfilling. This is likely intentional cleanup, but worth confirming no other event kinds use PARTIAL status.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
✅ Checklist
Testing
Tested the migration on test env and locally.
Tested query and merge performance.
Generated tasks and observed the ingested data and searches.
Changelog