Pull request overview

Fixes OpenMetrics unit compliance for the session_duration counter by separating the metric family name (used for # TYPE/# UNIT metadata and protobuf MetricFamily.name) from the emitted counter sample name (which must keep the _total suffix).

Changes:

• Rename SESSION_DURATION_FAMILY from session_duration_seconds_total → session_duration_seconds to make the declared unit (seconds) a valid suffix of the family name.
• Extend CounterStat with an optional sample_name override so emitted samples can differ from family metadata while preserving existing behavior when unset.
• Update runtime producer and tests to emit session_duration_seconds_total samples under the session_duration_seconds family for both text and protobuf outputs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Summary

This PR fixes an OpenMetrics compliance error where Prometheus scrapers reject the session_duration_seconds_total metric with: unit "seconds" not a suffix of metrics "session_duration_seconds_total". Per the OpenMetrics spec, the declared UNIT must be a suffix of the MetricFamily name.

The fix renames SESSION_DURATION_FAMILY from "session_duration_seconds_total" to "session_duration_seconds" and introduces an optional sample_name field on CounterStat so the emitted sample line can still use the required _total suffix while the family metadata uses the canonical (unit-suffixed) name.

Reviewer consensus: All three reviewers (claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high) approved the PR unanimously. There were no disagreements on the approach, correctness, or scope of the change.

Code Quality

All reviewers agree the implementation is clean, minimal, and well-structured:

• Adding sample_name as an optional field on a NamedTuple with a None default preserves full backward compatibility. All existing call sites use keyword arguments, so the new field at the end is safe.
• to_metric_str() cleanly falls back to family_name when sample_name is None, preserving existing behavior for all other counters.
• marshall_metric_proto() correctly does not use sample_name — in protobuf format, the family name already identifies the metric family with no separate "sample name" concept.
• The scope is well-contained: only the broken unitful counter is renamed, and the PR description explains why session_events_total is intentionally left unchanged.

Test Coverage

All reviewers rated test coverage as excellent/strong. The PR adds and updates tests across all layers:

• stats_test.py: New unit tests for CounterStat.to_metric_str() with sample_name (with and without labels), plus assertion that sample_name is None by default.
• websocket_session_manager_test.py: All references to the old family name updated; new assertion verifying sample_name is correctly set.
• stats_handler_test.py: New tests for unitful counter protobuf output and text-format output verifying canonical # TYPE, # UNIT, and sample lines.
• starlette_app_test.py: New protobuf test and text-format assertions added for the metrics endpoint.

No e2e tests were added, but all reviewers agree the existing unit/integration coverage is sufficient for this backend-only metrics formatting change.

Backwards Compatibility

All reviewers agree the change is backward compatible for normal usage. The observable changes are:

The sample line is unchanged, so Prometheus scraping rules matching on session_duration_seconds_total continue to work. One reviewer (gpt-5.3-codex-high) noted a minor compatibility consideration: consumers that filter by metric family name (e.g., via families=session_duration_seconds_total) would need to update to families=session_duration_seconds. This is a valid but low-impact observation since the old output was non-compliant and caused Prometheus scraping errors. The SESSION_DURATION_FAMILY constant is in an internal module and not part of the public st.* API.

Security & Risk

All reviewers agree there are no security concerns. Changes are limited to internal metrics reporting plumbing — no new endpoints, no authentication changes, no external dependencies, and no user-facing input handling.

External test recommendation

• Recommend external_test: No
• Triggered categories: None
• Evidence:
  • Changes are limited to internal metrics data model (stats.py) and its consumer (websocket_session_manager.py)
  • No routing, auth, websocket handshake, embedding, asset serving, CORS, or security header changes
  • The /_stcore/metrics endpoint behavior change is purely in metric name/format compliance
• Suggested external_test focus areas: N/A
• Confidence: High (unanimous across all three reviewers)
• Assumptions and gaps: None — the changes are clearly scoped to internal metrics infrastructure with no impact on externally hosted or embedded behavior.

Accessibility

No frontend changes; accessibility is not applicable. All reviewers concur.

Recommendations

1. Consider calling out the metric-family rename (session_duration_seconds_total → session_duration_seconds) in release notes for operators using families= filters or protobuf consumers keyed by family name. (Raised by gpt-5.3-codex-high; agreed as good practice by consolidation review.)

Verdict

APPROVED: All three reviewers unanimously approved. The fix is well-scoped, correctly addresses the OpenMetrics unit-suffix compliance issue, has thorough test coverage across all serialization layers, and carries no backward compatibility concerns for normal usage.

This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from: claude-4.6-opus-high-thinking, gemini-3.1-pro, gpt-5.3-codex-high. All expected models completed their reviews successfully.