Conversation
chronark
commented
Nov 7, 2025
chronark
commented
- chore: switch clickhouse reads to new tables
- revert: version
- revert: version
- fix: billable_ratelimits_per_month_v2 to default
- chore: add default database everywhere
- [autofix.ci] apply automated fixes
- wip
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
📝 WalkthroughWalkthroughReplace V1 ClickHouse schemas and buffer APIs with unified v2 types ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant H as API Handler
participant M as Metrics Middleware (WithMetrics + InstanceInfo)
participant B as ClickHouse Bufferer (EventBuffer)
participant P as BatchProcessor (apiRequests / keyVerifications / ratelimits)
H->>M: pass request context
M->>M: construct schema.ApiRequest{..., ResponseStatus: int32(...), Region: info.Region}
M->>B: BufferApiRequest(schema.ApiRequest)
activate B
B->>P: enqueue event into v2 batch
deactivate B
P->>P: async flush to ClickHouse
H->>H: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
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 (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)go/apps/api/routes/register.go (3)
⏰ 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). (4)
🔇 Additional comments (2)
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
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
go/apps/api/routes/chproxy_metrics/handler.go(2 hunks)go/apps/api/routes/chproxy_ratelimits/handler.go(1 hunks)go/apps/api/routes/chproxy_verifications/handler.go(2 hunks)go/apps/api/routes/register.go(1 hunks)go/apps/api/routes/v2_ratelimit_limit/handler.go(1 hunks)go/apps/api/run.go(2 hunks)go/internal/services/keys/verifier.go(1 hunks)go/pkg/clickhouse/client.go(4 hunks)go/pkg/clickhouse/interface.go(1 hunks)go/pkg/clickhouse/noop.go(1 hunks)go/pkg/clickhouse/schema/requests.go(0 hunks)go/pkg/zen/instance.go(1 hunks)go/pkg/zen/middleware_metrics.go(4 hunks)
💤 Files with no reviewable changes (1)
- go/pkg/clickhouse/schema/requests.go
🧰 Additional context used
🧠 Learnings (3)
📓 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: imeyer
Repo: unkeyed/unkey PR: 3755
File: .github/workflows/job_detect_changes.yaml:111-114
Timestamp: 2025-08-08T16:10:00.224Z
Learning: Repo: unkeyed/unkey — When imeyer writes “make me an issue” on a PR thread, automatically open a thorough GitHub issue (Summary, Impact, Where, Repro, Observed vs Expected, Acceptance Criteria, Validation Plan, Out of Scope, References), include backlinks to the PR and comment, and assign to imeyer.
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: 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.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/api/keys/query-key-usage-timeseries/index.ts:1-1
Timestamp: 2025-08-22T12:48:58.289Z
Learning: The team at Unkey is planning to move from TRPC to calling their v2 API directly. They're comfortable keeping TRPC input schemas imported from app route folders as technical debt since this code will be replaced, rather than refactoring to move schemas to lib/schemas.
Learnt from: perkinsjr
Repo: unkeyed/unkey PR: 3775
File: apps/dashboard/lib/trpc/routers/workspace/onboarding.ts:1-1
Timestamp: 2025-08-22T12:45:07.187Z
Learning: The team at Unkey is planning to move from TRPC to calling their v2 API directly, making current TRPC schema organization temporary. They're comfortable with keeping server input schemas imported from app route folders as technical debt since this code will be replaced.
📚 Learning: 2025-08-27T13:48:54.016Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3841
File: go/pkg/db/queries/key_find_for_verification.sql:15-15
Timestamp: 2025-08-27T13:48:54.016Z
Learning: The pending_migration_id field in FindKeyForVerification is only used internally by get_migrated.go for migration logic and doesn't need to be exposed in KeyVerifier struct or API response DTOs since it's an internal implementation detail.
Applied to files:
go/internal/services/keys/verifier.go
📚 Learning: 2025-08-20T10:52:19.334Z
Learnt from: Flo4604
Repo: unkeyed/unkey PR: 3810
File: go/apps/api/routes/chproxy_metrics/handler.go:36-37
Timestamp: 2025-08-20T10:52:19.334Z
Learning: In the Unkey codebase, ClickHouse logging should be controlled explicitly per-handler using s.DisableClickHouseLogging() calls rather than through middleware, to allow for granular control and handler-specific logic around when logging should be disabled.
Applied to files:
go/apps/api/routes/v2_ratelimit_limit/handler.go
🧬 Code graph analysis (11)
go/pkg/clickhouse/noop.go (1)
go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)
go/apps/api/routes/chproxy_ratelimits/handler.go (2)
go/pkg/zen/request_util.go (1)
BindBody(10-23)go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)
go/apps/api/routes/register.go (1)
go/pkg/zen/middleware_metrics.go (1)
WithMetrics(57-109)
go/internal/services/keys/verifier.go (1)
go/pkg/clickhouse/schema/types.go (1)
KeyVerificationV2(6-19)
go/apps/api/routes/v2_ratelimit_limit/handler.go (2)
go/pkg/clickhouse/interface.go (1)
ClickHouse(51-60)go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)
go/apps/api/routes/chproxy_metrics/handler.go (2)
go/pkg/zen/request_util.go (1)
BindBody(10-23)go/pkg/clickhouse/schema/types.go (1)
ApiRequestV2(41-60)
go/apps/api/routes/chproxy_verifications/handler.go (3)
go/pkg/zen/request_util.go (1)
BindBody(10-23)go/pkg/clickhouse/schema/types.go (1)
KeyVerificationV2(6-19)go/pkg/clickhouse/interface.go (1)
ClickHouse(51-60)
go/pkg/zen/middleware_metrics.go (3)
go/pkg/clickhouse/schema/types.go (1)
ApiRequestV2(41-60)go/apps/gw/server/middleware_metrics.go (2)
WithMetrics(22-90)EventBuffer(12-14)go/pkg/zen/instance.go (1)
InstanceInfo(3-6)
go/pkg/clickhouse/interface.go (1)
go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)
go/apps/api/run.go (1)
go/pkg/zen/instance.go (1)
InstanceInfo(3-6)
go/pkg/clickhouse/client.go (2)
go/pkg/batch/process.go (3)
BatchProcessor(21-27)New(84-120)Config(32-60)go/pkg/clickhouse/schema/types.go (1)
RatelimitV2(24-36)
🪛 GitHub Actions: PR
go/apps/api/routes/register.go
[error] 64-64: syntax error: non-declaration statement outside function body. Command failed: go build -o bin/unkey -ldflags="-X 'github.com/unkeyed/unkey/go/pkg/version.Version=${VERSION}'" ./main.go
🔇 Additional comments (15)
go/apps/api/routes/chproxy_ratelimits/handler.go (1)
51-63: LGTM! Clean migration to V2 schema.The update from
RatelimitRequestV1toRatelimitV2is consistent with the PR's migration objective. The binding and buffering logic remains unchanged, only the schema type has been updated.go/pkg/zen/instance.go (1)
1-6: LGTM! Clean type definition.The
InstanceInfostruct provides a clear way to propagate instance identification and region data through the API routing and middleware layers.go/internal/services/keys/verifier.go (1)
124-136: LGTM! V2 schema migration is correct.The migration from
BufferKeyVerificationtoBufferKeyVerificationV2with the V2 schema type is consistent with the PR objective. All relevant fields are properly mapped to the new schema.go/apps/api/routes/chproxy_verifications/handler.go (1)
51-63: LGTM! Consistent V2 migration.The updates to
KeyVerificationV2schema and theBufferKeyVerificationV2method align with the broader V2 migration across the codebase.go/apps/api/run.go (1)
294-315: LGTM! Clean integration of InstanceInfo.The route registration now properly passes instance identification and region data via the new
InstanceInfoparameter, enabling region-aware metrics and logging throughout the request lifecycle.go/apps/api/routes/chproxy_metrics/handler.go (1)
51-63: LGTM! V2 schema migration is correct.The migration from
ApiRequestV1toApiRequestV2and the corresponding update toBufferApiRequestare consistent with the V2 migration pattern across the PR.go/apps/api/routes/v2_ratelimit_limit/handler.go (1)
288-297: LGTM! Rate limit logging updated to V2 schema.The migration to
RatelimitV2is consistent with the broader V2 schema adoption across the codebase.go/pkg/clickhouse/noop.go (1)
28-30: LGTM! No-op implementation correctly updated to V2.The
BufferRatelimitsignature now matches the updated interface withschema.RatelimitV2.go/pkg/zen/middleware_metrics.go (3)
14-16: LGTM! EventBuffer interface updated to V2.The interface now correctly uses
BufferApiRequest(schema.ApiRequestV2)instead of the V1BufferRequestmethod.
87-104: LGTM! V2 event buffering properly implemented.The buffering call now uses:
BufferApiRequestwithApiRequestV2schema- Explicit
int32cast forResponseStatusmatching V2 schema definitioninfo.Regionto populate region data fromInstanceInfo
57-57: LGTM! InstanceInfo parameter added for region tracking.All callers of
zen.WithMetricshave been properly updated. The single call ingo/apps/api/routes/register.go:65correctly passes the newinfoparameter.go/pkg/clickhouse/client.go (4)
27-27: LGTM! Struct field migrated to V2.The
ratelimitsV2field now uses*batch.BatchProcessor[schema.RatelimitV2]instead of the V1 type.
247-249: LGTM! BufferRatelimit method migrated to V2.The method signature and implementation now use
schema.RatelimitV2and buffer to the V2 processor.
312-312: LGTM! Close method updated for V2 processor.The
Close()method now correctly closesratelimitsV2instead of the V1 processor.
144-162: LGTM! Batch processor correctly configured for V2.The ratelimits batch processor:
- Uses
schema.RatelimitV2type- Flushes to
default.ratelimits_raw_v2table (V2 table name)- Maintains appropriate batch sizing and flush intervals
Verification confirms the V2 table
default.ratelimits_raw_v2exists in the ClickHouse schema with the expected columns and indexes.
Graphite Automations"Notify author when CI fails" took an action on this PR • (11/11/25)1 teammate was notified to this PR based on Andreas Thomas's automation. "Post a GIF when PR approved" took an action on this PR • (11/12/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
lgtm, we could in theory adjust v1's clickhouse client to send the new fields over too but im not too bothered by it
|
|
3 participants
