-
-
Notifications
You must be signed in to change notification settings - Fork 70
feat(usage): add cache analytics support #196
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
Changes from all commits
3bbf9af
1291110
27371ea
ced91e2
a99a8d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| # Possible Refactoring | ||
|
|
||
| Ordered by lowest effort and lowest risk first. | ||
|
|
||
| ## 1. Remove dead `CacheTypeBoth` | ||
|
|
||
| Effort: very low | ||
| Risk: very low | ||
|
|
||
| Why: | ||
| - Defined in `internal/responsecache/semantic.go`. | ||
| - No call sites found in the repo. | ||
|
|
||
| How verified: | ||
| - Symbol searched: `CacheTypeBoth` | ||
| - Command: `rg -n "CacheTypeBoth"` | ||
|
|
||
| Suggested action: | ||
| - Delete the constant and let tests confirm nothing depended on it. | ||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| ## 2. Deduplicate the dashboard's empty `cacheOverview` object | ||
|
|
||
| Effort: low | ||
| Risk: very low | ||
|
|
||
| Why: | ||
| - The same shape is repeated in: | ||
| - `internal/admin/dashboard/static/js/dashboard.js` | ||
| - `internal/admin/dashboard/static/js/modules/usage.js` | ||
| - `internal/admin/dashboard/static/js/modules/execution-plans.js` | ||
|
|
||
| Suggested action: | ||
| - Keep a single `emptyCacheOverview()` factory and reuse it everywhere. | ||
|
|
||
| ## 3. Pick one owner for "cache overview is cached-only" | ||
|
|
||
| Effort: low | ||
| Risk: low | ||
|
|
||
| Why: | ||
| - The handler sets `CacheModeCached` in `internal/admin/handler.go`. | ||
| - Each reader sets it again in: | ||
| - `internal/usage/reader_sqlite.go` | ||
| - `internal/usage/reader_postgresql.go` | ||
| - `internal/usage/reader_mongodb.go` | ||
| - `GetCacheOverview()` already implies cached-only behavior. | ||
|
|
||
| Suggested action: | ||
| - Keep the override in one place only. | ||
| - Prefer reader ownership so the behavior stays correct regardless of caller. | ||
|
|
||
| ## 4. Remove the legacy `ResponseCacheMiddleware.Middleware()` path | ||
|
|
||
| Effort: medium | ||
| Risk: medium | ||
|
|
||
| Why: | ||
| - Production flow now uses `HandleRequest()` from `internal/server/translated_inference_service.go`. | ||
| - `.Middleware()` in `internal/responsecache/responsecache.go` is only referenced by tests. | ||
|
|
||
| How verified: | ||
| - Symbols searched: `Middleware()` and `HandleRequest(` | ||
| - Commands: | ||
| - `rg -n "\\.Middleware\\(\\)" | sort` | ||
| - `rg -n "HandleRequest\\(" | sort` | ||
|
|
||
| Suggested action: | ||
| - Before deleting the compatibility wrapper, keep equivalent cache-hit and cache-miss coverage around `HandleRequest()`. | ||
| - Existing tests in `internal/responsecache/handle_request_test.go` already cover core hit/miss flows and should be expanded first if wrapper-specific assertions are still needed. | ||
| - Delete the compatibility wrapper. | ||
| - Only remove `internal/responsecache/middleware_test.go` after `HandleRequest()`-level coverage fully preserves the hit/miss, response header/status, and cache population assertions currently carried by the middleware wrapper tests. | ||
|
|
||
|
Comment on lines
+67
to
+72
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Preserve behavioral coverage when removing middleware-focused tests. Please explicitly require replacement tests around 🤖 Prompt for AI Agents |
||
| ## 5. Centralize cache-type vocabulary across packages | ||
|
|
||
| Effort: medium to high | ||
| Risk: medium | ||
|
|
||
| Why: | ||
| - Overlapping cache constants and normalization logic exist in: | ||
| - `internal/usage/cache_type.go` | ||
| - `internal/auditlog/auditlog.go` | ||
| - `internal/responsecache/semantic.go` | ||
| - This increases the chance of drift when new cache types or modes are added. | ||
|
|
||
| Suggested action: | ||
| - Introduce a small shared internal package for cache semantics. | ||
| - Do it only if it can be done without creating import cycles. | ||
|
|
||
| ## Recommended order | ||
|
|
||
| 1. Remove `CacheTypeBoth`. | ||
| 2. Deduplicate the dashboard empty-state object. | ||
| 3. Keep cached-only policy in one layer. | ||
| 4. Remove the legacy middleware path. | ||
| 5. Centralize cache semantics in a shared package. | ||
Uh oh!
There was an error while loading. Please reload this page.