feat: add minute granularity - This should be merged before key details#3240
feat: add minute granularity - This should be merged before key details#3240ogzhanolguncu merged 4 commits intomainfrom
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 ↗︎
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe changes introduce minute-level granularity support for verification and active key timeseries queries, including new ClickHouse tables and materialized views for per-minute aggregation. The backend utilities and query functions are extended to allow selection of minute, five-minute, and thirty-minute intervals, with optional control over their usage in the granularity utility. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DashboardAPI
participant GranularityUtil
participant ClickHouse
participant ClickHouseDB
Client->>DashboardAPI: Request verification timeseries (with allowMinutelyForVerifications)
DashboardAPI->>GranularityUtil: getTimeseriesGranularity(params, allowMinutelyForVerifications)
GranularityUtil-->>DashboardAPI: Return selected granularity (minute, 5min, 30min, hour, etc.)
DashboardAPI->>ClickHouse: Query timeseries via appropriate function (e.g., perMinute)
ClickHouse->>ClickHouseDB: Query per-minute (or selected) table/materialized view
ClickHouseDB-->>ClickHouse: Return aggregated timeseries data
ClickHouse-->>DashboardAPI: Return timeseries data
DashboardAPI-->>Client: Respond with timeseries data
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (19)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (2)
55-56: Document parameter usage and future plans.The comment indicates this parameter is temporary and will be removed after the key details feature is merged. Consider adding more context about why this approach was chosen and what will replace it in the future.
97-103: Simplify redundant conditions in the time range logic.There are redundant conditions that always result in the same "perHour" granularity:
- Lines 97-100 all set granularity to "perHour" for time ranges between 1-3 days
- Lines 101-102 set granularity to "perHour" for time ranges between 8-16 hours
These could be simplified to reduce code complexity.
- if (timeRange >= DAY_IN_MS * 3) { - granularity = "perHour"; - } else if (timeRange >= DAY_IN_MS) { - granularity = "perHour"; - } else if (timeRange >= HOUR_IN_MS * 16) { - granularity = "perHour"; + if (timeRange >= HOUR_IN_MS * 16) { + granularity = "perHour";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/dashboard/lib/trpc/routers/utils/granularity.ts(2 hunks)internal/clickhouse/schema/050_create_verifications.key_verifications_per_minute_v1.sql(1 hunks)internal/clickhouse/schema/051_create_verifications.key_verifications_per_minute_mv_v1.sql(1 hunks)internal/clickhouse/src/index.ts(3 hunks)internal/clickhouse/src/keys/active_keys.ts(8 hunks)internal/clickhouse/src/verifications.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/dashboard/lib/trpc/routers/utils/granularity.ts (1)
apps/dashboard/lib/trpc/routers/utils/constants.ts (2)
DAY_IN_MS(2-2)HOUR_IN_MS(1-1)
internal/clickhouse/src/index.ts (2)
internal/clickhouse/src/verifications.ts (12)
getMinutelyVerificationTimeseries(510-512)getFiveMinutelyVerificationTimeseries(513-515)getThirtyMinutelyVerificationTimeseries(516-518)getHourlyVerificationTimeseries(521-523)getTwoHourlyVerificationTimeseries(524-526)getFourHourlyVerificationTimeseries(527-529)getSixHourlyVerificationTimeseries(530-532)getTwelveHourlyVerificationTimeseries(533-535)getDailyVerificationTimeseries(538-540)getThreeDayVerificationTimeseries(541-543)getWeeklyVerificationTimeseries(544-546)getMonthlyVerificationTimeseries(549-551)internal/clickhouse/src/keys/active_keys.ts (11)
getMinutelyActiveKeysTimeseries(291-293)getFiveMinutelyActiveKeysTimeseries(294-296)getThirtyMinutelyActiveKeysTimeseries(297-299)getHourlyActiveKeysTimeseries(302-304)getTwoHourlyActiveKeysTimeseries(305-307)getFourHourlyActiveKeysTimeseries(308-310)getSixHourlyActiveKeysTimeseries(311-313)getTwelveHourlyActiveKeysTimeseries(314-316)getDailyActiveKeysTimeseries(319-321)getThreeDayActiveKeysTimeseries(322-324)getWeeklyActiveKeysTimeseries(325-327)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: Test Go API Local / Test (Shard 4/8)
- GitHub Check: Test Go API Local / Test (Shard 8/8)
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Go API Local / Test (Shard 5/8)
- GitHub Check: Test Go API Local / Test (Shard 3/8)
- GitHub Check: Test Go API Local / Test (Shard 6/8)
- GitHub Check: Test Go API Local / Test (Shard 2/8)
- GitHub Check: Test Go API Local / Test (Shard 1/8)
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./apps/dashboard
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
internal/clickhouse/schema/051_create_verifications.key_verifications_per_minute_mv_v1.sql (1)
1-23: LGTM! Well-structured materialized view for minute-level aggregation.The materialized view correctly:
- Aggregates raw verification data at minute-level granularity
- Groups by all relevant dimensions
- Uses toStartOfMinute for proper timestamp bucketing
- Targets the appropriate table
internal/clickhouse/schema/050_create_verifications.key_verifications_per_minute_v1.sql (1)
1-15: LGTM! Schema is well-designed for minute-level aggregation.The table structure is appropriately designed with:
- SummingMergeTree engine for efficient aggregation workloads
- Proper columns for all required dimensions
- Effective ordering for query performance
internal/clickhouse/src/keys/active_keys.ts (3)
290-299: LGTM! New minute-based timeseries functions are properly implemented.The new exported functions for minute-level granularity are correctly implemented using the timeseries querier factory pattern, consistent with the existing hour, day, and month-based functions.
301-338: Good organizational improvement with granularity comments.Adding comments to group timeseries functions by granularity type (minute, hour, day, month) improves code readability and organization.
63-78:Details
✅ Verification successful
Verify the table reference for minute-based intervals.
All minute-based intervals reference "verifications.key_verifications_per_minute_v1" table. Confirm that active keys data should be queried from the verifications table rather than having a dedicated active_keys_per_minute table.
🏁 Script executed:
#!/bin/bash # Check if there's an active_keys_per_minute table or if active keys data is stored in the verifications table rg -A 3 "CREATE TABLE.*active_keys.*minute" --glob "*.sql" rg -A 3 "FROM verifications.key_verifications_per_minute" --glob "*.ts" | grep -v "active_keys.ts"Length of output: 159
🏁 Script executed:
#!/bin/bash # Broad search for any SQL table or view related to active keys rg -n 'active_keys' --glob '*.sql' rg -n 'active_keys_per_minute' --glob '*.sql' rg -n 'key_active_keys_per_minute_v1' --glob '*.sql' # Search references in TS code rg -n 'active_keys' --glob '*.ts' rg -n 'active_keys_per_minute' --glob '*.ts'Length of output: 277
Minute-based intervals use the correct table
I don’t see anyactive_keys_per_minutetable in our SQL definitions, so querying fromverifications.key_verifications_per_minute_v1for all minute-based intervals is appropriate.internal/clickhouse/src/index.ts (6)
5-11: Import organization looks good.The addition of import statements for the minute-level granularity functions is well-organized and follows the existing code pattern.
61-67: Import structure is consistent.The new verification timeseries import statements are properly organized alongside existing imports, maintaining the codebase's style.
121-124: Well-structured minute granularity addition for verification timeseries.The new minute-based granularity methods are properly implemented in the
verifications.timeseriesobject with a clear comment indicating the granularity level. This follows the PR objective of adding minute-level granularity support.
131-136: Good organization with descriptive comments.Adding section comments to distinguish between granularity levels improves code readability and maintainability.
139-143: Consistent implementation for active keys timeseries.The minute-based granularity functions for active keys are implemented consistently with the verification timeseries pattern.
149-154: Consistent organization across the codebase.The section comments for day-based and month-based granularities maintain the same structure as the added minute-based sections, ensuring consistency throughout the file.
internal/clickhouse/src/verifications.ts (7)
197-212: Well-defined intervals for minute-based granularity.The addition of minute, five-minute, and thirty-minute intervals is well-structured and consistent with the existing interval definitions. All new intervals correctly reference the new
verifications.key_verifications_per_minute_v1table.
275-277: Appropriate interval unit mapping for minutes.The interval unit mappings for "MINUTE" and "MINUTES" are correctly set to "minute", consistent with how other units are mapped.
287-289: Correct millisecond conversion for minute intervals.The millisecond per unit mappings for "MINUTE" and "MINUTES" are properly set to 60,000 (60 seconds * 1000 milliseconds), ensuring accurate time calculations.
509-519: Well-implemented minute-based timeseries functions.The new minute-based verification timeseries functions are implemented correctly, following the established pattern and using the appropriate interval constants.
520-536: Good organization with descriptive comments.Adding the "Hour-based timeseries" comment improves code organization and readability, making it clear which functions operate at which time granularity.
537-547: Consistent commenting for day-based timeseries.The "Day-based timeseries" comment maintains the organizational structure introduced with the minute and hour sections.
548-551: Complete organization with month-based comment.The "Month-based timeseries" comment finishes the organizational structure, ensuring all granularity levels are clearly labeled.
|
could you add a copy of the table schemas here as well please. no downlevel migrations, just the |
|
lemme see |
2 participants
What does this PR do?
This PR adds minute granularity for the upcoming Keys Details page and better chart results. We have to merge this first so we can have some data before we actually enable the minute granularity for charts.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Improvements