feat: performance chart - #6637
Conversation
WalkthroughAdds end-to-end administrative model performance metrics. The change includes backend aggregation and health classification, an authenticated API, a dashboard table with filtering and refresh behavior, validation tests, synchronization updates, and translations. ChangesAdministrative performance metrics
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin dashboard
participant API as Admin metrics API
participant Query as QueryAdmin
participant DB as Metric database
participant Table as ModelPerformanceTable
Admin->>API: Request selected timestamp range
API->>Query: QueryAdmin(startTs, endTs)
Query->>DB: Load persisted summaries and available range
DB-->>Query: Metric buckets
Query-->>API: Aggregated model and group results
API-->>Table: Typed performance response
Table->>Table: Build rows, health states, filters, and display states
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/perf_metrics/admin.go (1)
292-300: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReduce the repeated full-map scans per model.
buildAdminModelscallscountActiveAdminGroupstwice andbuildAdminGroupsonce for every model name. Each call scans the wholecurrentorpreviousmap. The total cost is O(models × model-group pairs).Group the counters by model name once, then reuse that index in the per-model loop.
Also applies to: 302-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/perf_metrics/admin.go` around lines 292 - 300, Refactor buildAdminModels and the related countActiveAdminGroups/buildAdminGroups flow to group current and previous counters by model name once before the per-model loop. Reuse each model’s grouped counters for counting active groups and building groups, eliminating repeated full-map scans while preserving existing results.model/perf_metric_admin_test.go (1)
10-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInitialize
DBbefore using it in this package-level test.
model/perf_metric_admin_test.gousesDBon lines 13, 21, and 23 without local fixture setup. Packagemodelalready has aTestMainin another test file with AutoMigrate support; add the needed local setup there or use an explicit setup fixture so this test does not depend on another test file’s bootstrap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/perf_metric_admin_test.go` around lines 10 - 23, Initialize the database explicitly before TestGetPerfMetricGroupSummariesUsesHalfOpenRange accesses DB, using the package’s established TestMain or an equivalent local fixture with AutoMigrate support. Ensure the test remains self-contained and does not rely on bootstrap behavior from another test file.Source: Coding guidelines
controller/perf_metrics_admin_test.go (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a non-positive
start_timestamp.The handler rejects
startTs <= 0at line 88 ofcontroller/perf_metrics.go. No table entry covers that branch.♻️ Proposed additional case
{name: "reversed", query: "?start_timestamp=200&end_timestamp=100"}, + {name: "non positive start", query: "?start_timestamp=0&end_timestamp=100"}, {name: "future end", query: "?start_timestamp=100&end_timestamp=" + strconv.FormatInt(now+60, 10)},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/perf_metrics_admin_test.go` around lines 22 - 27, Add a table-driven test case in the existing perf metrics validation cases for a non-positive start_timestamp, using a query that sets start_timestamp to zero or a negative value and a valid end_timestamp. Assert the handler rejects it, covering the startTs <= 0 branch in the perf metrics handler.web/src/features/performance-metrics/lib/__tests__/admin.test.ts (1)
85-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test cases for the untested display-state branches.
getAdminPerformanceDisplayStatehas six branches:loading,error,disabled,no_complete_buckets,empty, andready. The current tests exercise onlyerroranddisabled. Add cases forno_complete_buckets,empty, andreadyto lock in the precedence order.Also add a case for
buildAdminPerformanceRows(undefined)to cover the early-return branch, and a case with more than one model to confirm multi-model mapping.As per path instructions,
web/**/__tests__/**/*.{test,spec}.{ts,tsx}requires that "测试应覆盖主要成功路径及变更涉及的关键边界和失败路径" (tests should cover the main success path plus key boundaries and failure paths introduced by the change).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/performance-metrics/lib/__tests__/admin.test.ts` around lines 85 - 118, Extend the tests in the admin model performance helpers suite to cover getAdminPerformanceDisplayState results for no_complete_buckets, empty, and ready, preserving the documented precedence among all six states. Add buildAdminPerformanceRows(undefined) coverage for its early return and a multi-model fixture asserting each model is mapped correctly, while retaining the existing error and disabled cases.Source: Path instructions
web/src/features/dashboard/components/models/model-performance-columns.tsx (1)
123-127: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize the columns array to avoid unnecessary re-creation.
useModelPerformanceColumnsbuilds a newColumnDefarray on every call.ModelPerformanceTablecalls this hook on every render, including the automatic refetch everyREFRESH_INTERVAL_MS. The new array identity also defeats theuseMemoonbuildColumnSizingBounds(columns)insideuseDataTable, since that memo is keyed on[columns].Wrap the returned array in
useMemokeyed on[t, locale](or[i18n.language]) to keep column identity stable across renders.♻️ Proposed refactor
export function useModelPerformanceColumns(): ColumnDef<AdminPerformanceTableRow>[] { const { t, i18n } = useTranslation() const locale = toIntlLocale(i18n.resolvedLanguage || i18n.language) - return [ + return useMemo<ColumnDef<AdminPerformanceTableRow>[]>(() => [ { accessorKey: 'model_name', ... }, - ] + ], [t, locale]) }Based on coding guidelines: "合理使用
useMemo、useCallback和React.memo，避免渲染路径中不必要的新对象或数组" (web/**/*.tsx).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/features/dashboard/components/models/model-performance-columns.tsx` around lines 123 - 127, Memoize the columns array returned by useModelPerformanceColumns with useMemo, using [t, locale] as dependencies so it is recreated only when translations or locale change. Keep the existing column definitions and return behavior unchanged while preserving stable identity for downstream useDataTable memoization.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/perf_metrics/admin.go`:
- Around line 153-194: Reduce hotBucketsMu lock scope across all affected sites:
in pkg/perf_metrics/admin.go lines 153-194, acquire the read lock immediately
before hotBuckets.Range and release it after traversal, leaving the three
database calls outside the lock; apply the same placement in
pkg/perf_metrics/metrics.go lines 90-92 and 140-142 for Query and
QuerySummaryAll; in pkg/perf_metrics/flush.go lines 23-26, drain buckets into a
local slice while holding the write lock, release it, then perform
UpsertPerfMetric calls.
In `@web/src/i18n/locales/zh-TW.json`:
- Line 152: Update the `Active groups` translation in the locale mapping from
`活躍用戶組` to the generic `活躍分組`, preserving the source meaning and surrounding
terminology.
---
Nitpick comments:
In `@controller/perf_metrics_admin_test.go`:
- Around line 22-27: Add a table-driven test case in the existing perf metrics
validation cases for a non-positive start_timestamp, using a query that sets
start_timestamp to zero or a negative value and a valid end_timestamp. Assert
the handler rejects it, covering the startTs <= 0 branch in the perf metrics
handler.
In `@model/perf_metric_admin_test.go`:
- Around line 10-23: Initialize the database explicitly before
TestGetPerfMetricGroupSummariesUsesHalfOpenRange accesses DB, using the
package’s established TestMain or an equivalent local fixture with AutoMigrate
support. Ensure the test remains self-contained and does not rely on bootstrap
behavior from another test file.
In `@pkg/perf_metrics/admin.go`:
- Around line 292-300: Refactor buildAdminModels and the related
countActiveAdminGroups/buildAdminGroups flow to group current and previous
counters by model name once before the per-model loop. Reuse each model’s
grouped counters for counting active groups and building groups, eliminating
repeated full-map scans while preserving existing results.
In `@web/src/features/dashboard/components/models/model-performance-columns.tsx`:
- Around line 123-127: Memoize the columns array returned by
useModelPerformanceColumns with useMemo, using [t, locale] as dependencies so it
is recreated only when translations or locale change. Keep the existing column
definitions and return behavior unchanged while preserving stable identity for
downstream useDataTable memoization.
In `@web/src/features/performance-metrics/lib/__tests__/admin.test.ts`:
- Around line 85-118: Extend the tests in the admin model performance helpers
suite to cover getAdminPerformanceDisplayState results for no_complete_buckets,
empty, and ready, preserving the documented precedence among all six states. Add
buildAdminPerformanceRows(undefined) coverage for its early return and a
multi-model fixture asserting each model is mapped correctly, while retaining
the existing error and disabled cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ad6342b6-15d2-426a-9c99-592ee07dfe0c
📒 Files selected for processing (25)
controller/perf_metrics.gocontroller/perf_metrics_admin_test.gomodel/ability.gomodel/perf_metric.gomodel/perf_metric_admin_test.gopkg/perf_metrics/admin.gopkg/perf_metrics/admin_test.gopkg/perf_metrics/flush.gopkg/perf_metrics/metrics.gorouter/api-router.goweb/src/components/data-table/hooks/use-data-table.tsweb/src/features/dashboard/components/models/model-performance-columns.tsxweb/src/features/dashboard/components/models/model-performance-table.tsxweb/src/features/dashboard/index.tsxweb/src/features/performance-metrics/api.tsweb/src/features/performance-metrics/lib/__tests__/admin.test.tsweb/src/features/performance-metrics/lib/admin.tsweb/src/features/performance-metrics/types.tsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/i18n/locales/zh.json
| func readAdminCounters(currentPeriod AdminTimeRange, previousPeriod AdminTimeRange) (map[modelGroupKey]counters, map[modelGroupKey]counters, AdminAvailableRange, error) { | ||
| hotBucketsMu.RLock() | ||
| defer hotBucketsMu.RUnlock() | ||
|
|
||
| currentRows, err := model.GetPerfMetricGroupSummaries(currentPeriod.Start, currentPeriod.End) | ||
| if err != nil { | ||
| return nil, nil, AdminAvailableRange{}, err | ||
| } | ||
| previousRows, err := model.GetPerfMetricGroupSummaries(previousPeriod.Start, previousPeriod.End) | ||
| if err != nil { | ||
| return nil, nil, AdminAvailableRange{}, err | ||
| } | ||
| oldest, newest, err := model.GetPerfMetricAvailableRange() | ||
| if err != nil { | ||
| return nil, nil, AdminAvailableRange{}, err | ||
| } | ||
|
|
||
| current := adminCountersFromRows(currentRows) | ||
| previous := adminCountersFromRows(previousRows) | ||
| hotBuckets.Range(func(key, value any) bool { | ||
| bucket := key.(bucketKey) | ||
| snapshot := value.(*atomicBucket).snapshot() | ||
| if snapshot.requestCount == 0 { | ||
| return true | ||
| } | ||
| if oldest == nil || bucket.bucketTs < *oldest { | ||
| oldest = int64Pointer(bucket.bucketTs) | ||
| } | ||
| if newest == nil || bucket.bucketTs > *newest { | ||
| newest = int64Pointer(bucket.bucketTs) | ||
| } | ||
| if bucket.bucketTs >= currentPeriod.Start && bucket.bucketTs < currentPeriod.End { | ||
| mergeAdminCounters(current, modelGroupKey{model: bucket.model, group: bucket.group}, snapshot) | ||
| } | ||
| if bucket.bucketTs >= previousPeriod.Start && bucket.bucketTs < previousPeriod.End { | ||
| mergeAdminCounters(previous, modelGroupKey{model: bucket.model, group: bucket.group}, snapshot) | ||
| } | ||
| return true | ||
| }) | ||
|
|
||
| return current, previous, AdminAvailableRange{OldestBucketTs: oldest, NewestBucketTs: newest}, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
hotBucketsMu scope covers database I/O in four places. The new mutex only needs to protect traversal and draining of the in-memory hotBuckets map. Every current holder also spans synchronous database calls. Because a pending writer blocks new readers on a sync.RWMutex, one slow query or one slow flush stalls the flush loop and all metric queries.
pkg/perf_metrics/admin.go#L153-L194: moveRLock/RUnlockinreadAdminCountersbelow the threemodel.GetPerfMetric*calls, immediately around thehotBuckets.Rangeblock.pkg/perf_metrics/metrics.go#L90-L92: move theRLock/RUnlockinQueryto just before thehotBuckets.Rangecall at line 114.pkg/perf_metrics/metrics.go#L140-L142: move theRLock/RUnlockinQuerySummaryAllto just before thehotBuckets.Rangecall at line 162.pkg/perf_metrics/flush.go#L23-L26: drain all buckets into a local slice under the write lock, release the lock, then run theUpsertPerfMetriccalls.
📍 Affects 3 files
pkg/perf_metrics/admin.go#L153-L194(this comment)pkg/perf_metrics/metrics.go#L90-L92pkg/perf_metrics/metrics.go#L140-L142pkg/perf_metrics/flush.go#L23-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/perf_metrics/admin.go` around lines 153 - 194, Reduce hotBucketsMu lock
scope across all affected sites: in pkg/perf_metrics/admin.go lines 153-194,
acquire the read lock immediately before hotBuckets.Range and release it after
traversal, leaving the three database calls outside the lock; apply the same
placement in pkg/perf_metrics/metrics.go lines 90-92 and 140-142 for Query and
QuerySummaryAll; in pkg/perf_metrics/flush.go lines 23-26, drain buckets into a
local slice while holding the write lock, release it, then perform
UpsertPerfMetric calls.
| "Active apps": "活躍套用程式", | ||
| "Active Cache Count": "活躍緩存數", | ||
| "Active Files": "活躍檔案", | ||
| "Active groups": "活躍用戶組", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep Active groups generic.
活躍用戶組 adds user, but the source refers to generic groups. The surrounding performance translations use 分組 for this concept. Use 活躍分組 to preserve the source meaning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/i18n/locales/zh-TW.json` at line 152, Update the `Active groups`
translation in the locale mapping from `活躍用戶組` to the generic `活躍分組`, preserving
the source meaning and surrounding terminology.
1 participant
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
New Features
Bug Fixes