feat(webapp,run-engine,core,clickhouse): surface combined concurrency in metrics and dashboard - #4830
Conversation
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8f5ad5d to
c60df90
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
c60df90 to
2816d30
Compare
fe6772f to
6bc8bef
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e802dab to
1627de4
Compare
matt-aitken
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1627de4 to
f4b8109
Compare
f4b8109 to
ecfbdbb
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Observability mapAs of Nothing in this pull request moves the report any more. The findings an earlier push reported are gone. The score and findings here are report-only and never gate the merge. Separately, a required test suite keeps this tool's symbol and route lists in sync with the code they name, and can fail a pull request that renames or removes a symbol they reference, or that adds the first route with a segment they anticipate. Each failure names the list to edit. The rules and their reasons: internal-packages/observability-map/README.md. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The test harness splits a migration's up section on semicolons, so a semicolon inside a comment yields a comment-only statement that ClickHouse rejects as an empty query and every container-backed suite fails at setup.
Queue retrieve only asks the engine for total running when a total limit is set, matching the list presenter and avoiding a pointless read for the common uncapped case.
The total concurrency numbers come from live Redis, not the metrics pipeline, so the column belongs in both tables rather than only behind the queue metrics UI gate.
A separate Total column implied every queue should have one, and its dash read as a missing limit on queues that never use concurrency keys. Only queues that declare a totalConcurrencyLimit now change: their Limit cell reads as per-key plus total (e.g. 1 /key, 3 total) and Running turns warning-colored when the total cap is saturated. Plain queues are unchanged.
The total cap gates keyed admissions, so the Running cell now warns off the group count rather than the aggregate that also includes unkeyed runs.
…dashboard and metrics Queue API responses expose concurrency.combined, the dashboard says combined, and the new metrics columns are named combined_running and combined_limit.
Queues that set a combinedConcurrencyLimit show it bracketed next to the per-key limit with a fine dashed underline and an explanatory tooltip; the Limit header tooltip is width-capped. Queues without one are unchanged.
The tooltip trigger is a button, so nesting it in the Limit cell's link made clicking it navigate; it now renders as the cell's trailing adornment.
Also reflows an import to the formatter's current output.
The dequeue gauge ran before the loop, so a queue's first batch from idle and its final drain were never sampled with their runs in flight and the combined chart under-reported. The successful path now re-samples after admissions; early returns keep the entry sample. Also documents that combined.current is the declared cap, clamped at admit time.
…mit column Removes the override-aware admit and gauge reads from the CK Lua scripts and the per-key limit column, following the removal of runtime per-key overrides from this stack.
The class the ignore covered is deleted at this level, so the merged result carries no dead-code exemption.
The plain dequeue gauge re-samples after admissions like the keyed one, the repair path clears a keyed run's variant and group slots by concurrency key, pause responses include the combined limit, stale wording and a leftover changeset from before the rename are cleaned up, and two empty flag blocks are removed from the fast-path scripts.
ec7828e to
35972f5
Compare
…e and gauges The CK enqueue and dequeue scripts read the combined concurrency limit key once for admission and again for the metrics gauge tail. A per-call memo makes whichever runs first do the single GET; limits cannot change mid-script, so the value stays exact. Group cardinality remains a fresh read because gauges must reflect post-admission state.
The gauge slot is single-valued and the last write wins, so on the success path the post-admission resample made the entry sample pure waste. A return wrapper computes the gauge exactly once per call at exit, keeping every emitted value identical while dropping the discarded reads (about six per plain dequeue, ten per keyed dequeue at full sampling).
… fails the suite The gauge assertions were all satisfiable by enqueue-emitted gauges, so breaking the dequeue scripts' sample-at-return wrapper left the suite green. The base test now requires the post-admission reading (running 1, queued 0) and the CK test requires the wildcard aggregate only the CK dequeue emits. Verified by mutation: disabling the wrapper fails both.
…cising it The emitter drops emissions until its Redis client is ready, and the tests enqueued immediately after constructing it, so the first counter entry was occasionally lost and the suite flaked roughly two runs in eighty. Awaiting readiness makes every emission land.
…cover the consumer round-trip An unreachable Redis leaves waitUntilReady pending forever, so the bounded wait fails fast with a descriptive error instead of burning the test timeout. The consumer round-trip test gains the same readiness wait its gauge sibling already had, closing the remaining first-emission drop flake.
… cannot reject unhandled
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…test, honest timer comment The per-stream batches test carried the same first-emission drop race as its siblings; it now waits for the emitter connection too. The readiness helper's comment claimed a losing race branch rejects unhandled, which is not how Promise.race behaves (it handles every input); the abort's real benefit is releasing the timer promptly.
A quit written to a socket that accepted but never completes the handshake never settles, which made the diagnostic throw unreachable. Closing without awaiting keeps the fast, descriptive failure.
1 participant
Summary
Queues with a
combinedConcurrencyLimitnow show how they use it, in the dashboard and the API. Stacked on #4829.The queues list gains a Total column showing runs in flight across all concurrency keys against the queue's total cap, turning warning-colored when the cap is reached. The queue detail page gains a "Total concurrency" chart plotting total running against the effective cap over time. Queue retrieve and list API responses include the same totals under
concurrency.combined.Design
The queue metrics gauge (emitted from the dequeue and enqueue Lua scripts) is extended with two fields: total running (the group concurrency set's cardinality) and the raw stored total limit. They flow through the existing metrics stream into the ClickHouse raw table and are aggregated as
max_total_running/max_total_limitin the 10s and 5m rollup tiers, queryable from the dashboard's TRQL widgets. The per-key rollup additionally records the queue limit that applied to each key.Reading the live numbers is O(pages shown): the list presenter batches one SCARD per queue that has a cap.