feat: add /api/overview endpoint for public usage statistics#3
feat: add /api/overview endpoint for public usage statistics#3tym83 wants to merge 6 commits intocozystack:mainfrom
Conversation
Add monthly snapshot collection from VictoriaMetrics and a public GET /api/overview JSON endpoint that aggregates telemetry into three time periods (last month, last quarter, last 12 months). - Query VM for cluster count, node count, tenant count, and per-app usage - Fetch app list dynamically from GitHub cozystack/cozystack packages/apps - Store monthly snapshots to disk (PVC-backed) on the 1st of each month - Collect initial snapshot on first startup - Add Helm chart PVC, vmselect URL, and snapshot directory config Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as "HTTP Server"
participant OM as "OverviewManager"
participant Cache as "InMemoryCache"
participant FS as "FileSystem"
participant VM as "VictoriaMetrics (vmselect)"
participant GH as "GitHub (cozystack)"
Client->>API: GET /api/overview?year=YYYY&month=MM
API->>OM: HandleOverview(request)
OM->>Cache: check snapshot for month
alt snapshot in memory
Cache-->>OM: snapshot
else
OM->>FS: check <snapshotDir>/<month>.json
alt file exists
FS-->>OM: read snapshot
else
OM->>VM: promql queries (clusters, nodes, tenants, apps by kind)
OM->>GH: fetch packages/apps directory (app kinds)
VM-->>OM: metric vectors/scalars
GH-->>OM: app kind list (or fallback)
OM->>FS: write snapshot atomically (tmp -> rename)
OM->>Cache: store snapshot
end
end
OM->>OM: aggregate snapshots (month/quarter/year)
OM-->>API: JSON OverviewResponse
API-->>Client: 200 OK + body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Public Statistics API to the telemetry server, enabling the collection and serving of aggregated usage statistics. Key additions include an OverviewManager for handling monthly snapshots, a persistent storage mechanism via PVC, and the /api/overview endpoint. Feedback focuses on correcting Go time formatting strings to ensure accurate date reporting, addressing potential timezone loading issues in minimal runtime environments, and optimizing HTTP client usage for better performance.
overview.go
Outdated
|
|
||
| if len(snapshots) == 1 { | ||
| stats.Label = latestDate.Format("January 2006") | ||
| stats.Start = latestDate.Format("2006-01-01") |
There was a problem hiding this comment.
The time format string 2006-01-01 is incorrect. In Go's time formatting, 01 is the placeholder for the month and 02 is for the day. Using 2006-01-01 will result in the month being repeated (e.g., 2024-03-03 for March). Since latestDate is already the first of the month, you should use 2006-01-02 to correctly output YYYY-MM-01.
| stats.Start = latestDate.Format("2006-01-01") | |
| stats.Start = latestDate.Format("2006-01-02") |
There was a problem hiding this comment.
Fixed in commit 328eed6 — both format strings corrected to "2006-01-02".
overview.go
Outdated
| stats.Label = fmt.Sprintf("%s \u2014 %s", | ||
| oldestDate.Format("January 2006"), | ||
| latestDate.Format("January 2006")) | ||
| stats.Start = oldestDate.Format("2006-01-01") |
There was a problem hiding this comment.
The time format string 2006-01-01 is incorrect. It should be 2006-01-02 to correctly represent the first day of the month.
| stats.Start = oldestDate.Format("2006-01-01") | |
| stats.Start = oldestDate.Format("2006-01-02") |
overview.go
Outdated
| loc, err := time.LoadLocation("America/Los_Angeles") | ||
| if err != nil { | ||
| log.Printf("Warning: cannot load Pacific timezone, using UTC: %v", err) | ||
| return time.UTC | ||
| } | ||
| return loc |
There was a problem hiding this comment.
time.LoadLocation depends on the timezone database being present in the runtime environment. In minimal Docker images (like scratch or alpine without tzdata), this will fail and fallback to UTC. This would result in snapshots being collected at the wrong time relative to the Pacific Time mentioned in the documentation. Consider importing time/tzdata to embed the database in the binary.
There was a problem hiding this comment.
Good catch — fixed in 8bad2f4. Added import _ "time/tzdata" to embed the timezone database in the binary.
| type OverviewManager struct { | ||
| vmSelectURL string | ||
| snapshotDir string | ||
| mu sync.RWMutex | ||
| snapshots []Snapshot | ||
| } |
There was a problem hiding this comment.
Creating a new http.Client for every request (as seen in queryVector and fetchAppList) is inefficient because it prevents connection reuse. It is recommended to share a single http.Client instance by adding it as a field to the OverviewManager struct and initializing it in the constructor.
| type OverviewManager struct { | |
| vmSelectURL string | |
| snapshotDir string | |
| mu sync.RWMutex | |
| snapshots []Snapshot | |
| } | |
| type OverviewManager struct { | |
| vmSelectURL string | |
| snapshotDir string | |
| httpClient *http.Client | |
| mu sync.RWMutex | |
| snapshots []Snapshot | |
| } |
There was a problem hiding this comment.
Fixed in 8bad2f4 — httpClient is now a field on OverviewManager, initialized once in the constructor.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
overview.go (3)
440-445: Unchecked error from JSON encoding.The error from
json.NewEncoder(w).Encode(overview)is ignored. While encoding a simple struct is unlikely to fail, checking the error is good practice.♻️ Proposed fix
w.Header().Set("Content-Type", "application/json") w.Header().Set("Access-Control-Allow-Origin", "*") - json.NewEncoder(w).Encode(overview) + if err := json.NewEncoder(w).Encode(overview); err != nil { + log.Printf("Error encoding overview response: %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 440 - 445, The JSON encoder error from json.NewEncoder(w).Encode(overview) is ignored; capture its returned error and handle it (e.g., log the error and return an HTTP 500). Locate the handler that calls m.buildOverview(snapshots) to produce overview, replace the bare Encode call with code that checks err := json.NewEncoder(w).Encode(overview) and on non-nil err uses the existing logger (or http.Error) to report the failure and set an appropriate status code before returning.
295-339: GitHub API rate limit consideration.The anonymous GitHub API is limited to 60 requests/hour. While the fallback to
defaultAppList()handles rate limiting gracefully, consider adding aUser-Agentheader (GitHub recommends this) and potentially caching the app list for longer periods.♻️ Add User-Agent header (GitHub best practice)
req, err := http.NewRequest("GET", apiURL, nil) if err != nil { log.Printf("Error creating GitHub request: %v", err) return defaultAppList() } req.Header.Set("Accept", "application/vnd.github.v3+json") + req.Header.Set("User-Agent", "cozystack-telemetry-server")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 295 - 339, The fetchAppList function uses anonymous GitHub API calls and lacks a User-Agent header and caching; update fetchAppList to set a User-Agent header on req (e.g., req.Header.Set("User-Agent", "<app-name-or-service>")) and implement a simple in-memory cache inside OverviewManager (fields like cachedApps []string and cacheExpiry time.Time) so subsequent calls return cachedApps until cacheExpiry, falling back to defaultAppList() on error; keep existing error handling and sort/logging logic and ensure cache is refreshed on successful fetch.
559-564: Remove unnecessary customminfunction—Go 1.23+ provides a built-in.The project targets Go 1.23.4, which includes the built-in
minfunction introduced in Go 1.21. The custom definition at lines 559–564 shadows the built-in unnecessarily. Since both have the same signature for integers, replace calls to the customminwith the built-in function for clarity and reduced confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 559 - 564, A custom min(a, b int) function is shadowing the new built-in min; delete the custom func min definition in overview.go and update the code to call the language-provided min(a, b) directly everywhere (ensure no other local identifiers named min conflict). Remove the function declaration for min and run `go vet`/build to confirm there are no remaining references or shadowing collisions.main.go (1)
156-158: Consider adding graceful shutdown for the OverviewManager.The
OverviewManager.Start()spawns background goroutines but there's no mechanism to stop them gracefully. When the server terminates, any in-progress snapshot collection could be interrupted. Whileos.WriteFileis atomic, adding aStop()method with context cancellation would be cleaner.♻️ Sketch for graceful shutdown
In
overview.go, add a context and stop channel:type OverviewManager struct { // ... existing fields ctx context.Context cancel context.CancelFunc } func (m *OverviewManager) Stop() { m.cancel() }In
main.go, handle signals:overview := NewOverviewManager(*vmSelectURL, *snapshotDir) overview.Start() defer overview.Stop() // Handle graceful shutdown sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) go func() { <-sigChan overview.Stop() // shutdown server... }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.go` around lines 156 - 158, The OverviewManager starts background goroutines via OverviewManager.Start() but lacks a cancellation mechanism; add context cancellation to OverviewManager (fields ctx context.Context and cancel context.CancelFunc set in NewOverviewManager), implement a Stop() method that calls cancel, have goroutines select on ctx.Done() to exit cleanly, and in main.go call overview.Stop() (and defer it) and wire a signal handler (SIGINT/SIGTERM) to call overview.Stop() during shutdown so in-progress snapshot work is canceled gracefully.charts/cozy-telemetry/templates/pvc.yaml (1)
1-12: Consider making storageClassName configurable.The PVC uses the cluster's default storage class. For production flexibility, consider allowing users to specify a storage class via values.
♻️ Optional: Add configurable storageClassName
spec: accessModes: - ReadWriteOnce + {{- if .Values.snapshot.storageClassName }} + storageClassName: {{ .Values.snapshot.storageClassName }} + {{- end }} resources: requests: storage: {{ .Values.snapshot.storage }}And in
values.yaml:snapshot: storage: 1Gi storageClassName: "" # Uses cluster default if empty🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/cozy-telemetry/templates/pvc.yaml` around lines 1 - 12, The PersistentVolumeClaim "cozy-telemetry-snapshots" currently omits storageClassName and thus always uses the cluster default; add a spec.storageClassName field that sources its value from .Values.snapshot.storageClassName (leave empty/null to use the cluster default) alongside the existing .Values.snapshot.storage request, and update values.yaml to include snapshot.storageClassName so operators can configure the PVC storage class; ensure the new key name matches .Values.snapshot.storageClassName used in the chart templates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/cozy-telemetry/templates/deployment.yaml`:
- Around line 32-34: The deployment template hardcodes the mountPath
"/data/snapshots" under volumeMounts (name: snapshots) instead of using the
chart value; update the deployment.yaml volumeMounts entry for mountPath to
reference the Helm value config.snapshotDir (from values.yaml) so the pod mount
follows the configured snapshotDir (ensure the same Helm template variable is
used wherever snapshotDir is referenced to keep volume and application paths
consistent).
- Around line 8-10: The deployment hardcodes replicas: 1 while values.yaml
defines replicaCount, making the Helm value unused; either remove replicaCount
from charts/cozy-telemetry/values.yaml or update
charts/cozy-telemetry/templates/deployment.yaml to use the templated value
(e.g., replace the hardcoded replicas with .Values.replicaCount) and add a brief
comment above explaining the RWO storage constraint that requires single-replica
deployments when applicable; update whichever file contains
replicaCount/replicas to keep them consistent and document the reason.
In `@overview.go`:
- Around line 545-552: The parseMonth function currently swallows parse errors
and returns time.Now(), which can produce misleading labels; change
parseMonth(month string) to return (time.Time, error) instead of silently
falling back, call time.Parse and return the parsed time or the parse error, and
update every caller of parseMonth to handle the error (log a warning or
propagate it to the API response) rather than using time.Now(); this ensures
invalid input is surfaced and avoids incorrect date ranges in responses.
- Around line 158-227: collectSnapshot currently saves a snapshot even when all
metric queries fail (all-zero), which can overwrite valid prior data; modify
collectSnapshot to detect when no meaningful data was collected (e.g., track a
boolean like anySuccess set to true whenever
queryScalar/queryVector/fetchAppList returns usable results or snapshot fields
are non-zero/non-empty) and only call m.saveSnapshot(snapshot) if anySuccess (or
snapshot contains at least one non-zero field or apps). If no meaningful data
was collected, log a warning and skip saving to avoid overwriting previous
snapshots; use the existing functions queryScalar, queryVector, fetchAppList and
the Snapshot struct to determine success.
---
Nitpick comments:
In `@charts/cozy-telemetry/templates/pvc.yaml`:
- Around line 1-12: The PersistentVolumeClaim "cozy-telemetry-snapshots"
currently omits storageClassName and thus always uses the cluster default; add a
spec.storageClassName field that sources its value from
.Values.snapshot.storageClassName (leave empty/null to use the cluster default)
alongside the existing .Values.snapshot.storage request, and update values.yaml
to include snapshot.storageClassName so operators can configure the PVC storage
class; ensure the new key name matches .Values.snapshot.storageClassName used in
the chart templates.
In `@main.go`:
- Around line 156-158: The OverviewManager starts background goroutines via
OverviewManager.Start() but lacks a cancellation mechanism; add context
cancellation to OverviewManager (fields ctx context.Context and cancel
context.CancelFunc set in NewOverviewManager), implement a Stop() method that
calls cancel, have goroutines select on ctx.Done() to exit cleanly, and in
main.go call overview.Stop() (and defer it) and wire a signal handler
(SIGINT/SIGTERM) to call overview.Stop() during shutdown so in-progress snapshot
work is canceled gracefully.
In `@overview.go`:
- Around line 440-445: The JSON encoder error from
json.NewEncoder(w).Encode(overview) is ignored; capture its returned error and
handle it (e.g., log the error and return an HTTP 500). Locate the handler that
calls m.buildOverview(snapshots) to produce overview, replace the bare Encode
call with code that checks err := json.NewEncoder(w).Encode(overview) and on
non-nil err uses the existing logger (or http.Error) to report the failure and
set an appropriate status code before returning.
- Around line 295-339: The fetchAppList function uses anonymous GitHub API calls
and lacks a User-Agent header and caching; update fetchAppList to set a
User-Agent header on req (e.g., req.Header.Set("User-Agent",
"<app-name-or-service>")) and implement a simple in-memory cache inside
OverviewManager (fields like cachedApps []string and cacheExpiry time.Time) so
subsequent calls return cachedApps until cacheExpiry, falling back to
defaultAppList() on error; keep existing error handling and sort/logging logic
and ensure cache is refreshed on successful fetch.
- Around line 559-564: A custom min(a, b int) function is shadowing the new
built-in min; delete the custom func min definition in overview.go and update
the code to call the language-provided min(a, b) directly everywhere (ensure no
other local identifiers named min conflict). Remove the function declaration for
min and run `go vet`/build to confirm there are no remaining references or
shadowing collisions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ec9abc8-3fab-4378-9765-8710241897ee
📒 Files selected for processing (6)
README.mdcharts/cozy-telemetry/templates/deployment.yamlcharts/cozy-telemetry/templates/pvc.yamlcharts/cozy-telemetry/values.yamlmain.gooverview.go
Go's time format uses 01 for month, 02 for day. The Start field format was "2006-01-01" (year-month-month) instead of "2006-01-02" (year-month-day). Also fix parseMonth to parse with explicit day suffix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
overview.go (2)
83-100: Consider adding graceful shutdown support for background goroutines.The
Start()method spawns two long-running goroutines with no mechanism to stop them. When the process receives SIGTERM, these goroutines are abandoned without cleanup. While this may be acceptable for a simple scheduler, it's better practice to support graceful shutdown.♻️ Proposed refactor: Add context-based shutdown
type OverviewManager struct { vmSelectURL string snapshotDir string mu sync.RWMutex snapshots []Snapshot + stopCh chan struct{} } func NewOverviewManager(vmSelectURL, snapshotDir string) *OverviewManager { return &OverviewManager{ vmSelectURL: vmSelectURL, snapshotDir: snapshotDir, + stopCh: make(chan struct{}), } } -func (m *OverviewManager) Start() { +func (m *OverviewManager) Start() { // ... existing setup ... go func() { - time.Sleep(30 * time.Second) + select { + case <-time.After(30 * time.Second): + case <-m.stopCh: + return + } m.collectIfNeeded() }() go m.scheduleMonthly() } + +func (m *OverviewManager) Stop() { + close(m.stopCh) +}Then wire up signal handling in
main.goto calloverview.Stop()on SIGTERM/SIGINT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 83 - 100, Start() currently launches background goroutines (scheduleMonthly and the collectIfNeeded launcher) with no way to stop them; add context-based shutdown by giving OverviewManager a context.CancelFunc (or context.Context + cancel) and a sync.WaitGroup, update Start to create a cancellable context and increment the WaitGroup for each goroutine, pass the context into scheduleMonthly and collectIfNeeded (or make them check ctx.Done()) so they exit when canceled, implement a Stop() method that calls cancel and waits on the WaitGroup to ensure graceful shutdown, and wire main.go to call overview.Stop() on SIGINT/SIGTERM.
458-463: Consider using built-inminfunction instead of custom implementation.The project uses Go 1.23.4, which supports the built-in
minfunction introduced in Go 1.21. The customminfunction defined at lines 559-562 can be removed, and calls tomin()at lines 461 and 464 (and corresponding lines in the similar logic) can use Go's built-in instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 458 - 463, The code defines a custom min function but the project uses Go 1.23 which already provides a built-in min; remove the custom min implementation (the function around lines 559-562) and change calls like quarterCount := min(3, len(snapshots)) and yearCount := min(12, len(snapshots)) (used when preparing resp.Periods and in similar logic that calls aggregateSnapshots) to use the language-provided min directly (no extra import needed); run go build/tests to verify no references to the removed custom min remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@overview.go`:
- Around line 442-445: The JSON encoding call
json.NewEncoder(w).Encode(overview) currently ignores its returned error after
headers are sent; update the code to capture that error, e.g. err :=
json.NewEncoder(w).Encode(overview), and if err != nil log the failure
(including err and context like the overview payload or request info) using the
project's logger (or log.Printf) so encoding/writing failures are recorded for
debugging while leaving headers/status unchanged.
---
Nitpick comments:
In `@overview.go`:
- Around line 83-100: Start() currently launches background goroutines
(scheduleMonthly and the collectIfNeeded launcher) with no way to stop them; add
context-based shutdown by giving OverviewManager a context.CancelFunc (or
context.Context + cancel) and a sync.WaitGroup, update Start to create a
cancellable context and increment the WaitGroup for each goroutine, pass the
context into scheduleMonthly and collectIfNeeded (or make them check ctx.Done())
so they exit when canceled, implement a Stop() method that calls cancel and
waits on the WaitGroup to ensure graceful shutdown, and wire main.go to call
overview.Stop() on SIGINT/SIGTERM.
- Around line 458-463: The code defines a custom min function but the project
uses Go 1.23 which already provides a built-in min; remove the custom min
implementation (the function around lines 559-562) and change calls like
quarterCount := min(3, len(snapshots)) and yearCount := min(12, len(snapshots))
(used when preparing resp.Periods and in similar logic that calls
aggregateSnapshots) to use the language-provided min directly (no extra import
needed); run go build/tests to verify no references to the removed custom min
remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Import time/tzdata to embed timezone database for scratch/alpine images - Share single http.Client across OverviewManager instead of per-request - Skip saving snapshot when all values are zero (VM unreachable) - Log parseMonth errors and json.Encode failures - Remove unused replicaCount from values.yaml - Template volumeMount path from config.snapshotDir Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@overview.go`:
- Around line 127-149: nextFirstOfMonth currently always jumps to the following
month which causes a startup race where a process that starts on the 1st just
before 00:01 PT will miss the prior month's snapshot; update
nextFirstOfMonth(now) to compute the candidate for the current month's 1st at
00:01 (time.Date(year, month, 1, 0, 1, 0, 0, now.Location())) and return that
candidate if now is before it, otherwise return the candidate for the next
month, and also modify scheduleMonthly() so that when the process starts on day
1 and now is before the candidate (i.e., before 00:01) it immediately calls
m.collectSnapshot(prevMonth) for the just-ended month and then advances next to
the following month's 00:01 to avoid double-collecting.
- Around line 359-368: In saveSnapshot, avoid truncating the target month file
in place; instead write atomically by creating a temp file in m.snapshotDir
(e.g. using os.CreateTemp), write the JSON data to it, fsync/close the temp
file, then rename it to filepath.Join(m.snapshotDir, s.Month+".json") with
os.Rename so the replacement is atomic; also fsync the directory if possible.
Reference: saveSnapshot, m.snapshotDir, s.Month, and ensure loadSnapshots
continues to read the final file.
- Around line 463-472: The current logic uses simple slice lengths to build
periods (resp.Periods["month"/"quarter"/"year"]) which selects the newest N
snapshots rather than the last N calendar months; implement a calendar-aware
filter (e.g. filterSnapshotsByMonths(snapshots []Snapshot, months int) that
parses Snapshot.Month into time.Time, computes a cutoff from the latest
snapshot, and returns only snapshots whose month >= cutoff) and replace the
slice-based calls (aggregateSnapshots(snapshots[:1], ...),
aggregateSnapshots(snapshots[:quarterCount], ...),
aggregateSnapshots(snapshots[:yearCount], ...)) with
aggregateSnapshots(filterSnapshotsByMonths(snapshots, 1/3/12), ...) so
quarter/year cover the actual last 3/12 calendar months even when months are
missing in the data.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f3d5a85f-dc8b-4998-ba52-738113fbeccc
📒 Files selected for processing (3)
charts/cozy-telemetry/templates/deployment.yamlcharts/cozy-telemetry/values.yamloverview.go
- nextFirstOfMonth: if pod starts on 1st before 00:01, return current month's run time instead of skipping to next month - saveSnapshot: atomic write via temp file + fsync + rename to prevent partial files on crash/eviction - buildOverview: use calendar-range filter for quarter/year instead of slice length, so gaps don't produce wrong date ranges Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| app: cozy-telemetry | ||
| spec: | ||
| replicas: {{ .Values.replicaCount }} | ||
| replicas: 1 |
There was a problem hiding this comment.
This and PVC will make telepetry-server unable to run in multiple replicas, if you want to cache the state, it might be worth of using emptyDir: {} storage.
- Remove PVC and Recreate strategy; use emptyDir so the server can run with multiple replicas - Restore replicaCount to values.yaml - /api/overview now requires ?year=YYYY&month=MM params; requests without them are rejected with 400 to prevent public endpoint abuse - Snapshots are generated on the fly from VictoriaMetrics at the end of the requested month and cached in emptyDir for subsequent requests - Remove background scheduler and startup auto-collection; all data fetching is driven by incoming requests - Add &time= param to VM queries to support historical month lookups - Increase server WriteTimeout to 60s to accommodate on-demand VM queries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> -e Signed-off-by: tym83 <6355522@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
overview.go (1)
279-322: Consider adding a User-Agent header for GitHub API requests.GitHub's API documentation recommends setting a
User-Agentheader. While the fallback todefaultAppList()handles rate-limit or error scenarios gracefully, adding a descriptive User-Agent can help avoid issues with unauthenticated requests.🔧 Proposed fix
req.Header.Set("Accept", "application/vnd.github.v3+json") + req.Header.Set("User-Agent", "cozystack-telemetry-server")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@overview.go` around lines 279 - 322, The fetchAppList method in OverviewManager should set a User-Agent header on the GitHub API request to comply with GitHub recommendations and reduce chances of being rejected; after creating the request in fetchAppList (the req from http.NewRequest) add a descriptive User-Agent (e.g., include your service name and version like "cozystack-overview/<version>" or similar) in addition to the existing Accept header before calling m.httpClient.Do, ensuring the header is consistently applied for all GitHub calls from this function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@overview.go`:
- Around line 279-322: The fetchAppList method in OverviewManager should set a
User-Agent header on the GitHub API request to comply with GitHub
recommendations and reduce chances of being rejected; after creating the request
in fetchAppList (the req from http.NewRequest) add a descriptive User-Agent
(e.g., include your service name and version like "cozystack-overview/<version>"
or similar) in addition to the existing Accept header before calling
m.httpClient.Do, ensuring the header is consistently applied for all GitHub
calls from this function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a36397b-3fd3-4996-b4de-c33c683ee2d9
📒 Files selected for processing (4)
charts/cozy-telemetry/templates/deployment.yamlcharts/cozy-telemetry/values.yamlmain.gooverview.go
🚧 Files skipped from review as they are similar to previous changes (2)
- main.go
- charts/cozy-telemetry/values.yaml
- Add per-month singleflight via WaitGroup map to prevent thundering herd: concurrent requests for the same uncached month now coalesce into a single VM query instead of N parallel ones - Cap VictoriaMetrics response body at 10 MB with io.LimitReader to prevent OOM on malformed/hostile responses - Revert global WriteTimeout to 10s; wrap only the overview handler with http.TimeoutHandler(55s) so slow VM queries don't weaken the slow-client protection on the telemetry ingestion endpoint Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> -e Signed-off-by: tym83 <6355522@gmail.com>
2 participants
Summary
GET /api/overviewendpoint that returns aggregated telemetry as JSON (clusters, nodes, tenants, per-app usage)Changes
overview.go— new file with snapshot collection, VM querying, GitHub app list fetching, aggregation, and HTTP handlermain.go— wire in new--vmselect-urland--snapshot-dirflags, register/api/overviewroutecharts/cozy-telemetry/templates/deployment.yaml— add volume mount, new CLI args, single replica with Recreate strategy (PVC requires RWO)charts/cozy-telemetry/templates/pvc.yaml— new PVC for snapshot storagecharts/cozy-telemetry/values.yaml— addvmSelectURL,snapshotDir, andsnapshot.storageconfigREADME.md— document the new endpoint, response format, and all configuration flagsTest plan
/api/overviewreturns valid JSON (or 503 if no snapshots yet)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores