Conversation
![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 design document, docs/dashboard-metrics.md, which outlines the strategy for enabling the admin UI to read metrics without requiring a durable store. The proposed approach involves shunt owning the aggregate in-memory, using a bounded in-memory ring for history, and enforcing label cardinality limits. Additionally, docs/storage.md is updated to clarify that these changes satisfy the dashboard's need for recent history, decoupling it from the requirement for durable storage. I have no feedback to provide on these changes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will degrade performance by 13.05%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
|
![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: 9720c2b859
ℹ️ 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".
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Alternative A stated that `ManualReader` and `InMemoryMetricExporter` are "neither feature-gated" and "re-exported unconditionally" from `opentelemetry_sdk::metrics`. Both are gated in 0.32.1: `ManualReader` behind `experimental_metrics_custom_reader` (metrics/mod.rs:50-51, 75-76) and `InMemoryMetricExporter` behind `testing` (metrics/mod.rs:67-72). The SDK's default feature set carries neither, and this crate takes those defaults (Cargo.toml:73), so neither type is reachable in the current build. The decision is unchanged — Alternative A stays rejected — but the gating is a second blocker independent of the conditional-provider constraint, so it is recorded alongside it rather than left implicit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b971eb920
ℹ️ 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".
Two bot review findings on #587, both cross-document drift: - The ring's stated limitation named only "does not survive a restart". It is also process-local, so across several processes each ring sees one replica's traffic. Decision 2 now carries that, pointing at the caveat admin-ui-delivery.md already puts on `/admin/api/pool` — this instance's view, not the fleet's. - admin-ui-delivery.md still said a dashboard worth building needs durable history and that monitoring is blocked on storage.md, which contradicts dashboard-metrics.md Decision 2. Its storage section and its "What this does not decide" list now separate a recent in-memory window, which needs no store, from restart-surviving and per-account history, which does. The blocked-feature count in that list drops from two to one accordingly: access-permission management stays blocked, monitoring becomes partly unblocked.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Decision 2 listed "no retention policy, no PII decision" among the questions a ring buffer disposes of. Both overclaim, and Decision 3 in the same document is what contradicts them: `model` is client-controlled and passed through verbatim, so whatever a client puts there is retained for the ring's window and served over the admin API, and a fixed window is itself a retention policy. Capping cardinality bounds how many distinct labels are kept, not what one contains. The claim is now scoped to what a ring genuinely avoids — schema, migration, replicas — with storage.md's fourth question recorded as inherited rather than escaped, and the label set pointed at an allowlist/redact/hash policy alongside Decision 3's bounding.
The retention scoping said the admitted label set needs an allowlist, redaction, or hash policy without choosing among them, and the choice appeared nowhere in the open-questions list — so an implementer could not tell what transform an unmatched `model` value gets before the ring retains it. Decision 3 deliberately bounds cardinality rather than content, so the transform is a separate decision this record does not settle. It is now stated as such: open question 5, with a pointer to it from the retention paragraph.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97d4f727e3
ℹ️ 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".
Decision 1 said Sentry and OTel "become readers of that structure rather than its only home". Taken literally that breaks the sixteen counters and histograms: they push once per observation (Sentry `capture()`, OTel `add()`/`record()`, src/metrics.rs:423-443), and only the two observable gauges are read by a callback. Feeding a running aggregate into `add()` would accumulate it twice, and replacing the per-observation writes with a callback would stop those series exporting. The aggregate is an additional read model written alongside the existing emissions, which both sinks keep unchanged — the gauge pattern is the model for reading a value in-process, not for how a counter reaches a sink.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Alternative C was still headed "both sinks read from it" while Decision 1 now says the aggregate is an additional read model and the sinks keep receiving individual observations. The adopted alternative and the decision it adopts described different data flows, so an implementer could not tell whether exporters consume observations or aggregate values. C now names what is applied uniformly — the gauge pattern's ownership, not its emission — and points at Decision 1 for why.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
|
1 participant
Summary
Document how the admin dashboard can display the 18 metric series that shunt already records.
The decision record explains why the current OpenTelemetry instrument handles cannot serve as a
read path and why tying dashboard visibility to an
[otel]endpoint would invert the intendeduse case.
The record adopts three decisions:
This adopts no store and changes no code, behavior, configuration, endpoint, or CLI surface. It
removes the dashboard from the list of work blocked on the storage decision. The accompanying
docs/storage.mdscope note preserves the durable-storage requirement for history that survivesrestarts and for seven-day per-account retention.
Issue #296 remains open and is referenced only as context. Its title scopes the unbounded
modellabel to the Codex endpoint, but the
/v1/messagesdefault-provider fallback has the sameproperty. A Codex-only fix could therefore close #296 while leaving the Anthropic path unbounded.
Milestone / spec
Documentation-only design record:
docs/dashboard-metrics.md, aligned withdocs/storage.mdanddocs/admin-ui-delivery.md.Checklist
cargo buildpasses — not run; no code changedcargo testpasses (new behavior is covered; tests run without network/loopback where possible) — not run; no behavior changedcargo clippy --all-targets -- -D warningsclean — not run; no Rust changedcargo fmt --all --checkclean — not run; no Rust changeddocs/updated if this change deviates from itNotes for reviewers
This PR is a decision record, not an implementation. No tests apply because no code changed.
Every
src/<file>:<line>reference in the new document was mechanically verified to resolve tothe claimed line. Internal document links and the
admin-ui-delivery.mdanchor were also verifiedto resolve.
Summary by cubic
Adds
docs/dashboard-metrics.md, a documentation-only design record for how the admin dashboard can display the 18 metric series shunt already records, without adopting a store or changing code. It unblocks monitoring for a recent window; restart-surviving history and seven-day per-account retention still require durable storage.modelis client-controlled and passed through verbatim, the label set needs an explicit allowlist/redact/hash policy; the choice of transform is left as an open question.opentelemetry_sdk0.32.1 feature-gating as an independent reason to reject theManualReaderread path: bothManualReaderandInMemoryMetricExporterare gated behind features this crate does not enable.docs/admin-ui-delivery.mdanddocs/storage.mdto reconcile them with the new record; access-permission management remains blocked on the storage decision.modelmetrics label length #296 is scoped to the Codex endpoint, but the/v1/messagesdefault-provider fallback has the same unboundedmodellabel property.Written for commit 1961798. Summary will update on new commits.