-
Notifications
You must be signed in to change notification settings - Fork 529
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: clickhouse migration #2 #2135
Changes from 31 commits
dc19429
016bc0a
da476be
28728e3
0b1ecfb
a63a55e
87089f4
1dc863c
68bd7e2
92d5d7f
a180ebc
884a127
c759550
1914227
879ffc3
8562c03
ee5757d
df5a4e1
4c61745
7e3a75e
66584f4
ca01f03
296e27d
cb0e0b4
e1a7ba6
3a878fd
b453a1b
538392b
966890f
fa0b112
b0650ed
737262c
69093ae
ed3fa8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ tasks: | |
env: | ||
GOOSE_DRIVER: clickhouse | ||
GOOSE_DBSTRING: "tcp://default:password@127.0.0.1:9000" | ||
GOOSE_MIGRATION_DIR: ./apps/agent/pkg/clickhouse/schema | ||
GOOSE_MIGRATION_DIR: ./internal/clickhouse/schema | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Migration files successfully moved, but cleanup needed in old location The migration files have been successfully moved to the new location at
🔗 Analysis chainVerify migration files location. The migration path has been updated from Let's verify that the migration files exist in the new location: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify migration files have been moved to the new location
# Expected: Migration files should exist in the new location
# Check if migration files exist in the new location
fd -t f . "internal/clickhouse/schema"
# Ensure no migration files are left in the old location
fd -t f . "apps/agent/pkg/clickhouse/schema"
Length of output: 2070 |
||
cmds: | ||
- goose up | ||
|
||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
"@hono/zod-validator": "^0.2.1", | ||
"@planetscale/database": "^1.16.0", | ||
"@unkey/cache": "workspace:^", | ||
"@unkey/clickhouse-zod": "workspace:^", | ||
"@unkey/clickhouse": "workspace:^", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Update remaining import in There is still one file using the old
🔗 Analysis chainVerify migration from @unkey/clickhouse-zod. Let's ensure all imports have been updated to use the new package. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining references to the old package
# and verify that the new package is properly imported where needed
# Check for any remaining references to the old package
echo "Checking for remaining references to @unkey/clickhouse-zod:"
rg "@unkey/clickhouse-zod"
# Check current usage of the new package
echo -e "\nVerifying imports of @unkey/clickhouse:"
rg -A 2 "from ['\"']@unkey/clickhouse['\"]"
Length of output: 848 |
||
"@unkey/db": "workspace:^", | ||
"@unkey/encryption": "workspace:^", | ||
"@unkey/error": "workspace:^", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { NoopTinybird, Tinybird } from "@chronark/zod-bird"; | ||
import * as ch from "@unkey/clickhouse-zod"; | ||
import { ClickHouse } from "@unkey/clickhouse"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add validation for required ClickHouse configuration. The optional chaining on constructor(opts: {
tinybirdToken?: string;
tinybirdProxy?: {
url: string;
token: string;
};
- clickhouse?: {
+ clickhouse: {
url: string;
};
}) {
this.readClient = opts.tinybirdToken
? new Tinybird({ token: opts.tinybirdToken })
: new NoopTinybird();
this.writeClient = opts.tinybirdProxy
? new Tinybird({ token: opts.tinybirdProxy.token, baseUrl: opts.tinybirdProxy.url })
: this.readClient;
- this.clickhouse = new ClickHouse({ url: opts.clickhouse?.url });
+ if (!opts.clickhouse?.url) {
+ throw new Error("ClickHouse configuration is required");
+ }
+ this.clickhouse = new ClickHouse({ url: opts.clickhouse.url });
} Also applies to: 21-21, 41-41 |
||
import { newId } from "@unkey/id"; | ||
import { auditLogSchemaV1, unkeyAuditLogEvents } from "@unkey/schema/src/auditlog"; | ||
import { ratelimitSchemaV1 } from "@unkey/schema/src/ratelimit-tinybird"; | ||
|
@@ -18,7 +18,7 @@ const dateToUnixMilli = z.string().transform((t) => new Date(t.split(" ").at(0) | |
export class Analytics { | ||
public readonly readClient: Tinybird | NoopTinybird; | ||
public readonly writeClient: Tinybird | NoopTinybird; | ||
private clickhouse: ch.Clickhouse; | ||
private clickhouse: ClickHouse; | ||
|
||
constructor(opts: { | ||
tinybirdToken?: string; | ||
|
@@ -38,12 +38,12 @@ export class Analytics { | |
? new Tinybird({ token: opts.tinybirdProxy.token, baseUrl: opts.tinybirdProxy.url }) | ||
: this.readClient; | ||
|
||
this.clickhouse = opts.clickhouse ? new ch.Client({ url: opts.clickhouse.url }) : new ch.Noop(); | ||
this.clickhouse = new ClickHouse({ url: opts.clickhouse?.url }); | ||
} | ||
|
||
public get insertSdkTelemetry() { | ||
return this.clickhouse.insert({ | ||
table: "default.raw_telemetry_sdks_v1", | ||
return this.clickhouse.client.insert({ | ||
table: "telemetry.raw_sdks_v1", | ||
schema: z.object({ | ||
request_id: z.string(), | ||
time: z.number().int(), | ||
|
@@ -107,6 +107,20 @@ export class Analytics { | |
})), | ||
}); | ||
} | ||
public get insertRatelimit() { | ||
return this.clickhouse.client.insert({ | ||
table: "ratelimits.raw_ratelimits_v1", | ||
schema: z.object({ | ||
request_id: z.string(), | ||
time: z.number().int(), | ||
workspace_id: z.string(), | ||
namespace_id: z.string(), | ||
identifier: z.string(), | ||
passed: z.boolean(), | ||
}), | ||
}); | ||
} | ||
|
||
//tinybird | ||
public get ingestRatelimit() { | ||
return this.writeClient.buildIngestEndpoint({ | ||
|
@@ -116,8 +130,8 @@ export class Analytics { | |
} | ||
|
||
public get insertKeyVerification() { | ||
return this.clickhouse.insert({ | ||
table: "default.raw_key_verifications_v1", | ||
return this.clickhouse.client.insert({ | ||
table: "verifications.raw_key_verifications_v1", | ||
schema: z.object({ | ||
request_id: z.string(), | ||
time: z.number().int(), | ||
|
@@ -140,8 +154,8 @@ export class Analytics { | |
} | ||
|
||
public get insertApiRequest() { | ||
return this.clickhouse.insert({ | ||
table: "default.raw_api_requests_v1", | ||
return this.clickhouse.client.insert({ | ||
table: "metrics.raw_api_requests_v1", | ||
schema: z.object({ | ||
request_id: z.string(), | ||
time: z.number().int(), | ||
|
@@ -158,6 +172,10 @@ export class Analytics { | |
service_latency: z.number().int(), | ||
user_agent: z.string(), | ||
ip_address: z.string(), | ||
continent: z.string().nullable().default(""), | ||
city: z.string().nullable().default(""), | ||
country: z.string().nullable().default(""), | ||
colo: z.string().nullable().default(""), | ||
}), | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -678,7 +678,7 @@ export class KeyService { | |
} | ||
|
||
return [ | ||
res.val.pass, | ||
res.val.passed, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Inconsistency found in property naming The change from
🔗 Analysis chainLGTM! Property rename for consistency. The change from Let's verify that all usages of this property have been updated consistently: Let me analyze the search results further to ensure all instances are properly updated. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Search for any remaining instances of the old property name 'pass'
# that might need to be updated to 'passed' in rate limiting related code
# Search for 'pass' property in rate limit related code
rg -i 'pass' --type ts | grep -i 'ratelimit'
# Search for rate limit response type definitions
ast-grep --pattern 'interface $name {
$$$
pass: boolean
$$$
}'
Length of output: 6189 Script: #!/bin/bash
# Let's check the specific interface definitions and usages in the ratelimit related code
ast-grep --pattern 'interface $name {
$$$
passed: boolean
$$$
}'
# Also check the specific file for any remaining 'pass' references
rg 'pass' apps/api/src/pkg/keys/service.ts
Length of output: 263 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Property name change needs attention in The search results show that while the property name has been changed to const [pass, ratelimit] = await this.ratelimit(c, data.key, ratelimits);
if (!pass) {
// ...
} This should be updated to match the new property name for consistency:
🔗 Analysis chainProperty name change from The change aligns with the broader refactoring effort across the codebase to use consistent property names. Let's verify the consistency of this naming change across the codebase: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any remaining instances of the old property name 'pass' in rate limiting context
rg -A 3 -B 3 'pass.*ratelimit|ratelimit.*pass'
# Search for the new property name 'passed' to ensure consistent usage
rg -A 3 -B 3 'passed.*ratelimit|ratelimit.*passed'
Length of output: 22429 |
||
{ | ||
remaining: res.val.remaining, | ||
limit: ratelimits.default?.limit, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,14 @@ export function metrics(): MiddlewareHandler<HonoEnv> { | |
service_latency: Date.now() - c.get("requestStartedAt"), | ||
ip_address: c.req.header("True-Client-IP") ?? c.req.header("CF-Connecting-IP") ?? "", | ||
user_agent: c.req.header("User-Agent") ?? "", | ||
// @ts-ignore - this is a bug in the types | ||
continent: c.req.raw?.cf?.continent, | ||
// @ts-ignore - this is a bug in the types | ||
country: c.req.raw?.cf?.country, | ||
// @ts-ignore - this is a bug in the types | ||
colo: c.req.raw?.cf?.colo, | ||
// @ts-ignore - this is a bug in the types | ||
city: c.req.raw?.cf?.city, | ||
Comment on lines
+128
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Fix TypeScript type issues for Cloudflare request data. Instead of using @ts-ignore comments, consider properly typing the Cloudflare-specific request data. This will improve type safety and maintainability. // Add this type declaration in a separate types file
interface CloudflareRequestData {
cf?: {
continent?: string;
country?: string;
colo?: string;
city?: string;
};
}
// Extend the Request type
declare global {
interface Request extends CloudflareRequestData {}
}
Comment on lines
+128
to
+135
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider fixing TypeScript types and reducing code duplication. The geographical data collection has two issues:
Consider these improvements:
// Add this at the top of the file
interface CloudflareRequestData {
cf?: {
continent?: string;
country?: string;
colo?: string;
city?: string;
};
}
// Update the HonoEnv to include the CF data
interface HonoEnv {
// ... existing types ...
Bindings: {
req: Request & {
raw: Request & CloudflareRequestData;
};
};
}
export function metrics(): MiddlewareHandler<HonoEnv> {
return async (c, next) => {
const { metrics, analytics, logger } = c.get("services");
+ // Extract CF data once
+ const cfData = {
+ continent: c.req.raw?.cf?.continent,
+ country: c.req.raw?.cf?.country,
+ colo: c.req.raw?.cf?.colo,
+ city: c.req.raw?.cf?.city,
+ };
let requestBody = await c.req.raw.clone().text();
requestBody = requestBody.replaceAll(/"key":\s*"[a-zA-Z0-9_]+"/g, '"key": "<REDACTED>"');
const start = performance.now();
const m = {
isolateId: c.get("isolateId"),
isolateLifetime: Date.now() - c.get("isolateCreatedAt"),
metric: "metric.http.request",
path: c.req.path,
host: new URL(c.req.url).host,
method: c.req.method,
- // @ts-ignore - this is a bug in the types
- continent: c.req.raw?.cf?.continent,
- // @ts-ignore - this is a bug in the types
- country: c.req.raw?.cf?.country,
- // @ts-ignore - this is a bug in the types
- colo: c.req.raw?.cf?.colo,
- // @ts-ignore - this is a bug in the types
- city: c.req.raw?.cf?.city,
+ ...cfData,
userAgent: c.req.header("user-agent"),
fromAgent: c.req.header("Unkey-Redirect"),
context: {},
} as DiscriminateMetric<"metric.http.request">;
// ... rest of the code ...
c.executionCtx.waitUntil(
analytics.insertApiRequest({
// ... other fields ...
- // @ts-ignore - this is a bug in the types
- continent: c.req.raw?.cf?.continent,
- // @ts-ignore - this is a bug in the types
- country: c.req.raw?.cf?.country,
- // @ts-ignore - this is a bug in the types
- colo: c.req.raw?.cf?.colo,
- // @ts-ignore - this is a bug in the types
- city: c.req.raw?.cf?.city,
+ ...cfData,
}),
); |
||
}), | ||
); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
The migration files have been successfully moved to the new location, but there are several files still referencing the old path that need to be updated:
apps/agent/services/eventrouter/service.go
apps/agent/pkg/clickhouse/client.go
apps/agent/pkg/clickhouse/noop.go
apps/agent/pkg/clickhouse/interface.go
apps/agent/pkg/api/interface.go
apps/agent/pkg/api/mw_logging.go
These files need to have their import statements updated from
github.com/unkeyed/unkey/apps/agent/pkg/clickhouse/schema
to match the new path.🔗 Analysis chain
LGTM! Verify migration files transfer.
The path change from
./apps/agent/pkg/clickhouse/schema
to./internal/clickhouse/schema
aligns with standard Go project layout.Let's verify all migration files were transferred correctly:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 194
Script:
Length of output: 6027