RSPEED-2943: centralize metrics recording by major · Pull Request #1622 · lightspeed-core/lightspeed-stack

major · 2026-04-28T20:05:11Z

Description

Centralizes Prometheus metric updates behind a small metrics.recording facade so new metrics can be added without spreading Prometheus object details across application code. Existing metric names, labels, and output stay unchanged.

Type of change

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

Assisted-by: OpenCode
Generated by: N/A

Related Tickets & Documents

Related Issue # RSPEED-2943
Closes #

Checklist before requesting a review

I have performed a self-review of my code.
PR has passed all pre-merge test jobs.
If it is a core feature, I have added thorough tests.

Testing

uv run pytest tests/unit/metrics/test_recording.py tests/unit/app/test_main_middleware.py tests/unit/utils/test_responses.py tests/unit/utils/test_shields.py tests/unit/app/endpoints/test_streaming_query.py tests/unit/app/endpoints/test_rlsapi_v1.py
- Result: 361 passed, 3 warnings
uv run black --check src/app/endpoints/rlsapi_v1.py src/app/endpoints/streaming_query.py src/app/main.py src/metrics/__init__.py src/metrics/recording.py src/utils/responses.py src/utils/shields.py tests/unit/app/endpoints/test_streaming_query.py tests/unit/app/test_main_middleware.py tests/unit/metrics/test_recording.py tests/unit/utils/test_responses.py tests/unit/utils/test_shields.py
- Result: 12 files would be left unchanged
uv run make verify
- Result: passed black, pylint, pyright, ruff, pydocstyle, and mypy

Summary by CodeRabbit

Release Notes

Refactor
- Streamlined metrics recording through a centralized recording API, replacing direct metric updates across LLM calls, token usage tracking, and REST API monitoring with a unified approach featuring improved error handling to ensure metrics are captured reliably.
Tests
- Updated unit tests to validate the new centralized metrics recording system.

coderabbitai · 2026-04-28T20:05:25Z

Warning

Rate limit exceeded

@major has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 46 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: eef7b2c7-c04f-4d99-bd78-b0b303683762

📥 Commits

Reviewing files that changed from the base of the PR and between 27816d5 and 7a76f83.

📒 Files selected for processing (12)

src/app/endpoints/rlsapi_v1.py
src/app/endpoints/streaming_query.py
src/app/main.py
src/metrics/__init__.py
src/metrics/recording.py
src/utils/responses.py
src/utils/shields.py
tests/unit/app/endpoints/test_streaming_query.py
tests/unit/app/test_main_middleware.py
tests/unit/metrics/test_recording.py
tests/unit/utils/test_responses.py
tests/unit/utils/test_shields.py

Walkthrough

The pull request introduces a new metrics.recording facade module that centralizes Prometheus metric recording operations. Direct metric counter increments throughout the codebase are refactored to call standardized recording functions for LLM operations, REST API tracking, and response duration measurement. Corresponding test updates verify the new recording layer.

Changes

Cohort / File(s)	Summary
Recording Facade `src/metrics/recording.py`	New module introducing a centralized recording API with context manager `measure_response_duration()` and functions for REST API tracking, LLM call/failure/validation tracking, and token usage recording. Includes error handling with try/except wrapper for robustness.
Endpoint Metrics Refactoring `src/app/endpoints/rlsapi_v1.py`, `src/app/endpoints/streaming_query.py`, `src/app/main.py`	Refactor direct Prometheus metric increments to use `metrics.recording` functions: LLM failure and call tracking in endpoints, response duration and REST API call counting in middleware.
Utility Metrics Refactoring `src/utils/responses.py`, `src/utils/shields.py`	Replace direct metric manipulation with recording function calls: token usage recording via `record_llm_token_usage()` and LLM calls via `record_llm_call()` in responses; validation error tracking via `record_llm_validation_error()` in shields.
Metric Documentation `src/metrics/__init__.py`	Update inline documentation comments for `llm_token_sent_total` and `llm_token_received_total` counters to clarify their purpose; no metric definitions or behavior changed.
Test Updates `tests/unit/app/endpoints/test_streaming_query.py`, `tests/unit/app/test_main_middleware.py`, `tests/unit/metrics/test_recording.py`, `tests/unit/utils/test_responses.py`, `tests/unit/utils/test_shields.py`	New test module for recording facade; updates to existing tests to mock recording functions instead of direct Prometheus metrics, including verification of context manager and error-handling paths.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: centralizing metrics recording behind a facade, which aligns with the primary refactoring across the entire changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 84.31% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

✨ Simplify code

Create PR with simplified code

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/metrics/recording.py`:
- Around line 17-35: The metrics calls in measure_response_duration and
record_rest_api_call can raise and currently bubble into request handling; wrap
the metrics interactions in try/except to prevent telemetry failures from
affecting API responses. For measure_response_duration
(metrics.response_duration_seconds.labels(path).time()) catch exceptions around
creating/entering the timing context and ensure the contextmanager still yields
once so the request proceeds; log the exception (non-fatal) instead of
re-raising. For record_rest_api_call wrap
metrics.rest_api_calls_total.labels(path, status_code).inc() in a try/except,
log any exception and swallow it so the request flow is unchanged. Use the
existing project logger or a safe fallback logger for the error messages.

In `@tests/unit/utils/test_shields.py`:
- Line 201: The test currently patches
utils.shields.recording.record_llm_validation_error but never asserts it was
invoked; update the default-message blocked-path test to assert the patched
recorder was called (e.g., assert_called_once() or assert_called_with(...)
depending on expected args) after exercising the blocked-path branch,
referencing the patched symbol
"utils.shields.recording.record_llm_validation_error" to locate the mock and
ensure the metric/instrumentation is verified.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 11106912-7360-4785-a9c3-0bff9353f7f6

📥 Commits

Reviewing files that changed from the base of the PR and between 93b2bb5 and 27816d5.

📒 Files selected for processing (12)

src/app/endpoints/rlsapi_v1.py
src/app/endpoints/streaming_query.py
src/app/main.py
src/metrics/__init__.py
src/metrics/recording.py
src/utils/responses.py
src/utils/shields.py
tests/unit/app/endpoints/test_streaming_query.py
tests/unit/app/test_main_middleware.py
tests/unit/metrics/test_recording.py
tests/unit/utils/test_responses.py
tests/unit/utils/test_shields.py

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

GitHub Check: E2E: library mode / ci / group 2
GitHub Check: E2E: server mode / ci / group 1
GitHub Check: E2E: library mode / ci / group 3
GitHub Check: E2E: server mode / ci / group 3
GitHub Check: Pyright
GitHub Check: build-pr
GitHub Check: E2E Tests for Lightspeed Evaluation job
GitHub Check: Pylinter
GitHub Check: unit_tests (3.13)
GitHub Check: unit_tests (3.12)
GitHub Check: mypy

🧰 Additional context used

📓 Path-based instructions (5)

**/*.py