refactor: deepen analytics/usage read path and quota normalization - #77
Conversation
Replace the 10-arm type switch in NormalizeQuotaRows with a QuotaRowsProvider interface implemented by each provider result, so adding a provider no longer touches the exit normalization.
- Move the usage identity display-name rule onto entities.UsageIdentity, deleting the verbatim duplicate implementations in api and repository. - Delete the pass-through analytics and usage services; the HTTP layer now defines AnalyticsProvider/UsageProvider seams over repository DTOs and repository AnalyticsReader/UsageReader bind the database handle. - Derive Output TPS inside the usage events read model and drop the duplicated service-layer read DTO family. - Extract shared analytics response mappers so each payload field is copied once.
…per source descriptor Introduce analyticsAggregateSource describing the raw usage_events and usage_rollups_hourly column mappings, so the shared summary and trend aggregate SELECT is rendered from a single definition instead of two mirrored copies. Rollup==raw equivalence tests keep pinning behavior.
…gregate source descriptor Fold the mirrored raw/rollup segment queries for provider options, model, key alias, API key and identity trend breakdowns into single definitions parameterized by analyticsAggregateSource, and drop the table-switch helpers plus rollup-only expression wrappers. The raw-only fallback path stays intact as the backfill_incomplete owner and as the independent oracle for rollup==raw equivalence tests.
…e-savings columns
… events source descriptor
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9cf0db07b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rollup.CachedTokens += cachedTokens | ||
| rollup.TotalTokens += event.TotalTokens | ||
| // TotalTokens 与其他 token 列同样钳制负值，与分析读侧的 positive CASE 表达式保持镜像等价。 | ||
| rollup.TotalTokens += positiveInt64(event.TotalTokens) |
There was a problem hiding this comment.
当数据库在升级前已经完成 backfill，历史 usage_rollups_hourly 不会因这里的写入规则变更而自动重建；这些行仍保存旧逻辑对正、负 total_tokens 的净和，而新的 raw 查询改为逐事件钳制负值。此时 analyticsRollupReadAllowed 仍会接受已完成的 backfill，导致 /analytics/core 对完整小时读取旧 rollup，而 /analytics/summary 读取 raw 数据，两者在同一窗口返回不同的 token 总量。需要使现有 backfill 失效并从 raw events 重建，或提供等价的数据迁移，而不能只修改新写入。
Useful? React with 👍 / 👎.
…de rollup divergence Codex review P2: clamping total_tokens on write (9a6e2fd) only affects rollups written after the upgrade, so already-backfilled hours keep the old net sum while the raw path clamps per event, making /analytics/core (rollup) and the raw fallback disagree on token totals for the same covered window. Revert to the original unclamped total_tokens on both the rollup write and the raw read descriptor; both paths now SUM the stored/raw value as-is, keeping raw==rollup without rebuilding history.
|
Addressed Codex review P2 ( Fix: reverted the Verification: @codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
layout.md now records that analytics/usage read paths flow from repository readers through the internal/api provider seams, and that internal/service retains write-path use cases only. ADR-0005 gains an amendment capturing the same boundary move.
… total-tokens order fragment Give the analyticsAggregateSource token columns the Expr suffix used by their sibling fields, rename analyticsAggregateTrendSelect to analyticsSummaryTrendSelect (it serves both summary and trend), and render the five duplicated total-tokens DESC order fragments from one helper. Rendered SQL is unchanged.
1 participant
Summary
Architecture deepening pass over the analytics/usage read path and quota normalization, resolving the five deepening candidates from the architecture review (deep modules: smaller seams, single definitions, no pass-through layers). Net effect: ~-700 lines with behavior pinned by tests.
internal/apiandinternal/repository; it is now a singleentities.UsageIdentity.DisplayName()with 7 behavior tests (74d3f3e).service/analytics_service.godeleted; the HTTP layer owns theAnalyticsProviderseam over repository DTOs andrepository.AnalyticsReaderbinds the database handle; shared response mappers copy each payload field once (74d3f3e).analyticsAggregateSourcedescriptor (table + column mappings) renders summary, trend, breakdown, heatmap and remaining raw queries from single definitions; mirrored SELECTs and table-switch helpers deleted (acd95f1,e57acdf,a3ef25b,9a6e2fd).servicedtoread-model family deleted;repository.UsageReaderproduces read DTOs directly and Output TPS derivation moved into the events read model with its tests (74d3f3e,9cf0db0).NormalizeQuotaRowsswitch replaced byQuotaRows()methods on each provider result behindQuotaRowsProvider, with a typed-nil guard and regression test (b609d41,1f00462).Defect-first review findings fixed in-branch: typed-nil panic guard, trend query no longer computes unused cache-savings columns, dead
GetUsageWithFilterremoved. An intermediate commit clamped rolluptotal_tokenson write (9a6e2fd), but that made pre-upgrade rollup rows diverge from new ones; it was reverted ine9810abso both read paths sumtotal_tokensthe same way (see Compatibility).Verification
go build ./...— exit 0go vet ./cmd/... ./internal/...— exit 0, no findingsgo test -count=1 ./cmd/... ./internal/...— 18 packages ok, 0 FAILinternal/repository/analytics_test.go) pin the descriptor refactor semanticsFrontend untouched (no
web/changes); backend gate is the meaningful surface.Compatibility
total_tokensbehavior is unchanged from main: neither the raw nor the rollup read path clamps negative values — both sum the column as-is, which keeps the two paths equivalent (other token columns still clamp to non-negative on both sides). An intermediate commit briefly changed this; the net diff does not.Rangelabel string downstream (the repository only ever used the resolved time window); HTTP behavior unchanged.internal/repository).Auto-close: none