feat(admin): add container metrics to session traces - #4935
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| }, | ||
| ]; | ||
| }); | ||
| if (windows.length === 0) return { available: false, reason: 'no_provider_identity' }; |
There was a problem hiding this comment.
WARNING: no_provider_identity is reported for causes other than missing provider identity
windows can also end up empty because the clipping above dropped every interval via if (Date.parse(end) <= Date.parse(start)) return [];. That happens whenever a matched interval lies outside [windowStartAt, windowEndAt] — which is reachable, because the session_id branch of intervalIdentityCondition (line 178) has no time bound, so an interval that starts after the session's updated_at is selected and then clipped to an empty range.
In that case the UI renders "No Cloudflare instance identity was recorded for these container intervals" (SessionContainerTelemetry.tsx:206), which is factually wrong and sends the operator looking for missing durable_object_id metadata. Consider distinguishing "no interval had a cloudflareInstanceId" from "every interval clipped to an empty window" so the diagnostic stays accurate.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| eq(container_usage_interval.cloud_billing_sku_id, cloud_billing_sku.id) | ||
| ) | ||
| .where( | ||
| and(like(container_usage_interval.service, 'cloud-agent-next%'), intervalIdentityCondition) |
There was a problem hiding this comment.
WARNING: This filter cannot use any existing index on container_usage_interval, and the whole function runs twice per metrics request
container_usage_interval only defines (status, last_seen_at), (subject_type, subject_id, started_at), and a partial unique (service, instance_id) WHERE status = 'open' (packages/db/src/schema.ts:10118-10126). This predicate filters on service LIKE 'cloud-agent-next%' plus session_id / instance_id, so none of those indexes apply and Postgres will sequentially scan the full platform-wide billing interval table on every admin session-trace view.
Two amplifiers:
getSessionContainerMetrics(line 278) callsgetSessionContainerInfoagain, sogetContainerMetricsre-executes all three queries thatgetContainerInfoalready ran for the same session.- The
cloud_agent_session_runsquery (line 155) and therunsarray it populates are never read bySessionContainerTelemetry.tsxorSessionTraceViewer.tsx, so that query and payload are currently pure overhead.
Consider an index supporting session_id (and instance_id), passing the already-fetched SessionContainerInfo into getSessionContainerMetrics, and dropping the unused runs selection.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| sandboxId?.startsWith('crv-') || | ||
| sandboxId?.startsWith('dind-') | ||
| ? 'isolated' | ||
| : 'shared', |
There was a problem hiding this comment.
SUGGESTION: Unknown sandbox identity is reported as a shared container
cloud_agent_sessions.sandbox_id is nullable (packages/db/src/schema.ts:5639) and sandboxId is also null when no cloud_agent_sessions row exists, so every optional-chained startsWith is undefined and scope falls through to 'shared'. The UI then renders the "Isolated container"/"Shared container" badge and the assertive banner "This container is shared. Workload samples may include activity from other sessions." (SessionContainerTelemetry.tsx:182-188) for sessions where the sandbox identity is simply unknown.
A third 'unknown' scope (or gating the banner on sandboxId !== null) would avoid asserting sharing that was never observed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| for (const [index, plan] of batch.entries()) { | ||
| const alias = `m${index}`; | ||
| const aliasErrors = errors.filter( | ||
| error => error.path?.includes(alias) || error.path === undefined |
There was a problem hiding this comment.
WARNING: Unscoped GraphQL errors are silently dropped for single-window batches
Line 451 only throws on unscopedErrors when batch.length > 1. For a single-plan batch the intent is clearly to downgrade those errors to partial, but this filter only re-captures errors with path === undefined. An unscoped error whose path is defined yet contains no m<n> segment (for example ["viewer", "accounts"]) matches neither branch: it is not thrown, not added to issues, and does not set partial.
If account and groups are still present, the result is returned as { partial: false, issues: [] } even though Cloudflare reported an error, so the admin UI presents incomplete data as complete. Attributing unscopedErrors to the single plan (rather than only path-less errors) would close the gap.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| <div className="grid gap-6 xl:grid-cols-2"> | ||
| <figure | ||
| className="space-y-2" | ||
| role="img" |
There was a problem hiding this comment.
WARNING: role="img" hides the chart's heading and figure semantics from assistive technology
role="img" makes the element a leaf node in the accessibility tree, so every descendant is pruned — including the <h3>Memory</h3> on line 257 (and <h3>CPU Utilization</h3> on line 344). Screen-reader users lose both section headings, the document heading outline breaks, and the figure role is overridden so figure/figcaption semantics no longer apply.
Since the same data is already exposed in the "Metric samples" table, the accessible label belongs on the inner chart container (<div className="h-64 w-full">) rather than on the figure wrapping the heading.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| label: | ||
| placementId && placementId.length > 12 | ||
| ? `${placementId.slice(0, 8)}...` | ||
| : (placementId ?? 'unknown'), |
There was a problem hiding this comment.
SUGGESTION: Series labels drop the windowKey, so per-interval separation is invisible in the legend
The refactor deliberately keys series by (windowKey, placementId), but label only encodes placementId. When the same placement appears across two container intervals, two distinct Line series render with identical legend/tooltip names ("placem... max" twice), which defeats the separation this change introduced. Including a short window discriminator in label would keep them distinguishable.
Also, placementId now comes from Set<string> (line 109), so it is non-nullable — both the placementId && guard and the ?? 'unknown' fallback are unreachable leftovers from the previous split('\0') implementation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Executive SummaryAll six previously reported findings are fixed at HEAD; the remaining risk is that folding container info and Cloudflare metrics into one tRPC query makes an analytics outage or missing analytics config hide the Postgres-derived container interval, SKU, and instance data entirely. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed in this incremental pass (9 files)
Previously reported findings verified as fixed
Assumptions
Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 35092d8)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 35092d8)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryIncremental review of the Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed in this incremental pass (2 files)
Verified as correct in the new commit
Assumptions
Fix these issues in Kilo Cloud Previous review (commit f8ad783)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryHighest risk is the unindexed, doubly-executed Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (16 files)
Verified as correct (no action needed)
Assumptions
Reviewed by claude-opus-5 · Input: 66 · Output: 21.2K · Cached: 2.5M Review guidance: REVIEW.md from base branch |
| try { | ||
| const info = await getSessionContainerInfo(input.session_id); | ||
| const metrics = info | ||
| ? await getSessionContainerMetricsForInfo(info) |
There was a problem hiding this comment.
WARNING: A Cloudflare analytics failure now hides the DB-derived container info too
Merging info and metrics into one procedure means any ContainerMetricsAnalyticsError thrown by getSessionContainerMetricsForInfo rejects the whole query, so containerTelemetryQuery.data stays undefined. SessionTraceViewer then renders neither the interval/SKU/capacity table (SessionContainerTelemetryContent, gated on data?.info) nor the Container:/SKU: rows in the summary card (latestContainerInterval, SessionTraceViewer.tsx:239) — only a destructive alert.
This is not an exotic path: missing_config is thrown whenever CLOUDFLARE_ACCOUNT_ID/CLOUDFLARE_ANALYTICS_API_TOKEN are absent (container-metrics-analytics.ts:330), and BAD_GATEWAY covers every transient Cloudflare GraphQL failure. Before this change the separate getContainerInfo query kept the interval table visible and confined the metrics error to an inline alert inside the card.
Consider catching ContainerMetricsAnalyticsError here and returning { info, metrics: { available: false, ... } } (or an error field the card can render) so the Postgres-derived interval data survives analytics outages.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return { available: true, ...metrics }; | ||
| } | ||
|
|
||
| export async function getSessionContainerMetrics( |
There was a problem hiding this comment.
SUGGESTION: getSessionContainerMetrics is now only reachable from tests
After admin-router.ts switched to getSessionContainerInfo + getSessionContainerMetricsForInfo, this wrapper has no production caller; its only remaining use is admin-session-traces.test.ts:264. The queryMetrics parameter on the new ForInfo function is likewise only injected from tests. The same coverage can be obtained by composing the two exported functions in the test, which also keeps the tested path identical to the router path.
Related: admin.sessionTraces.getContainerInfo no longer has a UI consumer now that useAdminSessionContainerInfo is gone — worth deciding whether it stays as a debugging endpoint or is removed with the wrapper.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
2 participants
Summary
containersMetricsAdaptiveGroupsclient with account capability checks, retention/window limits, partial-result handling, and provider-safe errors.Verification
/admin/session-traces?sessionId=ses_048a82950fffQ60eeZmoxxETi6locally with a representative Cloud Agent usage interval and confirmed Session Details displayed the container instance and SKU.Visual Changes
Reviewer Notes