Feat/simplified query#144
Open
Vunovati wants to merge 25 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 57 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/ui-react-app/src/custom-observability-catalog.tsx (1)
461-467:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard only narrows summaries; query responses are still unsafely treated as trace rows.
Now that
TraceDetailcan receivemethod: "query", non-summary responses fall into theelsepath and are cast toOtelTracesRow[]without checking shape. That can break rendering when the query selects different dimensions.Suggested fix
function isTraceSummaries( props: TraceDetailProps & { hasData: true } ): props is Omit<TraceDetailProps & { hasData: true }, "response"> & { response: SearchResult<TraceSummaryRow> | null; } { return props.element.dataSource?.method === "searchTraceSummariesPage"; } + +function isTraceRows( + props: TraceDetailProps & { hasData: true } +): props is Omit<TraceDetailProps & { hasData: true }, "response"> & { + response: SearchResult<OtelTracesRow> | null; +} { + return props.element.dataSource?.method === "searchTracesPage"; +}Then branch in
TraceDetailwithisTraceRows(props)and add a safe fallback for unsupportedqueryshapes (instead of casting).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/ui-react-app/src/custom-observability-catalog.tsx` around lines 461 - 467, The current type guard isTraceSummaries only narrows summary responses, leaving the "query" branch in TraceDetail to unsafely cast to OtelTracesRow[]; add a new type guard isTraceRows that checks props.element.dataSource?.method === "query" and validates the response shape (e.g., existence of expected row fields) so TraceDetail can safely branch: use isTraceSummaries(...) for summaries, isTraceRows(...) for true trace rows, and in the else (unsupported query shapes) return a safe fallback UI/path instead of casting to OtelTracesRow[]; update TraceDetail to use these guards and remove the unsafe cast.
🧹 Nitpick comments (2)
packages/core/src/telemetry-datasource.ts (1)
165-187: ⚡ Quick winTighten per-method return types to preserve aggregate output shape.
The specialized methods currently erase
output.typeresult distinctions (notablybucket_startfortimeSeries). ReusingKopaiQueryResult<...>keeps these methods aligned with the genericquery()contract.Suggested typing update
queryTracesRaw( q: TraceRawQuery & { requestContext?: unknown } - ): Promise<{ data: OtelTracesRow[]; nextCursor: string | null }>; + ): Promise<KopaiQueryResult<TraceRawQuery>>; queryTracesAggregate( q: TraceAggregateQuery & { requestContext?: unknown } - ): Promise<{ data: KopaiAggregateRow[] }>; + ): Promise<KopaiQueryResult<TraceAggregateQuery>>; queryLogsRaw( q: LogRawQuery & { requestContext?: unknown } - ): Promise<{ data: OtelLogsRow[]; nextCursor: string | null }>; + ): Promise<KopaiQueryResult<LogRawQuery>>; queryLogsAggregate( q: LogAggregateQuery & { requestContext?: unknown } - ): Promise<{ data: KopaiAggregateRow[] }>; + ): Promise<KopaiQueryResult<LogAggregateQuery>>; queryMetricsRaw( q: MetricRawQuery & { requestContext?: unknown } - ): Promise<{ data: OtelMetricsRow[]; nextCursor: string | null }>; + ): Promise<KopaiQueryResult<MetricRawQuery>>; queryMetricsAggregate( q: MetricAggregateQuery & { requestContext?: unknown } - ): Promise<{ data: KopaiAggregateRow[] }>; + ): Promise<KopaiQueryResult<MetricAggregateQuery>>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/telemetry-datasource.ts` around lines 165 - 187, The aggregate (and raw) method signatures currently return plain shapes that drop the KopaiQueryResult output typing (losing distinctions like output.type/bucket_start); update the signatures for queryTracesAggregate, queryLogsAggregate, queryMetricsAggregate (and likewise the raw variants queryTracesRaw, queryLogsRaw, queryMetricsRaw if you want consistent behavior) to return Promise<KopaiQueryResult<...>> (e.g., Promise<KopaiQueryResult<KopaiAggregateRow>> for aggregates and Promise<KopaiQueryResult<OtelTracesRow>>/OtelLogsRow/OtelMetricsRow for raw) instead of Promise<{ data: ...; nextCursor?: ... }>, so the methods preserve the full KopaiQueryResult shape and its output.type information.packages/sdk/src/kopai-query-builder.ts (1)
123-177:resolveColumnintentionally keeps semconv strings like"service.name"as strings (no mismatch with the test).
STRUCTURAL_SETinpackages/sdk/src/kopai-query-builder.tsis built fromkopaiQuery.TraceColumn.options, andTraceColumnincludes both structural fields and semconv dotted keys (e.g."service.name"). SoresolveColumn("service.name", "traces")returns"service.name"unchanged, matchingit("semconv string stays string").
- Minor: rename
STRUCTURAL_SETto something likeTOP_LEVEL_COLUMN_SETfor clarity (it’s not only structural).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk/src/kopai-query-builder.ts` around lines 123 - 177, The constant name STRUCTURAL_SET is misleading because it contains top-level/semconv column names; rename it to TOP_LEVEL_COLUMN_SET (update its declaration and all references, e.g., in resolveColumn and any other usages like inferContainer logic) so the identifier better reflects its contents while leaving the value/behavior unchanged; ensure you update the const name in the same module and run tests to confirm resolveColumn("service.name", "traces") still returns the string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/clickhouse-datasource/src/query-kopai.ts`:
- Around line 590-603: The cursor handling currently only validates timestamp in
parseCursor but binds c.id as UInt64 (used in nextParam calls) which can allow
non-numeric hashes to reach execution; update parseCursor (or add a validation
step right after it in the query-building code that uses parseCursor) to ensure
c.id is a valid unsigned 64-bit integer (numeric string or BigInt) before
calling nextParam with type UInt64, and if validation fails throw/return a clear
query-validation error; reference parseCursor, c.id, nextParam, and the code
paths that push the conds using sipHash64 so the invalid cursor is rejected
early rather than causing runtime execution errors.
- Around line 522-544: The cursor predicate logic always uses Timestamp (and
SpanId) regardless of the actual ORDER BY, which causes incorrect pagination
when the primary sort is not time; update the pagination to either (A) validate
and reject unsupported combos by checking the primary sort field returned by
compileOrderByRaw/inferPrimaryDir and throwing a clear error when it's not the
time column (Timestamp/TimeUnix), or (B) build the cursor predicate from the
actual primary sort expression and a stable secondary tiebreaker (use the
compiled primary column name from compileOrderByRaw and keep SpanId as the
tiebreaker) so parseCursor/nextParam/nanosToDateTime64 are used only when the
primary is a time field; modify the block that uses parseCursor, nextParam,
inferPrimaryDir, compileOrderByRaw, and ensureTiebreaker accordingly to
implement one of these fixes.
In `@packages/core/src/kopai-query.ts`:
- Around line 17-22: DurationString currently allows zero values (regex uses
\d+), which conflicts with the compiler that rejects zero durations; update the
DurationString schema (the constant DurationString) to only accept positive
integers greater than zero by changing the numeric part of the regex to a
non-zero-leading pattern (e.g. use [1-9]\d*) and adjust the error message to
state "positive integer > 0" while keeping the same unit set (s,m,h,d,w) so
schema and compiler validation match.
In `@packages/sqlite-datasource/src/query-builder.ts`:
- Around line 442-462: The code currently accepts arbitrary order clauses in raw
mode which breaks the cursor format; change the branch that handles order (the
block that iterates order and uses assertColumnRef and columnSqlExpr to build
orderParts/orderParams) to instead reject any explicit order when in raw mode by
throwing kopaiQueryCompiler.KopaiQueryValidationError (same style as the
existing measure check), so only the default ordering produced by
kopaiQueryCompiler.timeColumnForSignal(signal) plus the tiebreak (tiebreak =
signal === "traces" ? "SpanId" : "rowid") is used; in short, if order is
non-empty in raw mode, throw a validation error and do not build orderParts from
user-supplied columns.
- Around line 825-831: The normalizeCellValue function currently coerces all
bigints to Number (Number(v)), which can silently lose precision for large
aggregate results; change the bigint branch in normalizeCellValue to return a
string representation (e.g., String(v) or v.toString()) instead of Number(v) so
large integer aggregates are preserved (KopaiAggregateRow already permits
strings) and other branches remain unchanged.
- Around line 335-358: Validate the cursor parts tsStr and idStr before
attempting BigInt or parseInt: check tsStr matches /^\d+$/ and idStr matches
/^\d+$/ for non-"traces" signals, and if either fails throw
kopaiQueryCompiler.KopaiQueryValidationError with a clear message; only then
call BigInt(tsStr) and parseInt(idStr, 10) (keeping the existing
timeColumnForSignal(signal), traces/direction, SpanId logic and the existing
rowid/Number.isNaN guard) so malformed timestamps or ids produce the intended
validation error instead of raw parsing exceptions or partial parse behavior.
---
Outside diff comments:
In `@examples/ui-react-app/src/custom-observability-catalog.tsx`:
- Around line 461-467: The current type guard isTraceSummaries only narrows
summary responses, leaving the "query" branch in TraceDetail to unsafely cast to
OtelTracesRow[]; add a new type guard isTraceRows that checks
props.element.dataSource?.method === "query" and validates the response shape
(e.g., existence of expected row fields) so TraceDetail can safely branch: use
isTraceSummaries(...) for summaries, isTraceRows(...) for true trace rows, and
in the else (unsupported query shapes) return a safe fallback UI/path instead of
casting to OtelTracesRow[]; update TraceDetail to use these guards and remove
the unsafe cast.
---
Nitpick comments:
In `@packages/core/src/telemetry-datasource.ts`:
- Around line 165-187: The aggregate (and raw) method signatures currently
return plain shapes that drop the KopaiQueryResult output typing (losing
distinctions like output.type/bucket_start); update the signatures for
queryTracesAggregate, queryLogsAggregate, queryMetricsAggregate (and likewise
the raw variants queryTracesRaw, queryLogsRaw, queryMetricsRaw if you want
consistent behavior) to return Promise<KopaiQueryResult<...>> (e.g.,
Promise<KopaiQueryResult<KopaiAggregateRow>> for aggregates and
Promise<KopaiQueryResult<OtelTracesRow>>/OtelLogsRow/OtelMetricsRow for raw)
instead of Promise<{ data: ...; nextCursor?: ... }>, so the methods preserve the
full KopaiQueryResult shape and its output.type information.
In `@packages/sdk/src/kopai-query-builder.ts`:
- Around line 123-177: The constant name STRUCTURAL_SET is misleading because it
contains top-level/semconv column names; rename it to TOP_LEVEL_COLUMN_SET
(update its declaration and all references, e.g., in resolveColumn and any other
usages like inferContainer logic) so the identifier better reflects its contents
while leaving the value/behavior unchanged; ensure you update the const name in
the same module and run tests to confirm resolveColumn("service.name", "traces")
still returns the string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25b0619d-4021-4e4e-ac82-4dfdc04ff890
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (41)
examples/ui-react-app/src/custom-observability-catalog.tsxpackages/api/src/index.test.tspackages/api/src/index.tspackages/api/src/routes/error-handler.tspackages/api/src/routes/query.tspackages/api/src/signals-query.test.tspackages/api/src/signals.test.tspackages/clickhouse-datasource/src/datasource-query-six.test.tspackages/clickhouse-datasource/src/datasource-query.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/query-kopai.test.tspackages/clickhouse-datasource/src/query-kopai.tspackages/core/src/denormalized-signals-zod.tspackages/core/src/index.tspackages/core/src/kopai-query-compiler.test.tspackages/core/src/kopai-query-compiler.tspackages/core/src/kopai-query.test.tspackages/core/src/kopai-query.tspackages/core/src/telemetry-datasource.tspackages/sdk/src/client.test.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/kopai-query-builder.test.tspackages/sdk/src/kopai-query-builder.tspackages/sqlite-datasource/src/datasource-query-six.test.tspackages/sqlite-datasource/src/datasource-query.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/optimized-datasource.tspackages/sqlite-datasource/src/query-builder.test.tspackages/sqlite-datasource/src/query-builder.tspackages/ui-core/src/hooks/use-kopai-data.test.tspackages/ui-core/src/hooks/use-kopai-data.tspackages/ui-core/src/hooks/use-live-logs.test.tspackages/ui-core/src/lib/component-catalog.test.tspackages/ui-core/src/lib/component-catalog.tspackages/ui-core/src/lib/observability-catalog.tspackages/ui-core/src/providers/kopai-provider.tsxpackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/pages/observability.test.tsxskills/create-dashboard/SKILL.mdskills/create-dashboard/rules/workflow.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/ui-react-app/src/custom-observability-catalog.tsx`:
- Around line 487-489: The current type-guard only checks the first element
(data[0]) which can miss malformed later items; update the guard to verify every
element using hasTraceRowShape so the branch is truly narrowed for all rows:
keep the Array.isArray check, return true for an empty array, and replace the
single-element check with data.every(hasTraceRowShape) to validate all items
before treating data as TraceRow[].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 545e1f4e-a21c-40ce-8944-b920a0be7fc8
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
examples/ui-react-app/src/custom-observability-catalog.tsxpackages/clickhouse-datasource/src/query-kopai.test.tspackages/clickhouse-datasource/src/query-kopai.tspackages/core/src/kopai-query.test.tspackages/core/src/kopai-query.tspackages/sdk/src/kopai-query-builder.tspackages/sqlite-datasource/src/query-builder.test.tspackages/sqlite-datasource/src/query-builder.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/kopai-query.test.ts
- packages/sqlite-datasource/src/query-builder.ts
- packages/sdk/src/kopai-query-builder.ts
- packages/core/src/kopai-query.ts
- packages/clickhouse-datasource/src/query-kopai.ts
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/observability/renderers/narrowRows.test.ts`:
- Around line 26-31: The test name is misleading — it currently says "returns
null..." but the assertion expects one metric row; update the test title to
reflect that a row missing the synthetic MetricType is still considered a metric
row. Locate the test in
packages/ui/src/components/observability/renderers/narrowRows.test.ts and change
the description for the it(...) case that calls narrowRows(res,
hasMetricRowShape) so it states something like "returns row when a metric
renderer receives raw metric rows lacking the synthetic MetricType" (or
equivalent clear wording) to match the expect(...).toHaveLength(1) assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2057c8f8-ecd8-4fbc-8533-0dbc57204f30
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (28)
examples/ui-react-app/src/custom-observability-catalog.tsxpackages/clickhouse-datasource/src/datasource-coerce.test.tspackages/clickhouse-datasource/src/datasource-query.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/query-kopai.test.tspackages/clickhouse-datasource/src/query-kopai.tspackages/core/src/kopai-query-compiler.test.tspackages/core/src/kopai-query-compiler.tspackages/core/src/kopai-query.test.tspackages/core/src/kopai-query.tspackages/sdk/src/client.test.tspackages/sdk/src/client.tspackages/sdk/src/index.tspackages/sdk/src/kopai-query-builder.test.tspackages/sdk/src/kopai-query-builder.tspackages/sqlite-datasource/src/datasource-query.test.tspackages/sqlite-datasource/src/query-builder.test.tspackages/sqlite-datasource/src/query-builder.tspackages/ui-core/src/lib/renderer.test.tsxpackages/ui-core/src/lib/renderer.tsxpackages/ui/src/components/observability/renderers/OtelLogTimeline.tsxpackages/ui/src/components/observability/renderers/OtelMetricHistogram.tsxpackages/ui/src/components/observability/renderers/OtelMetricStat.tsxpackages/ui/src/components/observability/renderers/OtelMetricTable.tsxpackages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsxpackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/renderers/narrowRows.test.tspackages/ui/src/components/observability/renderers/narrowRows.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/sdk/src/index.ts
- packages/core/src/kopai-query.test.ts
- packages/sqlite-datasource/src/datasource-query.test.ts
- packages/sqlite-datasource/src/query-builder.test.ts
- packages/core/src/kopai-query-compiler.ts
- packages/sdk/src/client.test.ts
- packages/clickhouse-datasource/src/datasource-query.test.ts
- packages/clickhouse-datasource/src/datasource.ts
- packages/sdk/src/kopai-query-builder.ts
- packages/clickhouse-datasource/src/query-kopai.ts
- packages/clickhouse-datasource/src/query-kopai.test.ts
- packages/sdk/src/kopai-query-builder.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/ui/src/components/observability/renderers/narrowRows.ts (1)
105-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
TimestampinhasTraceRowShape.Right now any aggregate row that happens to include
TraceId,SpanId, and one span-only key likeSpanNamewill pass this guard, sonarrowQueryRowscan forward a wrong-shaped result instead of surfacing the mismatch error this helper was added for. Requiring the raw trace timestamp closes that hole and matches the actual trace row contract.Proposed fix
export function hasTraceRowShape(v: unknown): v is OtelTracesRow { if (!isRecord(v)) return false; // Traces require SpanId + TraceId, but so can a trace-correlated log; the // presence of a span-only structural key distinguishes a real span row. return ( + typeof v.Timestamp === "string" && typeof v.SpanId === "string" && typeof v.TraceId === "string" && typeof v.TimeUnix !== "string" && hasAnyKey(v, SPAN_ONLY_KEYS) && !hasAnyKey(v, LOG_ONLY_KEYS) ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/observability/renderers/narrowRows.ts` around lines 105 - 115, hasTraceRowShape currently lets aggregate rows through because it doesn't require the raw trace timestamp; update the predicate in hasTraceRowShape to explicitly require the trace timestamp by adding a check like typeof v.Timestamp === "string" (instead of the existing typeof v.TimeUnix !== "string"), so the function (used with OtelTracesRow and keys SPAN_ONLY_KEYS / LOG_ONLY_KEYS) only returns true when TraceId, SpanId and a properly typed Timestamp are present along with a span-only key and no log-only keys.packages/sqlite-datasource/src/db-datasource.ts (2)
1425-1436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait the runner inside
guardKopaiQuery.This wrapper only catches synchronous throws right now. The callbacks here return promises, so any rejection from
runRawTraces,runRawLogs,runRawMetrics, orrunAggregatewill bypass this envelope and escape without the advertisedSqliteDatasourceQueryErrorwrapping.Proposed fix
- private guardKopaiQuery<T>(run: () => T): T { + private async guardKopaiQuery<T>( + run: () => Promise<T> | T + ): Promise<T> { try { - return run(); + return await run(); } catch (error) { if (error instanceof SqliteDatasourceQueryError) throw error; if (error instanceof kopaiQueryCompiler.KopaiQueryValidationError) { throw error; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqlite-datasource/src/db-datasource.ts` around lines 1425 - 1436, guardKopaiQuery currently only catches synchronous throws; make it handle promise rejections by awaiting the runner: change guardKopaiQuery to an async function that returns Promise<T>, await the call to run(), and keep the same error handling logic (re-throw SqliteDatasourceQueryError and kopaiQueryCompiler.KopaiQueryValidationError, otherwise throw new SqliteDatasourceQueryError with the original error as cause). Update all callers (e.g., runRawTraces, runRawLogs, runRawMetrics, runAggregate) to await the returned promise if necessary.
1274-1295: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the compiler-owned error-status literal here as well.
These summary queries still hardcode
"Error"even though the rest of this PR moves status-name mapping behindkopaiQueryCompiler. Leaving SQLite on a raw string makeserrorCount/hasErrorthe next place backend behavior drifts if that canonical literal changes again.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sqlite-datasource/src/db-datasource.ts` around lines 1274 - 1295, The queries aggSql and svcSql currently hardcode the status string "Error"; replace that literal with the compiler-owned error-status literal from kopaiQueryCompiler (e.g., kopaiQueryCompiler.errorStatusLiteral or the actual property name used on the compiler instance) so SQLite uses the canonical status name; update the template strings for aggSql and svcSql to interpolate that compiler-provided literal (or pass it as a bound parameter) instead of "Error", and keep the rest of the logic that uses aggSql, svcSql and this.sqliteConnection.prepare(...).all(..., ...traceIds) intact.
🧹 Nitpick comments (2)
packages/ui/src/components/observability/TraceTimeline/index.tsx (1)
102-103: ⚡ Quick winConsider adding case normalization at the data boundary.
These defaults now use title-case (
"Internal","Unset"), but ifrow.SpanKindorrow.StatusCodearrive from the backend in uppercase (e.g.,"INTERNAL","UNSET","ERROR"), the UI comparisons throughout the codebase will fail. This is the critical transform point between backend data and UI models.🔄 Add normalization helper if backend uses different casing
If verification reveals the backend sends uppercase values, add normalization here:
+function normalizeStatus(status: string | undefined): SpanNode["status"] | undefined { + if (!status) return undefined; + // Map OpenTelemetry uppercase to title-case + const statusMap: Record<string, SpanNode["status"]> = { + "UNSET": "Unset", + "OK": "Ok", + "ERROR": "Error", + }; + return statusMap[status.toUpperCase()] ?? status as SpanNode["status"]; +} + +function normalizeKind(kind: string | undefined): string | undefined { + if (!kind) return undefined; + // Capitalize first letter, lowercase rest + return kind.charAt(0).toUpperCase() + kind.slice(1).toLowerCase(); +} const span: SpanNode = { spanId: row.SpanId, parentSpanId: row.ParentSpanId || undefined, traceId: row.TraceId, name: row.SpanName ?? "", startTimeUnixMs: startMs, endTimeUnixMs: endMs, durationMs, - kind: row.SpanKind ?? "Internal", - status: row.StatusCode ?? "Unset", + kind: normalizeKind(row.SpanKind) ?? "Internal", + status: normalizeStatus(row.StatusCode) ?? "Unset", statusMessage: row.StatusMessage,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/observability/TraceTimeline/index.tsx` around lines 102 - 103, The mapping that sets kind: row.SpanKind ?? "Internal" and status: row.StatusCode ?? "Unset" must normalize incoming casing so UI comparisons work regardless of backend case; update the transform in TraceTimeline (index.tsx) to canonicalize row.SpanKind and row.StatusCode (e.g., trim + toLowerCase/upperCase then map to the UI enum or title-case) and/or add small helpers (normalizeSpanKind, normalizeStatus) used in this mapping to return the expected "Internal"/"Unset"/"Error" tokens while still falling back to the defaults.packages/clickhouse-datasource/src/datasource.test.ts (1)
1614-1699: ⚡ Quick winAssert the provisioning side effect explicitly.
This suite still passes if
discoverMetrics()falls back to full-table scans, because it only checks returned metrics. Since this describe is meant to pin the new on-demand provisioning path, add a postcondition after the first call thatDISCOVER_NAMES_TABLE,DISCOVER_ATTRS_TABLE, and at least one discover MV now exist. Otherwise a regression in the create/backfill path can slip through unnoticed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/clickhouse-datasource/src/datasource.test.ts` around lines 1614 - 1699, The test only asserts returned metrics but not that on-demand provisioning actually created the discovery tables/MVs; after calling ds.discoverMetrics() in the provisioning tests (the ones using dropDiscoverMVs and the "provisions…" cases), add assertions that DISCOVER_NAMES_TABLE and DISCOVER_ATTRS_TABLE now exist and that at least one of the materialized views from getDiscoverMVSchema(TEST_DATABASE).materializedViews was created (use adminClient to check existence via system.tables or a DROP/EXISTS-style query); place these checks immediately after the first discoverMetrics() call in the relevant it blocks to verify the create/backfill side-effect for discoverMetrics and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/clickhouse-datasource/src/datasource.ts`:
- Around line 1082-1099: The current discoverMetrics request path calls
provisionDiscoverMVs and backfills (provisionDiscoverMVs, readDiscoverMVs) when
CH_ERR_TABLE_NOT_FOUND, which can block/timeout and cause stampedes; change this
to not perform DDL/backfill inline: instead detect the missing MVs, log and
return a controlled error/placeholder response to the client (or a fast empty
result) and enqueue a single background/one-time job (use a cross-request
mutex/leader-election or an in-memory/Redis/DB lock keyed by database + function
like provisionDiscoverMVs) to perform provisioning/backfill off-request,
ensuring concurrent discoverMetrics calls observe the lock/flag and do not
re-queue the job; alternatively move provisioning to startup/admin flow and call
provisionDiscoverMVs only from that background/administrative path.
---
Outside diff comments:
In `@packages/sqlite-datasource/src/db-datasource.ts`:
- Around line 1425-1436: guardKopaiQuery currently only catches synchronous
throws; make it handle promise rejections by awaiting the runner: change
guardKopaiQuery to an async function that returns Promise<T>, await the call to
run(), and keep the same error handling logic (re-throw
SqliteDatasourceQueryError and kopaiQueryCompiler.KopaiQueryValidationError,
otherwise throw new SqliteDatasourceQueryError with the original error as
cause). Update all callers (e.g., runRawTraces, runRawLogs, runRawMetrics,
runAggregate) to await the returned promise if necessary.
- Around line 1274-1295: The queries aggSql and svcSql currently hardcode the
status string "Error"; replace that literal with the compiler-owned error-status
literal from kopaiQueryCompiler (e.g., kopaiQueryCompiler.errorStatusLiteral or
the actual property name used on the compiler instance) so SQLite uses the
canonical status name; update the template strings for aggSql and svcSql to
interpolate that compiler-provided literal (or pass it as a bound parameter)
instead of "Error", and keep the rest of the logic that uses aggSql, svcSql and
this.sqliteConnection.prepare(...).all(..., ...traceIds) intact.
In `@packages/ui/src/components/observability/renderers/narrowRows.ts`:
- Around line 105-115: hasTraceRowShape currently lets aggregate rows through
because it doesn't require the raw trace timestamp; update the predicate in
hasTraceRowShape to explicitly require the trace timestamp by adding a check
like typeof v.Timestamp === "string" (instead of the existing typeof v.TimeUnix
!== "string"), so the function (used with OtelTracesRow and keys SPAN_ONLY_KEYS
/ LOG_ONLY_KEYS) only returns true when TraceId, SpanId and a properly typed
Timestamp are present along with a span-only key and no log-only keys.
---
Nitpick comments:
In `@packages/clickhouse-datasource/src/datasource.test.ts`:
- Around line 1614-1699: The test only asserts returned metrics but not that
on-demand provisioning actually created the discovery tables/MVs; after calling
ds.discoverMetrics() in the provisioning tests (the ones using dropDiscoverMVs
and the "provisions…" cases), add assertions that DISCOVER_NAMES_TABLE and
DISCOVER_ATTRS_TABLE now exist and that at least one of the materialized views
from getDiscoverMVSchema(TEST_DATABASE).materializedViews was created (use
adminClient to check existence via system.tables or a DROP/EXISTS-style query);
place these checks immediately after the first discoverMetrics() call in the
relevant it blocks to verify the create/backfill side-effect for discoverMetrics
and prevent regressions.
In `@packages/ui/src/components/observability/TraceTimeline/index.tsx`:
- Around line 102-103: The mapping that sets kind: row.SpanKind ?? "Internal"
and status: row.StatusCode ?? "Unset" must normalize incoming casing so UI
comparisons work regardless of backend case; update the transform in
TraceTimeline (index.tsx) to canonicalize row.SpanKind and row.StatusCode (e.g.,
trim + toLowerCase/upperCase then map to the UI enum or title-case) and/or add
small helpers (normalizeSpanKind, normalizeStatus) used in this mapping to
return the expected "Internal"/"Unset"/"Error" tokens while still falling back
to the defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 715e4a2c-ada2-4050-8ac0-aa8b833a34b5
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
.changeset/sdk-error-detail-message.md.changeset/simplified-query.md.changeset/ui-core-dedup.mdpackages/clickhouse-datasource/src/datasource-coerce.test.tspackages/clickhouse-datasource/src/datasource-query-six.test.tspackages/clickhouse-datasource/src/datasource-query.test.tspackages/clickhouse-datasource/src/datasource.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/discover-mv-schema.tspackages/clickhouse-datasource/src/e2e.integration.test.tspackages/clickhouse-datasource/src/query-traces.tspackages/clickhouse-datasource/src/test/otel-ddl.tspackages/core/src/denormalized-signals-zod.tspackages/core/src/kopai-query-compiler.tspackages/core/src/kopai-query.tspackages/sdk/src/client.test.tspackages/sdk/src/kopai-query-builder.test.tspackages/sdk/src/request.tspackages/sqlite-datasource/src/datasource-query.test.tspackages/sqlite-datasource/src/datasource-read.test.tspackages/sqlite-datasource/src/datasource-write.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/query-builder.test.tspackages/sqlite-datasource/src/query-builder.tspackages/ui-core/src/lib/component-catalog.test.tspackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/Minimap.tsxpackages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsxpackages/ui/src/components/observability/TraceTimeline/SpanRow.tsxpackages/ui/src/components/observability/TraceTimeline/TimelineBar.tsxpackages/ui/src/components/observability/TraceTimeline/index.tsxpackages/ui/src/components/observability/__fixtures__/trace-summaries.tspackages/ui/src/components/observability/__fixtures__/traces.tspackages/ui/src/components/observability/renderers/OtelMetricHistogram.tsxpackages/ui/src/components/observability/renderers/OtelMetricStat.tsxpackages/ui/src/components/observability/renderers/OtelMetricTable.tsxpackages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsxpackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/renderers/narrowRows.test.tspackages/ui/src/components/observability/renderers/narrowRows.tspackages/ui/src/components/observability/types.tspackages/ui/src/pages/observability.tsx
💤 Files with no reviewable changes (1)
- packages/ui-core/src/lib/component-catalog.test.ts
✅ Files skipped from review due to trivial changes (5)
- packages/ui/src/components/observability/types.ts
- .changeset/sdk-error-detail-message.md
- .changeset/ui-core-dedup.md
- .changeset/simplified-query.md
- packages/ui/src/components/observability/fixtures/trace-summaries.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/ui/src/components/observability/renderers/OtelMetricTable.tsx
- packages/ui/src/components/observability/renderers/OtelTraceDetail.tsx
- packages/ui/src/components/observability/renderers/OtelMetricStat.tsx
- packages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsx
- packages/ui/src/components/observability/renderers/OtelMetricHistogram.tsx
- packages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsx
- packages/core/src/denormalized-signals-zod.ts
- packages/sdk/src/client.test.ts
- packages/core/src/kopai-query-compiler.ts
- packages/clickhouse-datasource/src/datasource-query-six.test.ts
- packages/clickhouse-datasource/src/datasource-query.test.ts
- packages/sqlite-datasource/src/query-builder.test.ts
- packages/sqlite-datasource/src/query-builder.ts
- packages/sqlite-datasource/src/datasource-query.test.ts
- packages/core/src/kopai-query.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/clickhouse-datasource/src/datasource.ts (1)
59-94: ⚡ Quick winConsider dedicated unit tests for the complex numeric coercion logic.
coerceMeasureCellValuehandles several edge cases: bigint overflow detection, integer vs. float string parsing, and NaN fallback. While the logic appears correct and is exercised through integration tests, dedicated unit tests would provide additional confidence for this exported function, especially for the precision-preserving bigint paths.Example test cases to consider
// Bigint edge cases coerceMeasureCellValue(String(Number.MAX_SAFE_INTEGER + 1)) // should return string coerceMeasureCellValue(String(Number.MAX_SAFE_INTEGER)) // should return number coerceMeasureCellValue("9007199254740992") // boundary check // Float strings coerceMeasureCellValue("123.456") // should return 123.456 // Non-numeric coerceMeasureCellValue("abc") // should return "abc" // ClickHouse UInt64 strings (common case) coerceMeasureCellValue("42") // should return 42🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/clickhouse-datasource/src/datasource.ts` around lines 59 - 94, Add dedicated unit tests for the exported function coerceMeasureCellValue to cover bigint overflow detection, integer vs float string parsing, NaN fallback, and boolean/null handling; specifically write tests that assert String(Number.MAX_SAFE_INTEGER + 1) and "9007199254740992" remain strings, String(Number.MAX_SAFE_INTEGER) and "42" convert to numbers, "123.456" converts to a numeric float, non-numeric "abc" returns the original string, boolean true/false map to 1/0, null/undefined return null, and BigInt inputs above/below Number.MAX_SAFE_INTEGER/Number.MIN_SAFE_INTEGER follow the same string/number behavior so the function (coerceMeasureCellValue) behavior is fully covered and guarded against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/clickhouse-datasource/src/datasource.test.ts`:
- Around line 2003-2014: The test suite has a contradictory contract for
discoverMetrics: one test (this block using tenantBRequestContext and
discoverMetrics) expects runtime MV provisioning, while other tests assert
discoverMetrics must be read-only and fall back to base-table scans when MVs are
absent; pick one behavior and make the suite consistent. To fix, either update
this test to assert the read-only fallback (remove expectations that MVs are
provisioned and that tenant B gets newly backfilled tables) or change the
implementation of discoverMetrics to perform tenant-scoped MV provisioning and
update the other tests that assert read-only behavior; locate usages of
discoverMetrics and tenantBRequestContext in the suite and align all assertions
to the chosen contract. Ensure any test that assumes provisioning explicitly
mocks or sets the provisioning flag so behavior is deterministic.
---
Nitpick comments:
In `@packages/clickhouse-datasource/src/datasource.ts`:
- Around line 59-94: Add dedicated unit tests for the exported function
coerceMeasureCellValue to cover bigint overflow detection, integer vs float
string parsing, NaN fallback, and boolean/null handling; specifically write
tests that assert String(Number.MAX_SAFE_INTEGER + 1) and "9007199254740992"
remain strings, String(Number.MAX_SAFE_INTEGER) and "42" convert to numbers,
"123.456" converts to a numeric float, non-numeric "abc" returns the original
string, boolean true/false map to 1/0, null/undefined return null, and BigInt
inputs above/below Number.MAX_SAFE_INTEGER/Number.MIN_SAFE_INTEGER follow the
same string/number behavior so the function (coerceMeasureCellValue) behavior is
fully covered and guarded against regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59d29c98-2272-428f-940e-13ac873e6ff6
⛔ Files ignored due to path filters (1)
packages/ui-core/src/lib/__snapshots__/generate-prompt-instructions.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
.changeset/sdk-error-detail-message.md.changeset/simplified-query.md.changeset/ui-core-dedup.mdpackages/clickhouse-datasource/src/datasource-coerce.test.tspackages/clickhouse-datasource/src/datasource-query-six.test.tspackages/clickhouse-datasource/src/datasource-query.test.tspackages/clickhouse-datasource/src/datasource.test.tspackages/clickhouse-datasource/src/datasource.tspackages/clickhouse-datasource/src/discover-mv-schema.tspackages/clickhouse-datasource/src/e2e.integration.test.tspackages/clickhouse-datasource/src/query-traces.tspackages/clickhouse-datasource/src/test/otel-ddl.tspackages/core/src/denormalized-signals-zod.tspackages/core/src/kopai-query-compiler.tspackages/core/src/kopai-query.tspackages/sdk/src/client.test.tspackages/sdk/src/kopai-query-builder.test.tspackages/sdk/src/request.tspackages/sqlite-datasource/src/datasource-query.test.tspackages/sqlite-datasource/src/datasource-read.test.tspackages/sqlite-datasource/src/datasource-write.test.tspackages/sqlite-datasource/src/db-datasource.tspackages/sqlite-datasource/src/query-builder.test.tspackages/sqlite-datasource/src/query-builder.tspackages/ui-core/src/lib/component-catalog.test.tspackages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsxpackages/ui/src/components/observability/TraceTimeline/GraphView.tsxpackages/ui/src/components/observability/TraceTimeline/Minimap.tsxpackages/ui/src/components/observability/TraceTimeline/SpanDetailInline.tsxpackages/ui/src/components/observability/TraceTimeline/SpanRow.tsxpackages/ui/src/components/observability/TraceTimeline/TimelineBar.tsxpackages/ui/src/components/observability/TraceTimeline/index.tsxpackages/ui/src/components/observability/__fixtures__/trace-summaries.tspackages/ui/src/components/observability/__fixtures__/traces.tspackages/ui/src/components/observability/renderers/OtelMetricHistogram.tsxpackages/ui/src/components/observability/renderers/OtelMetricStat.tsxpackages/ui/src/components/observability/renderers/OtelMetricTable.tsxpackages/ui/src/components/observability/renderers/OtelMetricTimeSeries.tsxpackages/ui/src/components/observability/renderers/OtelTraceDetail.tsxpackages/ui/src/components/observability/renderers/narrowRows.test.tspackages/ui/src/components/observability/renderers/narrowRows.tspackages/ui/src/components/observability/types.tspackages/ui/src/pages/observability.tsx
💤 Files with no reviewable changes (1)
- packages/ui-core/src/lib/component-catalog.test.ts
✅ Files skipped from review due to trivial changes (3)
- .changeset/ui-core-dedup.md
- packages/ui/src/components/observability/types.ts
- .changeset/sdk-error-detail-message.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/clickhouse-datasource/src/datasource-coerce.test.ts
- packages/ui/src/components/observability/renderers/OtelMetricTable.tsx
- packages/ui/src/components/observability/DynamicDashboard/DynamicDashboard.test.tsx
- packages/ui/src/components/observability/renderers/OtelTraceDetail.tsx
- packages/core/src/denormalized-signals-zod.ts
- packages/sqlite-datasource/src/db-datasource.ts
- packages/sdk/src/kopai-query-builder.test.ts
- packages/sqlite-datasource/src/query-builder.ts
- packages/sqlite-datasource/src/datasource-query.test.ts
Comment on lines
+2003
to
2014
| it("discoverMetrics provisions MVs on demand for a tenant without them", async () => { | ||
| // Tenant B has no discover MV tables — discoverMetrics must provision + | ||
| // backfill them in tenant B's database (not 500), then serve its own | ||
| // metrics only (tenant-isolated). | ||
| const tenantB = await ds.discoverMetrics({ | ||
| requestContext: tenantBRequestContext(), | ||
| }); | ||
| const names = tenantB.metrics.map((m) => m.name); | ||
| expect(names).toContain("tenant.b.gauge"); | ||
| // Tenant A's metrics must NOT leak into tenant B's discovery. | ||
| expect(names).not.toContain("system.cpu.utilization"); | ||
| }); |
There was a problem hiding this comment.
Resolve the contradictory discoverMetrics contract.
This test expects request-time MV provisioning, but Lines 1614-1714 explicitly assert the opposite contract: when discovery MVs are absent, discoverMetrics must stay read-only and fall back to a base-table scan. Both expectations cannot be true for the same method without an unshown tenant-specific branch, so one side of the suite is asserting the wrong behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/clickhouse-datasource/src/datasource.test.ts` around lines 2003 -
2014, The test suite has a contradictory contract for discoverMetrics: one test
(this block using tenantBRequestContext and discoverMetrics) expects runtime MV
provisioning, while other tests assert discoverMetrics must be read-only and
fall back to base-table scans when MVs are absent; pick one behavior and make
the suite consistent. To fix, either update this test to assert the read-only
fallback (remove expectations that MVs are provisioned and that tenant B gets
newly backfilled tables) or change the implementation of discoverMetrics to
perform tenant-scoped MV provisioning and update the other tests that assert
read-only behavior; locate usages of discoverMetrics and tenantBRequestContext
in the suite and align all assertions to the chosen contract. Ensure any test
that assumes provisioning explicitly mocks or sets the provisioning flag so
behavior is deterministic.
Vunovati
force-pushed
the
feat/simplified-query
branch
from
1599cd0 to
ad5ab71
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
UI
API
Reliability
Tests
Documentation