Skip to content

fix: Ensure last ratelimit usage is shown #2721

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.

Sign up for GitHub

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

Merged
chronark merged 7 commits into from
Dec 10, 2024
Merged

fix: Ensure last ratelimit usage is shown #2721

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0188af2
fix: order by time and add limit
Flo4604 Dec 8, 2024
c0bf725
chore: go mod tidy
Flo4604 Dec 8, 2024
8008347
fix: remove file that is not in use anymore
Flo4604 Dec 8, 2024
ecf8e84
Merge branch 'main' into fix/ratelimit-last-used
chronark Dec 9, 2024
ba912ee
fix: add missing limit...
Flo4604 Dec 9, 2024
fb32106
Merge branch 'main' into fix/ratelimit-last-used
Flo4604 Dec 9, 2024
31c28e6
Merge branch 'main' into fix/ratelimit-last-used
Flo4604 Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions apps/chproxy/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
module github.com/unkeyed/unkey/apps/chproxy

go 1.23.2

require (
github.com/influxdata/tdigest v0.0.1 // indirect
github.com/josharian/intern v1.0.0 // indirect
github.com/mailru/easyjson v0.7.7 // indirect
github.com/tsenart/vegeta v12.7.0+incompatible // indirect
golang.org/x/net v0.31.0 // indirect
golang.org/x/text v0.20.0 // indirect
)
17 changes: 0 additions & 17 deletions apps/chproxy/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +0,0 @@
github.com/google/go-cmp v0.2.0/go.mod h1:oXzfMopK8JAjlY9xF4vHSVASa0yLyX7SntLO5aqRK0M=
github.com/influxdata/tdigest v0.0.1 h1:XpFptwYmnEKUqmkcDjrzffswZ3nvNeevbUSLPP/ZzIY=
github.com/influxdata/tdigest v0.0.1/go.mod h1:Z0kXnxzbTC2qrx4NaIzYkE1k66+6oEDQTvL95hQFh5Y=
github.com/josharian/intern v1.0.0 h1:vlS4z54oSdjm0bgjRigI+G1HpF+tI+9rE5LLzOg8HmY=
github.com/josharian/intern v1.0.0/go.mod h1:5DoeVV0s6jJacbCEi61lwdGj/aVlrQvzHFFd8Hwg//Y=
github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0=
github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
github.com/tsenart/vegeta v12.7.0+incompatible h1:sGlrv11EMxQoKOlDuMWR23UdL90LE5VlhKw/6PWkZmU=
github.com/tsenart/vegeta v12.7.0+incompatible/go.mod h1:Smz/ZWfhKRcyDDChZkG3CyTHdj87lHzio/HOCkbndXM=
golang.org/x/exp v0.0.0-20180321215751-8460e604b9de/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
golang.org/x/net v0.31.0 h1:68CPQngjLL0r2AlUKiSxtQFKvzRVbnzLwMUn5SzcLHo=
golang.org/x/net v0.31.0/go.mod h1:P4fl1q7dY2hnZFxEk4pPSkDHF+QqjitcnDjUQyMM+pM=
golang.org/x/text v0.20.0 h1:gK/Kv2otX8gz+wn7Rmb3vT96ZwuoxnQlY+HlJVj7Qug=
golang.org/x/text v0.20.0/go.mod h1:D4IsuqiFMhST5bX19pQ9ikHC2GsaKyk/oF+pn3ducp4=
golang.org/x/tools v0.0.0-20180525024113-a5b4c53f6e8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
gonum.org/v1/gonum v0.0.0-20181121035319-3f7ecaa7e8ca/go.mod h1:Y+Yx5eoAFn32cQvJDxZx5Dpnq+c3wtXuadVZAcxbbBo=
gonum.org/v1/netlib v0.0.0-20181029234149-ec6d1f5cefe6/go.mod h1:wa6Ws7BG/ESfp6dHfk7C6KdzKA7wR7u/rKwOGE66zvw=
1 change: 1 addition & 0 deletions apps/dashboard/app/(app)/ratelimits/[namespaceId]/overrides/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ const LastUsed: React.FC<{
workspaceId,
namespaceId,
identifier: [identifier],
limit: 1,
});

const unixMilli = lastUsed.val?.at(0)?.time;
Expand Down
6 changes: 5 additions & 1 deletion apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ export default async function RatelimitNamespacePage(props: {
end: billingCycleEnd,
}).then((res) => res.val!),
clickhouse.ratelimits
.latest({ workspaceId: namespace.workspaceId, namespaceId: namespace.id })
.latest({
workspaceId: namespace.workspaceId,
namespaceId: namespace.id,
limit: 1,
})
Comment on lines +80 to +84
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Inconsistent usage of limit parameter across .latest() calls needs attention

The verification reveals that the limit: 1 parameter is only used in ratelimit-related components:

  • apps/dashboard/app/(app)/ratelimits/card.tsx
  • apps/dashboard/app/(app)/ratelimits/[namespaceId]/page.tsx

Other components using .latest() either:

  1. Don't use the limit parameter and rely on array access (.at(0)) to get the latest entry
  2. Have different parameter requirements (like the overrides table)

This inconsistency in implementation could lead to similar timestamp-related issues in other components.

🔗 Analysis chain

LGTM! The fix correctly implements the last used timestamp retrieval.

The addition of limit: 1 ensures we get only the most recent entry from the ratelimits table, fixing the bug where the first timestamp was being shown instead of the last used time.

Let's verify that this change is consistently implemented across all components that use the latest method:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if all components using clickhouse.ratelimits.latest include the limit parameter

# Search for all usages of the latest method
rg -A 3 "clickhouse\.ratelimits\.latest\(" --type ts --type tsx

Length of output: 93

Script:

#!/bin/bash
# Search for all usages of the latest method without file type restrictions
rg -A 3 "\.latest\("

Length of output: 2563

.then((res) => res.val?.at(0)?.time),
]);

Expand Down
6 changes: 5 additions & 1 deletion apps/dashboard/app/(app)/ratelimits/card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ export const RatelimitCard: React.FC<Props> = async ({ workspace, namespace }) =
})
.then((res) => res.val!),
clickhouse.ratelimits
.latest({ workspaceId: workspace.id, namespaceId: namespace.id })
.latest({
workspaceId: workspace.id,
namespaceId: namespace.id,
limit: 1,
})
.then((res) => res.val?.at(0)?.time),
]);

Expand Down
36 changes: 0 additions & 36 deletions internal/clickhouse/src/last_used.ts
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions internal/clickhouse/src/ratelimits.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ const getRatelimitLastUsedParameters = z.object({
workspaceId: z.string(),
namespaceId: z.string(),
identifier: z.array(z.string()).optional(),
limit: z.number().int(),
});

export function getRatelimitLastUsed(ch: Querier) {
Expand All @@ -213,6 +214,8 @@ export function getRatelimitLastUsed(ch: Querier) {
AND namespace_id = {namespaceId: String}
${args.identifier ? "AND multiSearchAny(identifier, {identifier: Array(String)}) > 0" : ""}
GROUP BY identifier
ORDER BY time DESC
LIMIT {limit: Int}
;`,
params: getRatelimitLastUsedParameters,
schema: z.object({
Expand Down