chore: switch clickhouse reads to new tables#4262
Merged
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughReplaces many ClickHouse v1/v2/v3 migration files with code updates that switch queries to versioned Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as AnalyticsHandler
participant Bucketer as BucketChooser
participant CH as ClickHouse
Client->>API: GET /v1/analytics.getVerifications (start,end,groupBy)
API->>Bucketer: compute queryStart/queryEnd, generateStart/generateEnd
Bucketer-->>API: choose table & time-filter (hour/day/month) [uses toStartOfDay/toStartOfMonth]
API->>CH: SELECT ... FROM default.key_verifications_per_*_v2 WHERE time BETWEEN queryStart and queryEnd
CH-->>API: rows (buckets + counts)
API->>API: align/normalize buckets and validate MV sync (bucket date checks)
API-->>Client: JSON response (buckets, totals)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to review closely:
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧠 Learnings (4)📓 Common learnings📚 Learning: 2025-10-21T09:45:47.560ZApplied to files:
📚 Learning: 2025-08-14T16:25:48.167ZApplied to files:
📚 Learning: 2024-10-20T07:05:55.471ZApplied to files:
🧬 Code graph analysis (2)apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts (1)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (1)
⏰ 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). (3)
🔇 Additional comments (1)
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 |
chronark
requested review from
Flo4604,
MichaelUnkey,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/engineering/content/docs/architecture/services/clickhouse.mdx (1)
173-173: Documentation examples lack schema prefix consistency.The debugging examples use
raw_key_verifications_raw_v2without any schema prefix. However, actual code shows mixed patterns: Go tests usedefault.key_verifications_raw_v2, some TypeScript usesdefault.key_verifications_raw_v2, while other TypeScript omits the prefix entirely. Consider adding a note about when schema prefixes are required versus optional.Consider adding documentation clarifying schema prefix usage:
If you need to understand or optimize a query: 1. Use the client's query method directly for custom queries: ```typescript const result = await ch.querier.query({ - query: `SELECT count() FROM raw_key_verifications_raw_v2 WHERE workspace_id = {workspaceId: String}`, + query: `SELECT count() FROM default.raw_key_verifications_raw_v2 WHERE workspace_id = {workspaceId: String}`, params: z.object({ workspaceId: z.string() }), schema: z.object({ count: z.number() }) })({ workspaceId: "ws_123" }); ``` + + Note: The `default.` schema prefix can be omitted if your client is configured with a default database.Also applies to: 182-183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (68)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts(1 hunks)apps/api/src/routes/v1_analytics_getVerifications.ts(1 hunks)apps/engineering/content/docs/architecture/services/analytics.mdx(1 hunks)apps/engineering/content/docs/architecture/services/clickhouse.mdx(2 hunks)apps/engineering/content/docs/contributing/pull-request-checks.mdx(6 hunks)go/apps/api/integration/multi_node_ratelimiting/run.go(2 hunks)go/apps/api/integration/multi_node_usagelimiting/run.go(2 hunks)go/apps/api/routes/v2_ratelimit_limit/200_test.go(1 hunks)go/pkg/clickhouse/billable_ratelimits.go(1 hunks)go/pkg/clickhouse/billable_verifications.go(1 hunks)internal/clickhouse/schema/000_README.md(0 hunks)internal/clickhouse/schema/001_create_databases.sql(0 hunks)internal/clickhouse/schema/002_create_metrics_raw_api_requests_v1.sql(0 hunks)internal/clickhouse/schema/003_create_verifications_raw_key_verifications_v1.sql(0 hunks)internal/clickhouse/schema/004_create_verifications_key_verifications_per_hour_v1.sql(0 hunks)internal/clickhouse/schema/005_create_verifications_key_verifications_per_day_v1.sql(0 hunks)internal/clickhouse/schema/006_create_verifications_key_verifications_per_month_v1.sql(0 hunks)internal/clickhouse/schema/007_create_verifications_key_verifications_per_hour_mv_v1.sql(0 hunks)internal/clickhouse/schema/008_create_verifications_key_verifications_per_day_mv_v1.sql(0 hunks)internal/clickhouse/schema/009_create_verifications_key_verifications_per_month_mv_v1.sql(0 hunks)internal/clickhouse/schema/010_create_ratelimits_raw_ratelimits_table.sql(0 hunks)internal/clickhouse/schema/011_create_telemetry_raw_sdks_v1.sql(0 hunks)internal/clickhouse/schema/012_create_billing_billable_verifications_per_month_v1.sql(0 hunks)internal/clickhouse/schema/013_create_billing_billable_verifications_per_month_mv_v1.sql(0 hunks)internal/clickhouse/schema/014_create_ratelimits_ratelimits_per_minute_v1.sql(0 hunks)internal/clickhouse/schema/015_create_ratelimits_ratelimits_per_hour_v1.sql(0 hunks)internal/clickhouse/schema/016_create_ratelimits_ratelimits_per_day_v1.sql(0 hunks)internal/clickhouse/schema/017_create_ratelimits_ratelimits_per_month_v1.sql(0 hunks)internal/clickhouse/schema/018_create_ratelimits_ratelimits_per_minute_mv_v1.sql(0 hunks)internal/clickhouse/schema/019_create_ratelimits_ratelimits_per_hour_mv_v1.sql(0 hunks)internal/clickhouse/schema/020_create_ratelimits_ratelimits_per_day_mv_v1.sql(0 hunks)internal/clickhouse/schema/021_create_ratelimits_ratelimits_per_month_mv_v1.sql(0 hunks)internal/clickhouse/schema/022_create_business_active_workspaces_per_month_v1.sql(0 hunks)internal/clickhouse/schema/023_create_business_active_workspaces_per_month_mv_v1.sql(0 hunks)internal/clickhouse/schema/024_create_ratelimits_last_used_mv_v1.sql(0 hunks)internal/clickhouse/schema/025_create_billable_verifications_v2.sql(0 hunks)internal/clickhouse/schema/026_create_billable_ratelimits_v1.sql(0 hunks)internal/clickhouse/schema/027_add_tags_to_verifications.raw_key_verifications_v1.sql(0 hunks)internal/clickhouse/schema/028_create_verifications.key_verifications_per_hour_v2.sql(0 hunks)internal/clickhouse/schema/029_create_verifications.key_verifications_per_hour_mv_v2.sql(0 hunks)internal/clickhouse/schema/030_create_verifications.key_verifications_per_day_v2.sql(0 hunks)internal/clickhouse/schema/031_create_verifications.key_verifications_per_day_mv_v2.sql(0 hunks)internal/clickhouse/schema/032_create_verifications.key_verifications_per_month_v2.sql(0 hunks)internal/clickhouse/schema/033_create_verifications.key_verifications_per_month_mv_v2.sql(0 hunks)internal/clickhouse/schema/034_billing_read_from_verifications.key_verifications_per_month_v2.sql(0 hunks)internal/clickhouse/schema/035_business_update_active_workspaces_keys_per_month_mv_v1_read_from_verifications.key_verifications_per_month_v2.sql(0 hunks)internal/clickhouse/schema/036_create_verifications.key_verifications_per_hour_v3.sql(0 hunks)internal/clickhouse/schema/037_create_verifications.key_verifications_per_hour_mv_v3.sql(0 hunks)internal/clickhouse/schema/038_create_verifications.key_verifications_per_day_v3.sql(0 hunks)internal/clickhouse/schema/039_create_verifications.key_verifications_per_day_mv_v3.sql(0 hunks)internal/clickhouse/schema/040_create_verifications.key_verifications_per_month_v3.sql(0 hunks)internal/clickhouse/schema/041_create_verifications.key_verifications_per_month_mv_v3.sql(0 hunks)internal/clickhouse/schema/042_create_api_requests_per_hour_v1.sql(0 hunks)internal/clickhouse/schema/043_create_api_requests_per_hour_mv_v1.sql(0 hunks)internal/clickhouse/schema/044_create_api_requests_per_minute_v1.sql(0 hunks)internal/clickhouse/schema/045_create_api_requests_per_minute_mv_v1.sql(0 hunks)internal/clickhouse/schema/046_create_api_requests_per_day_v1.sql(0 hunks)internal/clickhouse/schema/047_create_api_requests_per_day_mv_v1.sql(0 hunks)internal/clickhouse/schema/048_raw_ratelimits_metrics_indexes_v1.sql(0 hunks)internal/clickhouse/schema/049_raw_api_metrics_ratelimit_indexes_v1.sql(0 hunks)internal/clickhouse/schema/050_create_verifications.key_verifications_per_minute_v1.sql(0 hunks)internal/clickhouse/schema/051_create_verifications.key_verifications_per_minute_mv_v1.sql(0 hunks)internal/clickhouse/src/billing.ts(1 hunks)internal/clickhouse/src/logs-timeseries.test.ts(1 hunks)internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts(3 hunks)tools/migrate/ch_logs.ts(1 hunks)tools/migrate/ch_migrate_v2_simple.ts(0 hunks)tools/migrate/v1_deprecation.ts(2 hunks)
💤 Files with no reviewable changes (53)
- internal/clickhouse/schema/020_create_ratelimits_ratelimits_per_day_mv_v1.sql
- internal/clickhouse/schema/025_create_billable_verifications_v2.sql
- internal/clickhouse/schema/004_create_verifications_key_verifications_per_hour_v1.sql
- internal/clickhouse/schema/040_create_verifications.key_verifications_per_month_v3.sql
- internal/clickhouse/schema/042_create_api_requests_per_hour_v1.sql
- internal/clickhouse/schema/014_create_ratelimits_ratelimits_per_minute_v1.sql
- internal/clickhouse/schema/051_create_verifications.key_verifications_per_minute_mv_v1.sql
- internal/clickhouse/schema/006_create_verifications_key_verifications_per_month_v1.sql
- tools/migrate/ch_migrate_v2_simple.ts
- internal/clickhouse/schema/033_create_verifications.key_verifications_per_month_mv_v2.sql
- internal/clickhouse/schema/024_create_ratelimits_last_used_mv_v1.sql
- internal/clickhouse/schema/011_create_telemetry_raw_sdks_v1.sql
- internal/clickhouse/schema/008_create_verifications_key_verifications_per_day_mv_v1.sql
- internal/clickhouse/schema/021_create_ratelimits_ratelimits_per_month_mv_v1.sql
- internal/clickhouse/schema/027_add_tags_to_verifications.raw_key_verifications_v1.sql
- internal/clickhouse/schema/044_create_api_requests_per_minute_v1.sql
- internal/clickhouse/schema/034_billing_read_from_verifications.key_verifications_per_month_v2.sql
- internal/clickhouse/schema/005_create_verifications_key_verifications_per_day_v1.sql
- internal/clickhouse/schema/038_create_verifications.key_verifications_per_day_v3.sql
- internal/clickhouse/schema/019_create_ratelimits_ratelimits_per_hour_mv_v1.sql
- internal/clickhouse/schema/046_create_api_requests_per_day_v1.sql
- internal/clickhouse/schema/022_create_business_active_workspaces_per_month_v1.sql
- internal/clickhouse/schema/015_create_ratelimits_ratelimits_per_hour_v1.sql
- internal/clickhouse/schema/000_README.md
- internal/clickhouse/schema/007_create_verifications_key_verifications_per_hour_mv_v1.sql
- internal/clickhouse/schema/003_create_verifications_raw_key_verifications_v1.sql
- internal/clickhouse/schema/018_create_ratelimits_ratelimits_per_minute_mv_v1.sql
- internal/clickhouse/schema/031_create_verifications.key_verifications_per_day_mv_v2.sql
- internal/clickhouse/schema/035_business_update_active_workspaces_keys_per_month_mv_v1_read_from_verifications.key_verifications_per_month_v2.sql
- internal/clickhouse/schema/016_create_ratelimits_ratelimits_per_day_v1.sql
- internal/clickhouse/schema/017_create_ratelimits_ratelimits_per_month_v1.sql
- internal/clickhouse/schema/002_create_metrics_raw_api_requests_v1.sql
- internal/clickhouse/schema/045_create_api_requests_per_minute_mv_v1.sql
- internal/clickhouse/schema/037_create_verifications.key_verifications_per_hour_mv_v3.sql
- internal/clickhouse/schema/001_create_databases.sql
- internal/clickhouse/schema/026_create_billable_ratelimits_v1.sql
- internal/clickhouse/schema/036_create_verifications.key_verifications_per_hour_v3.sql
- internal/clickhouse/schema/032_create_verifications.key_verifications_per_month_v2.sql
- internal/clickhouse/schema/050_create_verifications.key_verifications_per_minute_v1.sql
- internal/clickhouse/schema/009_create_verifications_key_verifications_per_month_mv_v1.sql
- internal/clickhouse/schema/043_create_api_requests_per_hour_mv_v1.sql
- internal/clickhouse/schema/023_create_business_active_workspaces_per_month_mv_v1.sql
- internal/clickhouse/schema/010_create_ratelimits_raw_ratelimits_table.sql
- internal/clickhouse/schema/049_raw_api_metrics_ratelimit_indexes_v1.sql
- internal/clickhouse/schema/039_create_verifications.key_verifications_per_day_mv_v3.sql
- internal/clickhouse/schema/047_create_api_requests_per_day_mv_v1.sql
- internal/clickhouse/schema/030_create_verifications.key_verifications_per_day_v2.sql
- internal/clickhouse/schema/012_create_billing_billable_verifications_per_month_v1.sql
- internal/clickhouse/schema/013_create_billing_billable_verifications_per_month_mv_v1.sql
- internal/clickhouse/schema/029_create_verifications.key_verifications_per_hour_mv_v2.sql
- internal/clickhouse/schema/048_raw_ratelimits_metrics_indexes_v1.sql
- internal/clickhouse/schema/041_create_verifications.key_verifications_per_month_mv_v3.sql
- internal/clickhouse/schema/028_create_verifications.key_verifications_per_hour_v2.sql
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
tools/migrate/v1_deprecation.tstools/migrate/ch_logs.ts
📚 Learning: 2025-04-22T14:40:51.459Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Applied to files:
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.tsinternal/clickhouse/src/logs-timeseries.test.tsapps/engineering/content/docs/architecture/services/clickhouse.mdxgo/apps/api/integration/multi_node_ratelimiting/run.gotools/migrate/ch_logs.tsapps/engineering/content/docs/contributing/pull-request-checks.mdx
📚 Learning: 2025-09-12T13:25:41.849Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Applied to files:
internal/clickhouse/src/logs-timeseries.test.tsapps/engineering/content/docs/architecture/services/clickhouse.mdxgo/apps/api/integration/multi_node_ratelimiting/run.gogo/apps/api/integration/multi_node_usagelimiting/run.gotools/migrate/ch_logs.ts
📚 Learning: 2025-09-25T09:18:38.763Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
Applied to files:
apps/engineering/content/docs/architecture/services/clickhouse.mdxtools/migrate/ch_logs.ts
📚 Learning: 2025-04-22T14:43:11.724Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Applied to files:
apps/engineering/content/docs/architecture/services/clickhouse.mdx
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
apps/engineering/content/docs/architecture/services/clickhouse.mdxgo/apps/api/integration/multi_node_usagelimiting/run.goapps/engineering/content/docs/contributing/pull-request-checks.mdx
📚 Learning: 2024-10-20T07:05:55.471Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 2294
File: apps/api/src/pkg/keys/service.ts:268-271
Timestamp: 2024-10-20T07:05:55.471Z
Learning: In `apps/api/src/pkg/keys/service.ts`, `ratelimitAsync` is a table relation, not a column selection. When querying, ensure that table relations are included appropriately, not as columns.
Applied to files:
go/apps/api/integration/multi_node_ratelimiting/run.goapps/engineering/content/docs/contributing/pull-request-checks.mdx
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/apps/api/integration/multi_node_usagelimiting/run.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/apps/api/integration/multi_node_usagelimiting/run.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/200_test.go
📚 Learning: 2025-02-21T11:15:16.185Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2896
File: internal/clickhouse/src/ratelimits.ts:468-592
Timestamp: 2025-02-21T11:15:16.185Z
Learning: The cursor logic in getRatelimitOverviewLogs query uses (time, request_id) < (cursorTime, cursorRequestId) comparison which works well with descending order but may need adjustment for ascending sorts based on real usage patterns.
Applied to files:
apps/engineering/content/docs/contributing/pull-request-checks.mdx
🧬 Code graph analysis (2)
go/apps/api/integration/multi_node_usagelimiting/run.go (1)
go/pkg/testutil/seed/seed.go (1)
Resources(23-28)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (2)
go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)go/pkg/clickhouse/select.go (1)
Select(62-74)
⏰ 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). (5)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
🔇 Additional comments (11)
go/apps/api/routes/v2_ratelimit_limit/200_test.go (1)
130-136: LGTM! Clean migration to v2 schema.The changes correctly update the test to use the v2 ClickHouse schema:
- Struct type updated to
schema.RatelimitV2- Generic type parameter updated accordingly
- Table query updated to
default.ratelimits_raw_v2All fields used in the test assertions (
Identifier,Passed,RequestID) are present in theRatelimitV2struct, ensuring the test will continue to work correctly.go/pkg/clickhouse/billable_ratelimits.go (1)
25-25: LGTM! Change is correct and properly coordinated.The v2 table migration is verified:
- Table definition exists in
go/pkg/clickhouse/schema/018_billable_ratelimits_per_month_v2.sqland migrations- Schema is compatible: includes
workspace_id,year,month, andcountcolumns (populated by materialized view fromratelimits_per_month_v2)- Materialized view
billable_ratelimits_per_month_mv_v2auto-populates the table- No remaining v1 references in Go code
- Migration is coordinated across multiple language implementations (TypeScript also updated)
tools/migrate/v1_deprecation.ts (2)
49-53: LGTM: Improved readability.The multiline formatting improves readability without changing functionality.
31-31: Data continuity verification required—v2 tables may lack historical v1 usage data.Schema compatibility is confirmed:
api_requests_per_day_v2contains all required columns (workspace_id,path,time). However, a critical data concern remains:The script queries v2 tables (which populate from
api_requests_raw_v2) but no data migration logic was found. Since v2 tables are in thedefaultschema versus v1'smetricsschema, and no explicit backfill scripts exist, v2 tables may only contain data from after v2 deployment. If so, the past 30-day query will find no (or minimal) v1 endpoint usage, and workspaces won't receive deprecation notices.Verify before merging:
- When was v2 deployed relative to today?
- Was historical v1 usage data backfilled into
api_requests_raw_v2?- Does the 30-day query window include data from before v2 deployment?
tools/migrate/ch_logs.ts (1)
25-25: Migration script updated to v2 table.The source table reference has been updated from
metrics.raw_api_requests_v1toapi_requests_raw_v2. Ensure this migration script is run in an environment where the default schema is properly configured, or add an explicit schema prefix.apps/engineering/content/docs/contributing/pull-request-checks.mdx (1)
147-147: Documentation updated to reflect v2 table migration.The documentation examples have been updated from v1 to v2 table references. The changes are consistent with the broader migration effort and provide developers with current table names for performance optimization examples.
Also applies to: 167-168, 180-181, 195-196, 215-215, 231-232, 237-238, 241-244
go/apps/api/integration/multi_node_ratelimiting/run.go (1)
182-182: Integration test updated to v2 tables.The queries have been updated to use
default.ratelimits_raw_v2anddefault.api_requests_raw_v2. The changes are consistent with the v2 migration and the Go integration test patterns observed inmulti_node_usagelimiting/run.go.Also applies to: 208-208
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts (1)
76-76: Test updated to v2 table with explicit schema prefix.The test now queries
default.key_verifications_raw_v2with an explicitdefault.schema prefix. Note that this differs frominternal/clickhouse/src/billing.tsline 51 which queries without a schema prefix, andinternal/clickhouse/src/logs-timeseries.test.tsline 60 which also omits the prefix.go/apps/api/integration/multi_node_usagelimiting/run.go (1)
179-179: The review comment is based on incorrect information and should be disregarded.The review cites
internal/clickhouse/src/billing.ts(line 51) as evidence that TypeScript code removes the schema prefix, but this file does not exist in the repository. Verification of the actual codebase shows all production TypeScript ClickHouse queries consistently use thedefault.schema prefix, exactly matching the Go integration test pattern:
internal/clickhouse/src/verifications.ts: usesFROM default.key_verifications_raw_v2internal/clickhouse/src/logs.ts: usesFROM default.api_requests_raw_v2internal/clickhouse/src/ratelimits.ts: usesFROM default.ratelimits_raw_v2internal/clickhouse/src/keys/keys.ts: usesFROM default.key_verifications_raw_v2The code changes at lines 179 and 206 are consistent with the codebase pattern across both languages.
Likely an incorrect or invalid review comment.
apps/api/src/routes/v1_analytics_getVerifications.ts (1)
225-244: LGTM! Clean table migration with explicit database prefix.The table references have been correctly updated from
verifications.key_verifications_per_*_v3todefault.key_verifications_per_*_v2. The explicitdefault.prefix provides clarity and prevents ambiguity. The query structure, time-binning logic, and WITH FILL clauses remain unchanged, ensuring consistent behavior.apps/engineering/content/docs/architecture/services/analytics.mdx (1)
67-72: LGTM! Documentation correctly reflects the v2 table migration.The table alias mappings have been properly updated to reflect the new schema:
- Public aliases maintain stable
_v1naming for API compatibility- Internal tables correctly reference
default.*_v2with explicit database prefix- The example query on line 42 also demonstrates the
default.key_verifications_raw_v2usageThis documentation accurately represents the changes in this PR.
internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
Outdated
Show resolved
Hide resolved
ogzhanolguncu
approved these changes
Nov 6, 2025
Contributor
|
Thank you for following the naming conventions for pull request titles! 🙏 |
Contributor
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 (1)
tools/migrate/ch_logs.ts (1)
48-48: Pre-existing typo in logging statement.There's a typo on this line:
console.infishould beconsole.info. This would cause a runtime error if the script is executed.Apply this diff to fix the typo:
- console.infi(i++, logs.length); + console.info(i++, logs.length);
🧹 Nitpick comments (1)
tools/migrate/v1_deprecation.ts (1)
49-52: Optional formatting improvement.The multi-line template literal format improves readability without changing functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
go/pkg/clickhouse/billable_verifications.go(1 hunks)go/pkg/clickhouse/key_verifications_test.go(16 hunks)go/pkg/clickhouse/ratelimits_test.go(9 hunks)internal/clickhouse/src/logs-timeseries.test.ts(1 hunks)internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts(3 hunks)tools/migrate/ch_logs.ts(1 hunks)tools/migrate/v1_deprecation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/clickhouse/src/logs-timeseries.test.ts
- go/pkg/clickhouse/billable_verifications.go
- internal/clickhouse/src/verification_outcomes_propagate_correctly.test.ts
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
📚 Learning: 2025-09-12T13:25:41.849Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Applied to files:
go/pkg/clickhouse/key_verifications_test.gotools/migrate/v1_deprecation.tstools/migrate/ch_logs.ts
📚 Learning: 2025-04-22T14:40:51.459Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Applied to files:
go/pkg/clickhouse/key_verifications_test.gogo/pkg/clickhouse/ratelimits_test.gotools/migrate/ch_logs.ts
📚 Learning: 2025-10-30T15:10:52.743Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-04-22T14:43:11.724Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-07-17T14:24:20.403Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3631
File: go/pkg/db/bulk_keyring_insert.sql.go:23-25
Timestamp: 2025-07-17T14:24:20.403Z
Learning: In go/pkg/db/bulk_keyring_insert.sql.go and similar bulk insert generated files, hardcoded zero values for fields like size_approx and size_last_updated_at are intentional and reflect the original SQL query structure, not missing parameters.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-08-14T16:25:48.167Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3785
File: go/apps/api/routes/v2_keys_reroll_key/401_test.go:52-61
Timestamp: 2025-08-14T16:25:48.167Z
Learning: User Flo4604 requested creation of a GitHub issue to track converting all test files to use table-driven test patterns as a broader codebase improvement, following the suggestion made during review of go/apps/api/routes/v2_keys_reroll_key/401_test.go.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-07-16T15:38:53.491Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3606
File: go/pkg/db/replica.go:8-11
Timestamp: 2025-07-16T15:38:53.491Z
Learning: For debugging database replica usage in go/pkg/db/replica.go, it's acceptable to mark QueryRowContext operations as "success" even though SQL errors only surface during row.Scan() calls. The timing metrics are the primary concern for debugging replica performance patterns.
Applied to files:
go/pkg/clickhouse/key_verifications_test.gogo/pkg/clickhouse/ratelimits_test.go
📚 Learning: 2025-04-08T09:34:24.576Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 2872
File: apps/dashboard/lib/trpc/routers/ratelimit/createNamespace.ts:36-39
Timestamp: 2025-04-08T09:34:24.576Z
Learning: In the Unkey dashboard, when making database queries involving workspaces, use `ctx.workspace.id` directly instead of fetching the workspace separately for better performance and security.
Applied to files:
go/pkg/clickhouse/key_verifications_test.go
📚 Learning: 2025-07-25T19:09:43.284Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 3662
File: apps/dashboard/lib/trpc/routers/deployment/list.ts:11-11
Timestamp: 2025-07-25T19:09:43.284Z
Learning: In apps/dashboard/lib/trpc/routers/deployment/list.ts, the listDeployments procedure intentionally queries the versions table rather than a deployments table. The user mcstepp indicated that renaming the table would require a database migration, which was deferred for the current PR focused on UI features.
Applied to files:
tools/migrate/v1_deprecation.tstools/migrate/ch_logs.ts
📚 Learning: 2025-09-25T09:18:38.763Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
Applied to files:
tools/migrate/ch_logs.ts
🔇 Additional comments (5)
tools/migrate/v1_deprecation.ts (1)
31-31: Table migration aligns with PR objectives.The migration from
metrics.api_requests_per_day_v1todefault.api_requests_per_day_v2correctly aligns with the PR's goal of consolidating ClickHouse reads to v2 tables under the default schema.Please verify that this deprecation notification script has been tested against the v2 table to ensure it returns the expected workspace and path results.
go/pkg/clickhouse/ratelimits_test.go (2)
110-124: LGTM! Raw table references correctly updated to v2.The INSERT and verification queries now correctly target
default.ratelimits_raw_v2, consistent with the v2 migration. The EventuallyWithT pattern appropriately handles materialized view propagation delays.
140-312: Excellent! Comprehensive validation of v2 aggregated tables.All time-bucketed materialized view references (
per_minute_v2,per_hour_v2,per_day_v2,per_month_v2) are consistently updated to the default schema. The test thoroughly validates:
- Pass/fail counts across all granularities
- Latency aggregates (avg, p75, p99)
- Namespace and identifier-level aggregations
- Pass rate analysis from multiple dimensions
This provides strong confidence that the v2 tables and their materialized views are functioning correctly.
go/pkg/clickhouse/key_verifications_test.go (2)
101-118: LGTM! Raw table operations correctly updated.The INSERT and COUNT operations now correctly reference
default.key_verifications_raw_v2with proper schema qualification. The eventual consistency check appropriately handles the distributed nature of ClickHouse.
121-487: All v2 aggregation table references are correctly defined with supporting infrastructure.The schema verification confirms:
- external_id field is defined in the raw v2 table with an index
- Materialized views properly include external_id in SELECT and GROUP BY clauses for aggregation
- All required table definitions exist:
key_verifications_per_{minute,hour,day,month}_v2andbillable_verifications_per_month_v2- Test code comprehensively validates external_id, outcomes, credits, and latency metrics across all aggregation tables
No issues found with the code changes.
ogzhanolguncu
approved these changes
Nov 7, 2025
Contributor
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/api/src/routes/v1_analytics_getVerifications.ts (1)
403-414: Fix ORDER BY + WITH FILL: invalid SQL when ordering by non-time fields.WITH FILL must be attached to ORDER BY time. If filters.orderBy is set (e.g., "total"), the query becomes ORDER BY … then WITH FILL …, which ClickHouse rejects and/or fills incorrectly. Move/guard the fill so it’s only emitted when ordering by time.
Apply this minimal restructuring:
- if (filters.orderBy) { - query.push(`ORDER BY { orderBy: Identifier } ${filters.order === "asc" ? "ASC" : "DESC"} `); - } else if (groupBy.includes("time")) { - query.push("ORDER BY time ASC"); - } + if (groupBy.includes("time")) { + if (!filters.orderBy || filters.orderBy === "time") { + query.push("ORDER BY time ASC"); + query.push(table.fill); + } else { + // Can't use WITH FILL when ordering by a non-time column + query.push(`ORDER BY { orderBy: Identifier } ${filters.order === "asc" ? "ASC" : "DESC"} `); + } + } else if (filters.orderBy) { + query.push(`ORDER BY { orderBy: Identifier } ${filters.order === "asc" ? "ASC" : "DESC"} `); + } - - if (groupBy.includes("time")) { - query.push(table.fill); - }
🧹 Nitpick comments (2)
apps/api/src/routes/v1_analytics_getVerifications.ts (2)
232-236: Preserve time precision for day fill; avoid downcast to DateTime.toDateTime(toStartOfDay(fromUnixTimestamp64Milli(...))) may coerce to DateTime (seconds). If the v2 tables’ time column is DateTime64(3), prefer toStartOfInterval to keep types aligned and avoid implicit casts.
- fill: `WITH FILL -FROM toDateTime(toStartOfDay(fromUnixTimestamp64Milli({ start: Int64 }))) -TO toDateTime(toStartOfDay(fromUnixTimestamp64Milli({ end: Int64 }))) -STEP INTERVAL 1 DAY`, + fill: `WITH FILL +FROM toStartOfInterval(fromUnixTimestamp64Milli({ start: Int64 }), INTERVAL 1 DAY) +TO toStartOfInterval(fromUnixTimestamp64Milli({ end: Int64 }), INTERVAL 1 DAY) +STEP INTERVAL 1 DAY`,Please confirm the time column type in default.key_verifications_per_day_v2 (DateTime vs DateTime64). If it’s DateTime64, the change above is required.
239-244: Normalize month fill to toStartOfInterval for consistency.Minor but keeps types consistent across granularities and avoids implicit casts.
- fill: `WITH FILL -FROM toDateTime(toStartOfMonth(fromUnixTimestamp64Milli({ start: Int64 }))) -TO toDateTime(toStartOfMonth(fromUnixTimestamp64Milli({ end: Int64 }))) -STEP INTERVAL 1 MONTH`, + fill: `WITH FILL +FROM toStartOfInterval(fromUnixTimestamp64Milli({ start: Int64 }), INTERVAL 1 MONTH) +TO toStartOfInterval(fromUnixTimestamp64Milli({ end: Int64 }), INTERVAL 1 MONTH) +STEP INTERVAL 1 MONTH`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts(1 hunks)apps/api/src/routes/v1_analytics_getVerifications.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
⏰ 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). (5)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Test Go API Local / Test
🔇 Additional comments (1)
apps/api/src/routes/v1_analytics_getVerifications.ts (1)
223-231: Switch to default.*_v2 tables looks correct; verify availability/permissions.Names align with the v2 migration pattern. Please verify these exist and are readable in all envs the API talks to (prod, staging), and that columns used here (workspace_id, key_space_id, key_id, identity_id, tags, outcome, count, time) match v2.
If helpful, run a quick schema check in your CH console:
- DESC default.key_verifications_per_hour_v2
- DESC default.key_verifications_per_day_v2
- DESC default.key_verifications_per_month_v2
Also applies to: 239-244
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go/pkg/clickhouse/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
go/pkg/clickhouse/migrations/20251110103702.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Learnt from: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/actions/setup-node/action.yaml:0-0
Timestamp: 2025-08-08T15:10:46.436Z
Learning: Repo: unkeyed/unkey — Preference: If imeyer comments “issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
📚 Learning: 2025-09-25T09:18:38.763Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
Applied to files:
go/pkg/clickhouse/migrations/20251110103702.sql
📚 Learning: 2025-04-22T14:43:11.724Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Applied to files:
go/pkg/clickhouse/migrations/20251110103702.sql
📚 Learning: 2025-04-22T14:40:51.459Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Applied to files:
go/pkg/clickhouse/migrations/20251110103702.sql
📚 Learning: 2025-09-12T13:25:41.849Z
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Applied to files:
go/pkg/clickhouse/migrations/20251110103702.sql
⏰ 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). (6)
- GitHub Check: PR title
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
🧬 Code graph analysis (1)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts (2)
internal/clickhouse/src/index.ts (1)
verifications(128-173)apps/api/src/routes/v1_analytics_getVerifications.ts (1)
V1AnalyticsGetVerificationsResponse(190-192)
⏰ 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). (6)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/api/src/routes/v1_analytics_getVerifications.ts (1)
440-440: Update the utility comment to document Date type support.The
dateTimeToUnixutility atinternal/clickhouse/src/util.ts:3currently comments only on DateTime handling. Since the day and month tables now return ClickHouse Date types (YYYY-MM-DD format), update the comment to clarify that the utility handles both Date and DateTime string formats:// ClickHouse Date ('YYYY-MM-DD') and DateTime ('YYYY-MM-DD HH:MM:SS') both return strings. // JavaScript's Date constructor reliably parses both formats. export const dateTimeToUnix = z.string().transform((t) => new Date(t).getTime());The utility already handles both types correctly—JavaScript's Date constructor parses YYYY-MM-DD as UTC midnight and datetime strings without issues. The fix is purely documentary to reflect current usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts(5 hunks)apps/api/src/routes/v1_analytics_getVerifications.ts(2 hunks)tools/artillery/create-keys.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4190
File: go/internal/services/keys/verifier.go:51-53
Timestamp: 2025-10-30T15:10:52.743Z
Learning: PR #4190 for unkeyed/unkey is focused solely on database schema and query changes for identity-based credits. It adds IdentityCredits and KeyCredits fields to structs and queries, but does not implement the priority enforcement logic in the usagelimiter. The logic implementation is intentionally deferred to a later PR in the stack.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/002_ratelimits/006_ratelimits_per_day_v1.sql:1-13
Timestamp: 2025-04-22T14:43:11.724Z
Learning: In the unkey project, the SQL files in clickhouse/schema/databases represent the current production schema and shouldn't be modified directly in PRs. Schema changes require dedicated migration scripts.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3958
File: internal/clickhouse/src/requests.ts:7-8
Timestamp: 2025-09-12T13:25:41.849Z
Learning: In the ClickHouse v2 table migration, the location field was renamed from `colo` to `region` across all tables including `default.api_requests_raw_v2`. The zod insert schemas must be updated accordingly to avoid runtime insert failures.
Learnt from: chronark
Repo: unkeyed/unkey PR: 3161
File: go/pkg/clickhouse/schema/databases/001_verifications/002_raw_key_verifications_v1.sql:31-33
Timestamp: 2025-04-22T14:40:51.459Z
Learning: The ClickHouse table schemas in the codebase mirror the production environment and cannot be modified directly in PRs without careful migration planning.
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 4016
File: go/pkg/clickhouse/migrations/20250925091254.sql:10-10
Timestamp: 2025-09-25T09:18:38.763Z
Learning: In ClickHouse v2 table migrations, the region field should not be carried over from v1 because v1 uses Cloudflare region identifiers while v2 uses AWS region identifiers. These are incompatible data formats, so setting region to empty string in materialized view migrations is the correct approach.
📚 Learning: 2025-10-21T09:45:47.560Z
Learnt from: ogzhanolguncu
Repo: unkeyed/unkey PR: 4107
File: go/pkg/clickhouse/key_verifications_test.go:20-20
Timestamp: 2025-10-21T09:45:47.560Z
Learning: In ClickHouse tests (e.g., go/pkg/clickhouse/key_verifications_test.go), parallel execution with t.Parallel() is safe when tests use workspaceID-based isolation. Each test generates a unique workspaceID (via uid.New(uid.WorkspacePrefix)), and all database operations are scoped to that workspaceID, providing logical isolation even when tests share a single ClickHouse instance.
Applied to files:
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts
🧬 Code graph analysis (1)
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts (1)
internal/clickhouse/src/index.ts (1)
verifications(128-173)
⏰ 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). (6)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Test Packages / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
tools/artillery/create-keys.ts (2)
84-84: LGTM!The error logging improvement aids debugging of failed key creation requests.
148-149: LGTM!The switch to
Number.parseIntis a valid stylistic choice with no functional impact.apps/api/src/routes/v1_analytics_getVerifications.ts (3)
225-225: LGTM! Table names updated to v2 schema.The table references have been correctly updated from
verifications.key_verifications_per_*_v3todefault.key_verifications_per_*_v2, aligning with the ClickHouse v2 schema migration.Also applies to: 232-232, 239-239
234-235: LGTM! Date type handling in FILL clauses is correct.The use of
toDate()wrapping in the FILL clauses for day and month tables indicates these tables now use theDatecolumn type instead ofDateTime. This is consistent with ClickHouse best practices for daily and monthly aggregations.Also applies to: 241-242
392-404: LGTM! Conditional date handling properly accounts for schema differences.The WHERE clause logic correctly handles the different column types:
- Month/day tables: Uses
toDate(toStartOf*(...))for Date type columns- Hour tables: Uses
fromUnixTimestamp64Milli(...)for DateTime type columnsThe comment clearly explains the rationale.
apps/api/src/routes/v1_analytics_getVerifications.happy.test.ts (4)
43-67: LGTM! Dynamic date calculations prevent TTL-related test failures.The switch from fixed timestamps to relative date calculations (using
generateDaysAgoandqueryDaysAgo) ensures tests won't fail due to the 1-month TTL policy on ClickHouse tables. The runtime calculation ofgenerateStart,generateEnd,queryStart, andqueryEndproperly implements this approach.
83-83: LGTM! Table reference updated to v2 schema.The query now correctly references
default.key_verifications_raw_v2, aligning with the ClickHouse v2 schema migration.
410-414: LGTM! Start date calculation aligns with month-based aggregation.Setting the start date to the first day of last month at midnight (
setMonth(..., 1)andsetHours(0, 0, 0, 0)) correctly establishes a month boundary that aligns with how ClickHouse's monthly aggregation tables work.
436-469: LGTM! Month boundary logic correctly handles calendar month aggregation.The
toStartOfMonthhelper and the updated filtering logic properly account for the fact that monthly aggregations in ClickHouse return complete calendar months. The normalization to month start boundaries (toStartOfMonth) and the subsequent filtering (vMonthStart < startMonthStart || vMonthStart > endMonthStart) ensures the test expectations match the actual query behavior.
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
3 participants
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.
docker compose -f ./deployment/docker-compose.yaml up api -d
wait
pnpm --dir=apps/api test:integration v1_analytics_getVerifications