feat: add outcome sort to ratelimit overview#2981
Conversation
ogzhanolguncu
requested review from
MichaelUnkey,
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes add sorting functionality for the "passed" and "blocked" columns in the rate limit logs overview. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Table as RatelimitOverviewLogsTable
participant API as getRatelimitOverviewLogs
participant DB as Clickhouse
User->>Table: Click "passed"/"blocked" column header
Table->>Table: Execute onSort (calls toggleSort)
Table->>API: Send updated sort parameters (e.g., column "passed")
API->>DB: Query logs sorted by "passed_count" (or "blocked_count")
DB-->>API: Return sorted logs data
API-->>Table: Send sorted logs data
Table-->>User: Display sorted logs
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
|
don't merge it |
|
ok |
|
i'll tell you 😄 |
|
then make it a draft |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (1)
55-61: Clarify the default sort direction for "passed".
It’s not immediately clear whytoggleSort("passed", false)is set tofalse. Consider documenting or renaming this parameter to improve clarity of the logic (e.g., specifyingisAscendingvs.isDescending).internal/clickhouse/src/ratelimits.ts (2)
537-543: Consider unifying the detection of custom sorts.
Having multiple checks forhasAvgLatencySort,hasP99LatencySort, etc. is fine, but you can unify them into a single function that inspects each sort rule, which might reduce repetition and simplify future expansions.
607-607: Check dynamic insertion ofcursorCondition.
Confirm that string interpolation of${cursorCondition}is reliable and that special characters or spacing do not interrupt the query. It might be safer to build the full query outside the template literal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx(3 hunks)internal/clickhouse/src/ratelimits.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/clickhouse/src/ratelimits.ts (1)
Learnt from: ogzhanolguncu
PR: unkeyed/unkey#2896
File: internal/clickhouse/src/ratelimits.ts:468-592
Timestamp: 2025-03-12T13:07:07.472Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
apps/dashboard/app/(app)/ratelimits/[namespaceId]/_overview/components/table/logs-table.tsx (2)
88-94: Extend test coverage for "blocked" column sorting.
Similar to "passed", ensure that the new sorting capability for "blocked" is tested to avoid regressions. Verifying that sorting is correct for both ascending and descending orders would be helpful.
175-175: Confirm consistent sorting approach for "time".
Previously, thetimesort might have used a different default direction. Verify that settingfalsealigns with the intended default (ascending or descending) to maintain consistency across columns.internal/clickhouse/src/ratelimits.ts (4)
437-437: Validate additional sort fields in the schema.
Adding"blocked"and"passed"to thez.enumis correct. Please confirm there are no other fields (e.g.,"avg_latency") missing that might need to be included.
517-518: Ensure column mapping is correct for "passed" and "blocked".
The new mappings"passed" -> "passed_count"and"blocked" -> "blocked_count"appear logically sound. Verify that these fields exist and return accurate counts in all usage scenarios.
547-553: Revisit defaulting to "ASC" for custom sorts.
The code enforces ascending order if any of the custom sort columns are present. Verify that this matches the intended UX, as some users might expect descending order (show highest counts first).
566-592: Verify cursor pagination logic for both ascending and descending sorts.
The new conditions properly distinguish between ascending and descending to compare request timestamps and IDs. Given prior learnings about cursor usage, ensure real usage patterns behave as expected.
|
we'll merge this first then api-overview feel free to merge it after you check |
2 participants
What does this PR do?
Fixes # (issue)
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit