fix(metrics): vt-aware oldest_claimable_age_sec — one delayed job no longer reads as a degraded queue - #390
Merged
Merged
Conversation
…longer reads as a degraded queue ## Summary pgmq's oldest_msg_age_sec ignores vt, so a scheduled or backoff-parked message made age-based latency alerts fire for hours on healthy queues. Every metrics surface now exposes oldest_claimable_age_sec (now() - min(vt) over rows with vt <= now()): DataSource (dashboard/API/MCP), Prometheus gauge, AppSignal gauge, Client#oldest_claimable_ages, CLI CLAIMABLE column. AppSignal queue_latency now derives from the claimable age (always emitted, 0 = no claimable backlog). Dashboard queue tables split depth into claimable vs parked and show the claimable age. ## Test Coverage - data_source specs: SQL selects min(vt) scoped to claimable rows; parked-only queue maps to nil - metrics_serializer spec: new gauge emitted, nil-skipped - probe spec: queue_latency from claimable age, 0 when parked-only; raw gauge unchanged - client spec: prefixed single-queue read + all-queues hash via raw connection - cli spec: CLAIMABLE column, dash for parked-only - integration spec (real PG): delayed message -> nil claimable while raw age counts it; in-flight excluded ## Verification - [x] bundle exec rubocop passes - [x] bundle exec rspec passes (only the 2 known pre-existing i18n baseline failures) - [x] bun run lint:herb passes - [x] docs rake lint passes Refs #389
📝 WalkthroughWalkthroughQueue metrics now distinguish claimable backlog from scheduled, retrying, and in-flight messages. Claimable age appears in APIs, Prometheus, AppSignal, CLI output, dashboards, documentation, and tests. ChangesClaimable Queue Metrics
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant QueueTables
participant Client
participant WebDataSource
participant AppSignalProbe
participant MetricsSerializer
QueueTables-->>Client: Claimable age or nil
QueueTables-->>WebDataSource: Claimable queue metric
WebDataSource->>MetricsSerializer: Serialize claimable age
WebDataSource->>AppSignalProbe: Provide claimable age
AppSignalProbe-->>AppSignalProbe: Set latency to claimable age
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/views/pgbus/dashboard/_queues_table.html.erb (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMove the "parked" calculation into the data layer instead of duplicating it in three views.
All three views independently compute parked depth as
queue_length - queue_visible_length. Add aparked_lengthfield to the metrics hashes returned bybatched_queue_metricsandqueue_metrics_via_sqlinlib/pgbus/web/data_source.rb, then reference that field from each view. This removes the duplicated arithmetic and keeps the "parked" definition in one place if it needs to change later (for example, to guard against unexpected negative values).
app/views/pgbus/dashboard/_queues_table.html.erb#L29-30: replaceq[:queue_length] - q[:queue_visible_length]withq[:parked_length]once the data source exposes it.app/views/pgbus/queues/_queues_list.html.erb#L28-29: replaceq[:queue_length] - q[:queue_visible_length]withq[:parked_length].app/views/pgbus/queues/show.html.erb#L16-17: replace@queue[:queue_length] -@Queue[:queue_visible_length]with@queue[:parked_length].♻️ Proposed data-layer addition
name: row["queue_name"], queue_length: row["queue_length"].to_i, queue_visible_length: row["queue_visible_length"].to_i, + parked_length: row["queue_length"].to_i - row["queue_visible_length"].to_i, oldest_msg_age_sec: row["oldest_msg_age_sec"]&.to_i,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/views/pgbus/dashboard/_queues_table.html.erb` at line 1, Centralize parked-depth calculation in the data layer: update batched_queue_metrics and queue_metrics_via_sql in DataSource to include a parked_length field in every metrics hash, then update the queues table, queues list, and queue show views to read q[:parked_length] or `@queue`[:parked_length] instead of subtracting queue_visible_length from queue_length.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/pgbus/client.rb`:
- Around line 556-565: Update oldest_claimable_ages to invoke
with_raw_connection with synchronization enabled, ensuring the shared
PG::Connection path is protected while executing both the metadata query and
claimable_age_for calls. Preserve the existing queue-specific and all-queues
behavior.
In `@lib/pgbus/mcp/tools/queues_tool.rb`:
- Around line 17-20: Update the queue metric documentation around
oldest_claimable_age_sec to state that nil means no message is currently
eligible for pickup, including cases where messages are scheduled,
backoff-parked, or in flight with a future vt; do not describe nil as proof that
the queue is healthy.
In `@README.md`:
- Line 994: The documentation for the pgbus_queue_oldest_claimable_age_seconds
metric does not explain its absent-series behavior. Update the metric
description to clarify that when this gauge is absent (nil, not emitting a zero
sample), it indicates there is no claimable backlog. Include context that this
absence is distinct from the raw enqueue-age gauge, which may still report
parked messages waiting for their visibility timeout to elapse.
In `@spec/integration/claimable_age_metrics_spec.rb`:
- Around line 31-32: Update the assertion for oldest_claimable_ages to fetch the
queue using client.config.queue_name("claimable_test") instead of the hardcoded
"pgbus_int_claimable_test" key, preserving the existing age expectation.
In `@spec/pgbus/web/data_source_batched_metrics_spec.rb`:
- Around line 93-118: The batched metrics specs only assert SQL tokens and
mocked results, so they do not prove future visibility times are excluded.
Strengthen the coverage around queues_with_metrics and batched_queue_metrics by
adding a PostgreSQL-backed case with both due and future-vt rows, asserting the
claimable age excludes future rows and includes them once due; verify or extend
spec/integration/claimable_age_metrics_spec.rb for both timing states.
- Around line 127-135: The batched metrics fixture’s retry-parked scenario is
inconsistent because max_read_ct is zero. In the select_all stub within the
metrics spec, set max_read_ct to a positive read count so it represents a
previously claimed message, preserving the rest of the scenario.
---
Outside diff comments:
In `@app/views/pgbus/dashboard/_queues_table.html.erb`:
- Line 1: Centralize parked-depth calculation in the data layer: update
batched_queue_metrics and queue_metrics_via_sql in DataSource to include a
parked_length field in every metrics hash, then update the queues table, queues
list, and queue show views to read q[:parked_length] or `@queue`[:parked_length]
instead of subtracting queue_visible_length from queue_length.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 21ac42e2-bc83-4300-bf31-41751947a7d5
📒 Files selected for processing (33)
CHANGELOG.mdREADME.mdapp/views/pgbus/dashboard/_queues_table.html.erbapp/views/pgbus/queues/_queues_list.html.erbapp/views/pgbus/queues/show.html.erbconfig/locales/da.ymlconfig/locales/de.ymlconfig/locales/en.ymlconfig/locales/es.ymlconfig/locales/fi.ymlconfig/locales/fr.ymlconfig/locales/it.ymlconfig/locales/ja.ymlconfig/locales/nb.ymlconfig/locales/nl.ymlconfig/locales/pt.ymlconfig/locales/sv.ymldocs/app/views/docs/pages/observability.rblib/pgbus/cli.rblib/pgbus/client.rblib/pgbus/integrations/appsignal/probe.rblib/pgbus/mcp/tools/queues_tool.rblib/pgbus/web/data_source.rblib/pgbus/web/metrics_serializer.rbspec/dummy/lib/stub_data_source.rbspec/integration/claimable_age_metrics_spec.rbspec/pgbus/cli_spec.rbspec/pgbus/client_spec.rbspec/pgbus/integrations/appsignal/probe_spec.rbspec/pgbus/web/data_source_batched_metrics_spec.rbspec/pgbus/web/data_source_spec.rbspec/pgbus/web/metrics_serializer_spec.rbspec/support/pgbus/stub_data_source.rb
…ength, doc clarifications CodeRabbit review fixes on PR #390: - Client#oldest_claimable_ages wraps the raw query in `synchronized` — on the shared-Proc path with_raw_connection yields the same AR connection every @pgmq operation uses, so an unserialized query could interleave mid-protocol - parked depth derived once in DataSource (parked_length) instead of three views - MCP queues_tool description: nil claimable age = nothing currently eligible (scheduled, backoff-parked, OR in-flight), not proof of health - README: document that the claimable gauge omits its series when nil - integration spec: derive queue key via config.queue_name; tolerate wall-clock growth between calls on a contended server - batched-metrics spec: fixture comment matches the read_ct=0 wait:-re-enqueue incident shape
Collaborator
Author
|
Re the outside-diff comment on |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/views/pgbus/queues/show.html.erb`:
- Around line 16-17: Update the queue summary view around the existing parked
and oldest_claimable entries to also render `@queue`[:oldest_msg_age_sec] with its
localized oldest-message-age label and the same "—" fallback used for missing
age values. Preserve the existing summary fields and ordering while displaying
both age metrics returned by the data source.
In `@lib/pgbus/client.rb`:
- Around line 560-568: Update oldest_claimable_ages to execute its metric
queries through the bounded `@pgmq` connection, using `@pgmq.with_connection` while
preserving the existing synchronized block and query behavior. Remove the
with_raw_connection path for this method so String/Hash configurations reuse the
statement and socket bounds applied during Client#initialize.
🪄 Autofix
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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1fd12ea5-d23e-4889-92e5-f7c4d89a91f9
📒 Files selected for processing (11)
README.mdapp/views/pgbus/dashboard/_queues_table.html.erbapp/views/pgbus/queues/_queues_list.html.erbapp/views/pgbus/queues/show.html.erblib/pgbus/client.rblib/pgbus/mcp/tools/queues_tool.rblib/pgbus/web/data_source.rbspec/dummy/lib/stub_data_source.rbspec/integration/claimable_age_metrics_spec.rbspec/pgbus/web/data_source_batched_metrics_spec.rbspec/support/pgbus/stub_data_source.rb
…n, show raw age on queue detail - Client#oldest_claimable_ages now uses @pgmq.with_connection (health-checked, bounded by the statement/socket timeouts from Client#initialize) instead of a fresh unbounded PG.connect per call — same migration notify_trigger_current? made; locally this cut the integration spec from ~150s to ~9s on a contended server - queues/show summary line now shows the raw oldest age alongside the claimable age (new pgbus.queues.show.oldest key in all 12 locales)
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/pgbus/client.rb (1)
557-576: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse enqueue time for
oldest_claimable_age_sec.
claimable_age_forsubtractsmin(vt)at Line 988. This reports visibility duration, not message age. A message enqueued one hour ago and due one second ago reports about one second.lib/pgbus/web/data_source.rb, Lines 1211-1260, computes this metric frommin(enqueued_at)after filteringvt <= NOW(). Replacemin(vt)withmin(enqueued_at)and add a regression case with an oldenqueued_atand a duevt, so client and dashboard results match.Proposed fix
- SELECT EXTRACT(epoch FROM (NOW() - min(vt)))::int AS age_sec + SELECT EXTRACT(epoch FROM (NOW() - min(enqueued_at)))::int AS age_secAlso applies to: 985-993
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/pgbus/client.rb` around lines 557 - 576, The claimable_age_for method currently calculates age using min(vt) which measures visibility duration rather than actual message age. Update the query in claimable_age_for to compute min(enqueued_at) instead of min(vt) to measure the age from when messages were originally enqueued, making the oldest_claimable_ages results consistent with how the web dashboard computes this metric. Add a regression test case that verifies the correct behavior with a message having an old enqueued_at and a recent vt value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@lib/pgbus/client.rb`:
- Around line 557-576: The claimable_age_for method currently calculates age
using min(vt) which measures visibility duration rather than actual message age.
Update the query in claimable_age_for to compute min(enqueued_at) instead of
min(vt) to measure the age from when messages were originally enqueued, making
the oldest_claimable_ages results consistent with how the web dashboard computes
this metric. Add a regression test case that verifies the correct behavior with
a message having an old enqueued_at and a recent vt value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f4512a1f-eaca-4abd-906d-941dfa7fdfe1
📒 Files selected for processing (15)
app/views/pgbus/queues/show.html.erbconfig/locales/da.ymlconfig/locales/de.ymlconfig/locales/en.ymlconfig/locales/es.ymlconfig/locales/fi.ymlconfig/locales/fr.ymlconfig/locales/it.ymlconfig/locales/ja.ymlconfig/locales/nb.ymlconfig/locales/nl.ymlconfig/locales/pt.ymlconfig/locales/sv.ymllib/pgbus/client.rbspec/pgbus/client_spec.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pgmq's
oldest_msg_age_secis computed fromenqueued_atand ignoresvt— but a job enqueued withwait:or parked on a retry backoff lives in the queue table with a futurevt(that is the delayed-delivery mechanism). One parked message therefore made the age metric grow at wall-clock rate for hours on an otherwise drained queue, and any latency alert thresholding on it fired continuously.This adds a vt-aware
oldest_claimable_age_sec—now() - min(vt)over rows withvt <= now(), i.e. the age of the oldest message actually eligible for pickup — to every metrics surface:Web::DataSource#batched_queue_metrics/#queue_metrics_via_sql(lib/pgbus/web/data_source.rb) — feeds dashboard, JSON API, and the MCPpgbus_queuestoolpgbus_queue_oldest_claimable_age_seconds(lib/pgbus/web/metrics_serializer.rb), nil-skippedlib/pgbus/integrations/appsignal/probe.rb):pgbus_queue_latencynow derives from the claimable age and always emits ((claimable || 0) * 1000, 0 = no claimable backlog) — existing latency alerts stop false-firing with no dashboard changes. The rawpgbus_queue_oldest_message_age_secondsgauge keeps enqueue-time semantics; a newpgbus_queue_oldest_claimable_age_secondsgauge is added, nil-skippedPgbus::Client#oldest_claimable_ages(queue_name = nil)(lib/pgbus/client.rb) — raw-SQL reader (pgmq'smetrics_resulttype is frozen upstream, so the SQL function can't grow the field), single-queue age or all-queues hashpgbus queuesgains a CLAIMABLE (s) columndepth − visible) and show the claimable age instead of the raw age; queue detail header shows both; all 12 locales updatedSemantics: an immediately-enqueued message contributes from enqueue time (matches the old number on a plain backlog); a scheduled/backoff-parked message contributes nothing until due — then grows at wall-clock rate, which is exactly the starvation signal the alert wants; an in-flight message (vt pushed forward) is excluded; nil means "no claimable backlog" even when the table is non-empty.
Closes #389
Test plan
min(vt)scoped tovt <= NOW(); a parked-only queue mapsoldest_claimable_age_secto nil whileoldest_msg_age_seckeeps counting (the incident shape from Queue-age metrics count vt-parked (scheduled/retrying) messages, so one delayed job reads as a degraded queue #389)queue_latency= claimable × 1000; 0 for parked-only and empty queues; raw gauge unchanged; claimable gauge nil-skippedClient#oldest_claimable_ages— prefixed single-queue read and all-queues hash viawith_raw_connectiondelay: 3600→ claimable nil while raw age counts it; immediate message → age ≥ 0 on both paths; message read withvt: 60(in-flight) → claimable nilDeviations & judgment calls
docs/app/views/docs/pages/observability.rb's "gauge list" — that page only documents the event-driven metrics backend; the dashboard-scrape gauge table actually lives in README.md ("Prometheus metrics"). Added the new gauge row to the README table and expanded the AppSignal section wording in the docs page instead.queues_table.headers.oldest/queues_list.headers.oldestkeys from all 12 locales — the plan only said "add keys", butspec/i18n_spec.rbruns i18n-tasks' unused-keys check, which would fail once the views stopped referencing them.queue_metrics_via_sqlthe claimable age uses an aggregateFILTER (WHERE vt <= NOW())inside the existing single-pass CTE, whilebatched_queue_metricsuses a scalar subselect — each matches the surrounding query's existing style; both produce identical results.Client#oldest_claimable_agesruns one query per queue on the all-queues path (same shape as pgmq's ownmetrics_allloop) rather than one dynamic-SQL statement — table names are per-queue identifiers and the only caller is the CLI.metricsstruct returnsqueue_lengthas a string on the integration path; the integration spec compares via.to_i.queue_latencyalways emits (0 when nothing is claimable) so alert series show "healthy" rather than no-data; the raw and claimable age gauges stay nil-skipped, mirroring the existing idiom.Summary by CodeRabbit
pgbus_queue_oldest_claimable_age_seconds.