Both of the concerns I left open from my previous pass have landed cleanly:
targets.go line 96 — the event-loop log is now d.log.Warn(...), matching the resync path at line 123 and the coding guideline for transient handled failures.integration_test.go — the broad post-Start skip (matching "permission denied", "connection refused", "no such file", "dial unix") is gone entirely. The call is now require.NoError(t, err), so a real regression in the scrape or remote-write path surfaces as a failure rather than a silent skip. The two remaining skips are both properly scoped to containerd availability, which is exactly the right boundary.All earlier concerns (Warn log in disabled.go, coreutil.ResolveRuntimeConfig at both decision points, Docusaurus admonitions in server-config.md, config_cluster_name qualified as a telemetry fallback) remain addressed. No new issues introduced. This is ready to merge.
Posted to the PR's comment threads when you submit.
Start skip is gone — require.NoError(t, err) at line 116 means any real startup failure is now a hard test failure. The only skips left are the two containerd availability checks at the top of the test, which are exactly the right scope. Resolving this.d.log.Warn(...), consistent with the resync path at line 123. Exactly what I asked for. Resolving.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1087,
"verdict": "ready",
"event": "approve",
"summary": "Both of the concerns I left open from my previous pass have landed cleanly:\n\n- **`targets.go` line 96** — the event-loop log is now `d.log.Warn(...)`, matching the resync path at line 123 and the coding guideline for transient handled failures.\n- **`integration_test.go`** — the broad post-`Start` skip (matching \"permission denied\", \"connection refused\", \"no such file\", \"dial unix\") is gone entirely. The call is now `require.NoError(t, err)`, so a real regression in the scrape or remote-write path surfaces as a failure rather than a silent skip. The two remaining skips are both properly scoped to containerd availability, which is exactly the right boundary.\n\nAll earlier concerns (Warn log in `disabled.go`, `coreutil.ResolveRuntimeConfig` at both decision points, Docusaurus admonitions in `server-config.md`, `config_cluster_name` qualified as a telemetry fallback) remain addressed. No new issues introduced. This is ready to merge.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6c8BIu",
"body": "The broad post-`Start` skip is gone — `require.NoError(t, err)` at line 116 means any real startup failure is now a hard test failure. The only skips left are the two containerd availability checks at the top of the test, which are exactly the right scope. Resolving this.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6c8BI2",
"body": "Line 96 is now `d.log.Warn(...)`, consistent with the resync path at line 123. Exactly what I asked for. Resolving.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true
}All three open threads from my previous pass are addressed in the current code:
disabled.go line 103 — the error log is now r.log.Warn(...), exactly as requested.targets.go line 189 — coreutil.ResolveRuntimeConfig is used consistently at both decision points (targets.go and disabled.go line 158).docs/docs/server-config.md lines 215–227 — both Docusaurus admonitions (:::warning and :::info) are in place, and the config_cluster_name description in server-config.md line 82 and command/server.md line 29 correctly qualifies it as the telemetry label fallback.Nothing new has appeared in the top-level comments beyond repeated /biscuit review invocations. The CodeQL thread on hack/cmd/remote-write-dump/main.go remains resolved and outdated — still a false positive on a dev tool, still not a concern.
My overall assessment of the PR is unchanged and the requested improvements have landed cleanly. This draft is ready to hand to human reviewers.
Posted to the PR's comment threads when you submit.
r.log.Warn(...) — exactly right. Closing this out.coreutil.ResolveRuntimeConfig: targets.go line 189 and disabled.go line 158. The concern is fully resolved.:::warning and :::info admonitions are in place at lines 215–227 of server-config.md. Looks good — resolving.Verdict: ready
My verdict is unchanged: this draft is ready to hand to human reviewers.
Since my last pass, the only new item is a CodeQL alert on hack/cmd/remote-write-dump/main.go flagging "clear-text logging of sensitive information." The thread is already marked outdated. I traced the actual data flow: req.Header.Get("Authorization") flows into parseAndCheckClaims, but every error path that function returns emits a generic message ("malformed bearer token", "token does not contain audience …") — the raw token value is never interpolated into the log call in reject. The alert is a false positive, and it's on a developer-only smoke-test tool under hack/, not production code. It doesn't affect merge readiness.
Everything from the previous review stands. The privateMetricsPath comment is present and correct at line 764. The appmetrics lifecycle, lock discipline, token rotation, atomic file writes, config validation, and test coverage are all solid. Nothing new in the thread history or top-level comments changes the picture.
Verdict: ready
I re-read the PR in its current state. No inline threads remain, and the only top-level comment is the /biscuit review trigger.
The one caveat I raised last time is addressed. The privateMetricsPath function at line 760 now has the comment I asked for:
Public HTTP ingress only routes the web service. If that ever expands to other services, their private metrics paths must be checked here too.
That's precisely what I wanted — a signal to future authors that the service.Name == "web" filter is load-bearing, not accidental. It's there, it's clear, and it says the right thing.
Everything else I examined in the first review holds. The appmetrics lifecycle is correct (error paths call stopBackground() before returning), token rotation and watcher goroutines are properly cleaned up, the target-sync path is correctly lock-disciplined, writeFileAtomic is implemented safely, the versionConfigs LRU is a sound optimization over immutable entities, config validation is thorough, and the privacy check runs before auth middleware. The tests cover the material paths including the full integration round-trip and token file permission enforcement.
This draft is ready to hand to human reviewers.
Verdict: ready
This is a draft PR, so I'm judging whether it's ready to hand off to human reviewers. Having read the whole change, I think it is — the implementation is solid and the concerns I have are small enough to act on now or flag before merge.
Adds a appmetrics component that runs a vmagent container inside the coordinator's containerd namespace. It watches sandboxes via the entity index, builds Prometheus file-SD targets for every running sandbox whose app.toml enables metrics, and forwards scraped samples to a configurable remote-write endpoint authenticated with a workload-identity bearer token that rotates every ~55 minutes. The httpingress server gains a privateMetricsPath guard that 404s requests to the configured metrics path when public: false, preventing the scrape endpoint from being reachable through the public ingress.
appmetrics.go: Start/Stop lifecycle. Error paths correctly call stopBackground() before returning, so the token rotation goroutine and watcher are cleaned up on every failure. writeFileAtomic is correctly implemented with Chmod before Write, atomic rename, and a deferred Remove that's a safe no-op after a successful rename.targets.go: The sync-event path copies current targets to previous before releasing the lock, then builds the next set in an unlocked region, and restores stale entries on transient lookup errors. This is correct.disabled.go: The dedup map is guarded by a mutex, and sandboxes being deleted require no cleanup because the key is keyed on immutable version+service rather than sandbox ID.httpingress.go: The versionConfigs LRU is a sound optimization — AppVersion entities are immutable after creation, so caching by version ID is safe. The privacy check (privateMetricsPath) runs before auth middleware, which is the right call: a 404 before auth avoids leaking route information via redirect flows. The function is scoped to service.Name == "web", which makes sense because the ingress only routes web traffic.0600, the audience is enforced in the workload identity verifier, and the integration test verifies the full round-trip including that spoofed labels in scraped samples are overwritten by vmagent's honor_labels: false configuration.ValidateMetricsCoherence correctly gates on http/https schemes, rejects embedded credentials, and requires both url and audience to be set together.privateMetricsPath only guards service.Name == "web" — if a future app defines metrics on a non-web service (e.g. a background worker service exposed via a separate ingress), that path wouldn't be blocked. This is clearly intentional today since the ingress only routes web traffic, but the function gives no indication of this assumption. A short comment — "only the web service is routed through the public ingress" — would make this safe to evolve without accidentally creating a future hole.
privateMetricsPath function only checks service.Name == "web". This is correct today because the ingress only routes web traffic, but it's a silent assumption that could become a gap if a non-web service ever gets its own ingress route. A short comment explaining the constraint would make this safe to evolve without accidentally skipping a future service's metrics path.Verdict: ready