Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
|
Warning Rate limit exceeded@chronark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 Walkthrough## Walkthrough
A new `ClickHouseProxyClient` class was introduced to handle batched event inserts to ClickHouse via a proxy HTTP API. The `Analytics` class was updated to use this proxy client for inserts if configured, and its constructor and insert methods were adjusted accordingly. Error handling for proxy-based inserts was standardized. Additional changes include increasing batch sizes in the ClickHouse client, adding Prometheus metrics for batch processing and HTTP request body sizes, renaming some buffer metrics, and modifying batch processor flush logic to include flush trigger labels. A generic batch processing utility was removed.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|------------------------------------------------------------------------------------------------------------|
| apps/api/src/pkg/analytics.ts | Refactored `Analytics` to always instantiate a `ClickHouse` client for queries/reads; conditionally instantiate `ClickHouseProxyClient` for inserts if proxy config is present. Updated constructor and insert methods to use proxy client with error handling. |
| apps/api/src/pkg/clickhouse-proxy.ts | Added new `ClickHouseProxyClient` class with methods to send batched event data to internal ClickHouse proxy API endpoints. Centralized HTTP request logic and error handling. |
| go/pkg/batch/consume.go | Deleted generic batch processing utility function `Process` that batched items and flushed periodically. |
| go/pkg/batch/process.go | Modified `BatchProcessor` to add a flush trigger string parameter to flush function; updated flush calls and wrapped flush with Prometheus metrics instrumentation. |
| go/pkg/clickhouse/client.go | Increased batch size from 1000 to 10000 for three batch processors (API requests, key verifications, rate limits). |
| go/pkg/prometheus/metrics/batch.go | Added new Prometheus metrics for batch processing: batch size distribution histogram, batch operations counter, and items processed counter. |
| go/pkg/prometheus/metrics/buffer.go | Renamed buffer-related Prometheus metrics: changed subsystem from "api" to "buffer" and updated metric names accordingly. |
| go/pkg/prometheus/metrics/http.go | Added new histogram metric to track HTTP request body sizes with predefined buckets. |
| go/pkg/zen/middleware_metrics.go | Added a line to record HTTP request body size metric in middleware, labeled by method, path, and status. |
| apps/api/src/pkg/env.ts | Removed `CLICKHOUSE_INSERT_URL` env var; added `CLICKHOUSE_PROXY_URL` and `CLICKHOUSE_PROXY_TOKEN` optional env vars. |
| apps/api/src/pkg/middleware/init.ts | Updated `Analytics` instantiation to use `clickhouseProxyUrl` and `clickhouseProxyToken` from env vars instead of `clickhouseInsertUrl`; adjusted indentation and formatting. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Caller
participant Analytics
participant ClickHouse
participant ClickHouseProxyClient
Caller->>Analytics: new Analytics({ clickhouseUrl, clickhouseInsertUrl, clickhouseProxyToken })
Analytics->>ClickHouse: instantiate for queries/reads
alt Proxy config provided
Analytics->>ClickHouseProxyClient: instantiate for inserts
endsequenceDiagram
participant Caller
participant Analytics
participant ClickHouse
participant ClickHouseProxyClient
Caller->>Analytics: insertApiRequest(event)
alt ProxyClient configured
Analytics->>ClickHouseProxyClient: insertApiRequests([event])
ClickHouseProxyClient-->>Analytics: Promise<void> (error captured)
Analytics-->>Caller: { err: null | Error }
else
Analytics->>ClickHouse: api.insert(event)
ClickHouse-->>Analytics: Promise<void>
Analytics-->>Caller: result
end
Estimated code review effort3 (~40 minutes) Possibly related PRs
Suggested reviewers
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
…2-feat_use_the_new_api_based_chproxy
|
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: 2
♻️ Duplicate comments (1)
apps/api/src/pkg/analytics.ts (1)
9-24: Address missing parameter passing as noted in previous review.The constructor properly handles conditional proxy client instantiation, but as mentioned in the previous review comment, the
clickhouseProxyTokenparameter needs to be passed from the calling code and CI workflows need updates.The constructor logic is correct for conditional proxy client creation when both URL and token are provided.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/pkg/analytics.ts(2 hunks)apps/api/src/pkg/clickhouse-proxy.ts(1 hunks)
🧠 Learnings (2)
apps/api/src/pkg/analytics.ts (3)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
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.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
apps/api/src/pkg/clickhouse-proxy.ts (1)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
🧰 Additional context used
🧠 Learnings (2)
apps/api/src/pkg/analytics.ts (3)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
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.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
apps/api/src/pkg/clickhouse-proxy.ts (1)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
⏰ 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)
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
apps/api/src/pkg/clickhouse-proxy.ts (5)
5-12: LGTM: Clean constructor implementation with proper URL normalization.The constructor correctly normalizes the base URL by removing trailing slashes and stores the required parameters as private readonly fields.
17-31: LGTM: Well-structured verification events insert method.The method signature includes all necessary fields for key verification events with proper TypeScript typing. The delegation to
sendEventsprovides good separation of concerns.
36-60: LGTM: Comprehensive API request events structure.The method captures all relevant API request metrics including headers, body, latency, and geographical information. The type definitions are thorough and appropriate for analytics purposes.
65-76: LGTM: Simple and effective rate limit events method.The rate limit event structure includes the essential fields needed for tracking rate limiting decisions with proper boolean typing for the
passedfield.
91-91: X-Unkey-Metrics header correctly disables internal metricsThe
X-Unkey-Metrics: "disabled"header is explicitly checked in our Go middleware and rate-limit handler to skip buffering any events. Including it inclickhouse-proxy.tsaligns with the intended behavior—no further changes are needed.• go/pkg/zen/middleware_metrics.go
– SkipsBufferApiRequestwhenHeader.Get("X-Unkey-Metrics") == "disabled"
• apps/api/routes/v2_ratelimit_limit/handler.go
– SkipsBufferRatelimitunder the same header checkapps/api/src/pkg/analytics.ts (4)
3-3: LGTM: Clean import and property declaration.The import statement and optional property declaration are properly structured for the proxy client integration.
Also applies to: 7-7
40-58: LGTM: Consistent proxy integration pattern with proper error handling.The conditional proxy usage pattern is well-implemented:
- Returns async function when proxy is available
- Maintains consistent error handling with
{ err: null | Error }return type- Falls back to original ClickHouse client when proxy is unavailable
- Properly handles both Error instances and other thrown values
61-82: LGTM: Consistent implementation with verification events.The method follows the same pattern as
insertRatelimitwith proper type definitions and error handling. The event structure matches the proxy client's expected interface.
85-116: LGTM: Comprehensive API request event handling.The method maintains consistency with the other insert methods and includes all necessary fields for API request analytics. The extensive type definition ensures data integrity.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/pkg/clickhouse-proxy.ts (1)
82-103: Consider implementing timeout configuration for HTTP requests.The sendEvents method could benefit from timeout configuration to prevent long-running requests from blocking the application, as suggested in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/pkg/analytics.ts(2 hunks)apps/api/src/pkg/clickhouse-proxy.ts(1 hunks)apps/api/src/pkg/env.ts(1 hunks)apps/api/src/pkg/middleware/init.ts(1 hunks)
🧠 Learnings (4)
apps/api/src/pkg/env.ts (2)
Learnt from: ogzhanolguncu
PR: #3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Learnt from: chronark
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.
apps/api/src/pkg/middleware/init.ts (2)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2832
File: go/cmd/api/main.go:88-88
Timestamp: 2025-01-24T08:55:18.790Z
Learning: The metrics middleware in the API v2 implementation is intentionally initialized with nil configuration during development, with the understanding that proper configuration will be added before production deployment.
apps/api/src/pkg/analytics.ts (3)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
Learnt from: chronark
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.
apps/api/src/pkg/clickhouse-proxy.ts (2)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
🪛 GitHub Actions: autofix.ci
apps/api/src/pkg/analytics.ts
[error] 52-52: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[error] 79-79: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[error] 116-116: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[warning] 50-50: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
[warning] 77-77: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
[warning] 114-114: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
🧰 Additional context used
🧠 Learnings (4)
apps/api/src/pkg/env.ts (2)
Learnt from: ogzhanolguncu
PR: #3292
File: apps/dashboard/lib/trpc/routers/key/create.ts:11-14
Timestamp: 2025-06-02T11:09:58.791Z
Learning: In the unkey codebase, TypeScript and the env() function implementation already provide sufficient validation for environment variables, so additional runtime error handling for missing env vars is not needed.
Learnt from: chronark
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.
apps/api/src/pkg/middleware/init.ts (2)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2832
File: go/cmd/api/main.go:88-88
Timestamp: 2025-01-24T08:55:18.790Z
Learning: The metrics middleware in the API v2 implementation is intentionally initialized with nil configuration during development, with the understanding that proper configuration will be added before production deployment.
apps/api/src/pkg/analytics.ts (3)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
Learnt from: chronark
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.
apps/api/src/pkg/clickhouse-proxy.ts (2)
Learnt from: chronark
PR: #2825
File: apps/dashboard/app/(app)/logs/components/table/logs-table.tsx:100-118
Timestamp: 2025-01-31T13:50:45.004Z
Learning: Sensitive data in logs is masked at the API layer (apps/api/src/pkg/middleware/metrics.ts) before being written to Clickhouse. This includes redacting API keys, plaintext values, and authorization headers using regex replacements.
Learnt from: chronark
PR: #2544
File: apps/api/src/pkg/env.ts:4-6
Timestamp: 2024-10-23T12:05:31.121Z
Learning: The cloudflareRatelimiter type definition in apps/api/src/pkg/env.ts should not have its interface changed; it should keep the limit method returning Promise<{ success: boolean }> without additional error properties.
🪛 GitHub Actions: autofix.ci
apps/api/src/pkg/analytics.ts
[error] 52-52: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[error] 79-79: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[error] 116-116: biome lint/style/noNonNullAssertion: Forbidden non-null assertion. Unsafe fix suggested: replace with optional chain operator ?.
[warning] 50-50: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
[warning] 77-77: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
[warning] 114-114: Suppression comment has no effect. Remove the suppression or make sure you are suppressing the correct rule.
⏰ 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 Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/api/src/pkg/env.ts (1)
35-36: LGTM! Environment schema properly configured for proxy client.The addition of optional
CLICKHOUSE_PROXY_URLandCLICKHOUSE_PROXY_TOKENproperties follows the existing schema pattern and maintains backward compatibility.apps/api/src/pkg/middleware/init.ts (1)
99-103: LGTM! Analytics initialization correctly updated for proxy configuration.The middleware properly passes the new proxy URL and token from environment variables to the Analytics constructor, maintaining consistency with the updated schema.
apps/api/src/pkg/analytics.ts (2)
10-26: Constructor properly handles optional proxy configuration.The conditional instantiation of the proxy client when both URL and token are provided is correct and maintains backward compatibility.
63-84: Fix the placement of biome-ignore comments.Similar to the previous method, move the biome-ignore comment from line 76 to line 77.
- // biome-ignore lint/style/noNonNullAssertion: <explanation> + // biome-ignore lint/style/noNonNullAssertion: proxyClient existence verified above return await wrap(this.proxyClient!.insertVerifications([event]), err => new FetchError(⛔ Skipped due to learnings
Learnt from: CR PR: unkeyed/unkey#0 File: go/deploy/CLAUDE.md:0-0 Timestamp: 2025-07-21T18:05:58.236Z Learning: Applies to go/deploy/**/*.{go,js,ts,tsx,py,sh,md,txt,json,yaml,yml,ini,env,conf,html,css,scss,xml,c,h,cpp,java,rb,rs,php,pl,sql} : Make sure to add relevant anchor comments whenever a file or piece of code is too complex, very important, confusing, or could have a bug.apps/api/src/pkg/clickhouse-proxy.ts (2)
1-13: Well-structured proxy client implementation.The class properly encapsulates the proxy communication logic with clear separation of concerns. The constructor correctly normalizes the base URL.
92-92: Good practice: Preventing metrics recursion.The
X-Unkey-Metrics: disabledheader prevents recursive metrics collection when the proxy itself generates metrics, avoiding potential infinite loops.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Graphite Automations"Notify author when CI fails" took an action on this PR • (07/23/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 • (07/23/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
|
nvm someone else has to approvie this |
|
|
3 participants
What does this PR do?
Adds a ClickHouse proxy client to support sending analytics events through an internal proxy service instead of directly to ClickHouse. This implementation:
ClickHouseProxyClientclass that handles sending batched events to internal proxy endpointsAnalyticsclass to conditionally use the proxy client for writes when configuredFixes # (issue)
Type of change
How should this be tested?
clickhouseInsertUrlandclickhouseProxyTokenand verify events are sent through the proxyChecklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores