fix analytics queries#1141
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdd a retry wrapper around ClickHouse profiling queries to enforce exactly one result with backoff retries; update several GitHub Actions workflows to use Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile OverviewGreptile SummaryModified the Key Changes:
Concerns:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant getQueryTimingStats
participant ClickHouse as ClickHouse Cluster
participant QueryLog as system.query_log
Client->>getQueryTimingStats: Request timing stats for queryId
getQueryTimingStats->>ClickHouse: SYSTEM FLUSH LOGS (admin auth)
ClickHouse-->>getQueryTimingStats: Logs flushed
getQueryTimingStats->>ClickHouse: Query clusterAllReplicas('default', system.query_log)
ClickHouse->>QueryLog: Fetch from all replicas
QueryLog-->>ClickHouse: ProfileEvents data
ClickHouse-->>getQueryTimingStats: cpu_time_ms, wall_clock_time_ms
alt Query log entry found
getQueryTimingStats-->>Client: Return timing stats
else No entry found
getQueryTimingStats-->>Client: Throw StackAssertionError
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/src/lib/clickhouse.tsx (1)
52-58: Make cluster name configurable and align log flush with clustered query.Hardcoding
clusterAllReplicas('default', system.query_log)will fail on deployments with non-default cluster names, and the localSYSTEM FLUSH LOGSdoesn't flush logs from other replicas, potentially missing query statistics. Configure the cluster name via environment variable and useSYSTEM FLUSH LOGS ON CLUSTERwhen clustering is enabled.🔧 Proposed fix (configurable cluster + cluster-aware flush)
const clickhouseExternalPassword = getEnvVariable("STACK_CLICKHOUSE_EXTERNAL_PASSWORD", ""); const clickhouseDefaultDatabase = getEnvVariable("STACK_CLICKHOUSE_DATABASE", "default"); +const clickhouseCluster = getEnvVariable("STACK_CLICKHOUSE_CLUSTER", ""); const HAS_CLICKHOUSE = !!clickhouseUrl && !!clickhouseAdminPassword && !!clickhouseExternalPassword;await client.exec({ - query: "SYSTEM FLUSH LOGS", + query: clickhouseCluster + ? `SYSTEM FLUSH LOGS ON CLUSTER '${clickhouseCluster}'` + : "SYSTEM FLUSH LOGS", auth: { username: clickhouseAdminUser, password: clickhouseAdminPassword, }, }); + const queryLogSource = clickhouseCluster + ? `clusterAllReplicas('${clickhouseCluster}', system.query_log)` + : "system.query_log"; const profile = await client.query({ query: ` SELECT ProfileEvents['CPUTimeMicroseconds'] / 1000 AS cpu_time_ms, ProfileEvents['RealTimeMicroseconds'] / 1000 AS wall_clock_time_ms - FROM clusterAllReplicas('default', system.query_log) + FROM ${queryLogSource} WHERE query_id = {query_id:String} AND type = 'QueryFinish' ORDER BY event_time DESC LIMIT 1 `,
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/src/lib/clickhouse.tsx`:
- Around line 51-67: The queryProfile function's ClickHouse query hard-codes
clusterAllReplicas('default', ...) and risks per-replica LIMIT pushdown; update
the client.query call to use the configured cluster name (use the existing
cluster variable or an env var like CLICKHOUSE_CLUSTER instead of 'default') and
append "SETTINGS distributed_push_down_limit = 0" to the SQL to force the LIMIT
on the initiator; modify the query string in queryProfile accordingly so it
references the configurable cluster name and includes the SETTINGS clause.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/e2e/tests/backend/endpoints/api/v1/analytics-events.test.ts (1)
64-70: Fix inconsistentteam_idexpectation in the second test.The backend explicitly sets
team_id: nullfor token refresh events (seeapps/backend/src/lib/events.tsx:255), yet the "cannot read events from other projects" test expectsteam_id: ""(line 55). The first test correctly expectsnull. Update the snapshot in the second test to match the backend behavior:Snapshot change required
{ "branch_id": "main", "event_type": "$token-refresh", "project_id": "<stripped UUID>", - "team_id": "", + "team_id": null, "user_id": "<stripped UUID>", },
🧹 Nitpick comments (1)
apps/e2e/tests/backend/endpoints/api/v1/analytics-events.test.ts (1)
141-152: Consider typing therowparameter instead of usingany.Lines 143 and 152 use
(row: any)without explanation. Per coding guidelines, avoidanyor leave a comment explaining why the type system fails. A simple inline type would improve type safety:type AnalyticsRow = { user_id: string; event_type: string; project_id: string; branch_id: string; team_id: string | null };♻️ Suggested improvement
+type AnalyticsRow = { user_id: string; event_type: string; project_id: string; branch_id: string; team_id: string | null }; + // Then use: - expect(userAResults.every((row: any) => row.user_id === userA)).toBe(true); + expect(userAResults.every((row: AnalyticsRow) => row.user_id === userA)).toBe(true);As per coding guidelines: "Avoid the
anytype. When necessary, leave a comment explaining why it's being used."
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/setup-tests.yaml:
- Line 46: The workflow step uses an invalid command string "pnpm test run
--reporter=verbose" which passes an extra "run" to the test script; update that
step to call "pnpm test --reporter=verbose" (or "pnpm run test
--reporter=verbose") so the package script is invoked correctly—replace the
incorrect command token in the workflow step that currently contains "pnpm test
run --reporter=verbose".
2 participants
Summary by CodeRabbit
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.