-
Notifications
You must be signed in to change notification settings - Fork 201
chore: improve gql_schema_usage_5m_90d table #2432
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
base: main
Are you sure you want to change the base?
chore: improve gql_schema_usage_5m_90d table #2432
Conversation
WalkthroughA comprehensive database migration for the GraphQL metrics schema, introducing a new optimized table ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Pre-merge checks✅ Passed checks (3 passed)
Comment |
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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
graphqlmetrics/migrations/20251223110743_new_graphql_schema_usage_table_5m_90d.sql (1)
25-30: Indexidx_operation_hashremoved.The old table included a bloom filter index on
OperationHash, which is not present in the new schema. SinceOperationHashis part of the ORDER BY clause, ClickHouse can still efficiently filter on it using primary key indexing. This is likely acceptable, but verify that query performance onOperationHashfilters remains satisfactory after migration.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
graphqlmetrics/migrations/20251223110743_new_graphql_schema_usage_table_5m_90d.sqlgraphqlmetrics/migrations/20251223131620_migrate_data_90_to_68_days.sqlgraphqlmetrics/migrations/20251223131621_migrate_data_68_to_46_days.sqlgraphqlmetrics/migrations/20251223131622_migrate_data_46_to_24_days.sqlgraphqlmetrics/migrations/20251223131623_migrate_data_24_to_1_day.sqlgraphqlmetrics/migrations/20251223131624_migrate_data_last_1_day.sqlgraphqlmetrics/migrations/20251223131629_drop_old_gql_metrics_schema_usage_5m_90d.sqlgraphqlmetrics/migrations/20251223131720_rename_new_gql_metrics_schema_usage_5m_90d.sql
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2327
File: graphqlmetrics/core/metrics_service.go:435-442
Timestamp: 2025-11-12T18:38:46.619Z
Learning: In graphqlmetrics/core/metrics_service.go, the opGuardCache TTL (30 days) is intentionally shorter than the gql_metrics_operations database retention period (90 days). This design reduces memory usage while relying on database constraints to prevent duplicate inserts after cache expiration.
📚 Learning: 2025-11-12T18:38:46.619Z
Learnt from: StarpTech
Repo: wundergraph/cosmo PR: 2327
File: graphqlmetrics/core/metrics_service.go:435-442
Timestamp: 2025-11-12T18:38:46.619Z
Learning: In graphqlmetrics/core/metrics_service.go, the opGuardCache TTL (30 days) is intentionally shorter than the gql_metrics_operations database retention period (90 days). This design reduces memory usage while relying on database constraints to prevent duplicate inserts after cache expiration.
Applied to files:
graphqlmetrics/migrations/20251223131629_drop_old_gql_metrics_schema_usage_5m_90d.sqlgraphqlmetrics/migrations/20251223110743_new_graphql_schema_usage_table_5m_90d.sql
⏰ 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: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
graphqlmetrics/migrations/20251223110743_new_graphql_schema_usage_table_5m_90d.sql (1)
32-36: Verify ENGINE type change is intentional.The new table uses
SummingMergeTreewhile the down migration in20251223131629_drop_old_gql_metrics_schema_usage_5m_90d.sqlrecreates the old table withSharedSummingMergeTree. This is a significant change:
SharedSummingMergeTreeis for ClickHouse Cloud/replicated setupsSummingMergeTreeis for single-node deploymentsPlease confirm this ENGINE change aligns with your deployment topology.
graphqlmetrics/migrations/20251223131620_migrate_data_90_to_68_days.sql (1)
1-10: Data migration logic is sound.The time-windowed approach for migrating data in chunks (90-68 days) is appropriate for large tables to avoid long-running transactions. The empty down migration is acceptable since the original table retains the data until the drop migration runs.
graphqlmetrics/migrations/20251223131621_migrate_data_68_to_46_days.sql (1)
1-10: LGTM!Time window correctly specified with no overlap with adjacent migrations.
graphqlmetrics/migrations/20251223131622_migrate_data_46_to_24_days.sql (1)
1-10: LGTM!Consistent with the migration sequence pattern.
graphqlmetrics/migrations/20251223131623_migrate_data_24_to_1_day.sql (1)
1-10: LGTM!Time window boundaries are correct.
graphqlmetrics/migrations/20251223131629_drop_old_gql_metrics_schema_usage_5m_90d.sql (1)
1-42: Down migration recreates schema but not data.If a rollback is needed, the down migration will recreate an empty
gql_metrics_schema_usage_5m_90dtable. Combined with the empty down migrations in the data migration files and the rename down, a full rollback leaves you with:
gql_metrics_schema_usage_5m_90d_v2containing all data (renamed back fromgql_metrics_schema_usage_5m_90d)- A recreated but empty
gql_metrics_schema_usage_5m_90dThis may be acceptable if the rollback strategy is to fix forward, but document this limitation for operational awareness.
graphqlmetrics/migrations/20251223131720_rename_new_gql_metrics_schema_usage_5m_90d.sql (1)
1-7: LGTM!The rename operation correctly follows the drop migration, avoiding name conflicts. The down migration properly reverses the rename.
| ) | ||
| ENGINE = SummingMergeTree | ||
| PARTITION BY toDate(Timestamp) | ||
| ORDER BY (FederatedGraphID, Timestamp, OrganizationID, FieldName, NamedType, OperationHash, ClientName, ClientVersion) |
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.
ORDER BY change may alter aggregation semantics.
The new ORDER BY excludes columns present in the old table's ordering:
Path,TypeNames,SubgraphIDs(Array columns)RouterConfigVersion,IsArgument,IsInput
With SummingMergeTree, rows matching on all ORDER BY columns are merged, summing TotalUsages, TotalErrors, TotalClientErrors. Rows that previously remained distinct (differing by Path, IsArgument, etc.) will now be merged together, potentially losing granularity.
Please confirm this aggregation behavior change is intentional and won't impact downstream queries that rely on per-path or per-argument-type breakdowns.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist