Repository navigation
[chore] Report completed storage requests from the latency histogram - #192
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟢 Approval recommended
The focused metric correction is consistent with existing histogram semantics and has appropriate regression coverage.
0 open findings
What changed in this PR
Corrects storage operation completion metrics to exclude in-flight handlers.
Changes:
- Derives completed request counts from histogram observations.
- Adds coverage for running and failed handlers.
| File | Description |
|---|---|
transfer_queue/storage/simple_storage.py |
Reports completed operations accurately. |
tests/test_storage_request_retry.py |
Tests in-flight and failed handler counts. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 7285258c34
ℹ️ 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".
| "request_count": counter._value.get(), | ||
| # Observed when the handler returns or raises; request_total counts entry, | ||
| # which would hide a handler that never finishes. | ||
| "request_count": duration_count, |
There was a problem hiding this comment.
When a real storage handler stalls, this completion value cannot be queried concurrently: _worker_routine synchronously runs the single _process_one_worker_request path (transfer_queue/storage/simple_storage.py:1048-1067), and GET_METRICS is another branch of that same dispatcher at lines 1118-1119. The collector's metrics request therefore waits behind the stuck request (and times out), while after the handler returns the histogram has already been observed, so tq_storage_request_ops never exposes the intended arrival/completion gap. The new test only passes because it calls _handle_get_metrics() directly from a second thread, which the supported ZMQ path cannot do; this snapshot needs an independently serviced path and a test through the real dispatcher.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. A storage unit has one worker, and it answers GET_METRICS only after the previous handler has returned. Through the supported path, request_total and the histogram count are therefore always equal when read. A stuck handler shows up as an unanswered collector query or probe (unit_not_serving), not as a gap.
I've narrowed the PR to match (4146d6d):
- Framing: the change now only makes
request_countmean "completed" by construction, from the same histogram thatlatency_avguses, and it states that no reported value changes. - Docs:
docs/metrics.mdclaimed that the collected gap reveals unfinished requests. That claim is now removed. - Test: it stays as a contract test for the field, and its docstring now says that this interleaving is not reachable through the worker today.
I don't think a separate metrics path is needed. The storage unit's own endpoint is served by its own thread, so handlers still in progress are already visible there as tq_storage_request_total minus tq_storage_request_duration_seconds_count; the updated doc now points there.
There was a problem hiding this comment.
Follow-up: I dropped the test in 4a223d5. It could only tell the old and new source apart by reading the metrics while a handler is still running, which is the interleaving this worker never produces, so it guarded nothing a user can observe. The PR is now the field source plus the docs/metrics.md correction.
7285258 to
4146d6d
Compare
0oshowero0
changed the title
op_stats.request_count is documented as completed requests, but it was read from request_total, which measure() increments on entry. The two agree only because a storage unit's single worker answers GET_METRICS after every earlier handler has returned. Read the request-duration histogram instead, which latency_avg already uses, so the field means completed by construction. No reported value changes. docs/metrics.md claimed that a gap between arrivals and tq_storage_request_ops shows requests that arrived and did not finish. The same worker answers the collector, so it cannot: a stuck unit stops answering instead. Say so, and point to the storage unit's own endpoint, which can show handlers still running. Signed-off-by: 0oshowero0 <o0shower0o@outlook.com>
4146d6d to
4a223d5
Compare
Motivation
op_stats.request_countis documented as the number of completed requests:tq_storage_request_ops;completed=;It is actually read from
request_total, whichTQMetricsExporter.measure()increments on entry, before the handler runs. The two numbers agree today only because a storage unit has a single worker, and that worker answersGET_METRICSafter every earlier handler has returned.docs/metrics.mdalso draws a conclusion that these counters cannot support: it says that a sustained gap between arrivals andtq_storage_request_opsmeans requests are arriving and not finishing. Because the same worker answers the collector, the counters are only reported between requests. A unit stuck in a handler stops answering instead; the collector skips it, and its gauges stay at their last values.Found while reviewing #189.
Change
request_count: read it from the request-duration histogram.measure()observes the histogram when the handler returns or raises, andlatency_avgin the same function already uses its count. The field then means "completed" by construction. No reported value changes today.docs/metrics.md: say what the collected counters can and cannot show, and point to the storage unit's own endpoint instead. That endpoint is served by its own thread, sotq_storage_request_totalminustq_storage_request_duration_seconds_countgives the handlers still running, even while the worker is stuck. Also,tq_storage_request_totalis now described as requests whose handler has started; it was "Total requests processed".Validation
This PR adds no test. No reported value changes, and telling the old source from the new one would require reading the metrics while a handler is still running, which the single worker never does.
Checks run:
pre-commit(ruff, ruff-format, mypy)python -m compileall -q transfer_queue tutorial testspython -m pytest -q, excludingtests/e2eand the Yuanrong e2e tests, which need the optionaldatasystempackage: 671 passed, 8 skipped on the first revision. One run oftests/test_controller.py::test_controller_clear_metafailed when the local Raylet crashed; the file passes on rerun. Later revisions changed only comments and docs, andtests/test_storage_request_retry.pyandtests/test_metrics.pypass.