Add faktory.queue.paused gauge to metrics plugin - #41
Merged
Conversation
Emits a per-queue gauge (1 if paused, 0 otherwise) so dashboards and alerts can distinguish "queue has no work" from "queue is paused and won't drain." Pause state is read once per tick via store.PausedQueues(ctx) and checked against each queue in-memory inside the existing EachQueue loop. Also marks the legacy namespaced "<namespace>.*" metrics in task.go and middleware.go as deprecated; new metrics should be emitted exclusively via the canonical faktory.* tagged form.
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a new per-queue gauge metric 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 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 `@metrics/metrics_test.go`:
- Around line 271-273: The fixed 500ms sleep is flaky; remove time.Sleep(500 *
time.Millisecond) and instead wait deterministically for the expected paused
metric emission by wiring a synchronization primitive (channel or
sync.WaitGroup) into the mocked emitter used in this test: have the code that
emits the paused metric signal a channel (e.g., close a doneCh or send on
doneCh) and then block the test on receiving from that channel before letting
the deferred pausedCtrl.Finish() run; update the test to reference the existing
pausedCtrl and the metric emission point (the mocked emitter or Pause handler
that currently produces the paused metric) so the test proceeds only after the
actual emission is observed.
In `@metrics/task.go`:
- Around line 75-93: Paused queue retrieval errors cause pausedSet to be empty
and emit false "faktory.queue.paused=0" for every queue; change the logic so
that when m.Subsystem.Server.Store().PausedQueues(ctx) returns an error you log
the error and skip emitting the "faktory.queue.paused" Gauge for that tick
(i.e., don't call m.Subsystem.StatsDClient().Gauge("faktory.queue.paused", ...)
inside the EachQueue loop). Implement this by recording the error state (e.g., a
boolean like pausedLookupFailed) after calling PausedQueues(ctx) and only
computing/using pausedValue and calling the Gauge when the lookup succeeded.
🪄 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 YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 275672c8-2158-40a1-80ba-810bba83ce27
📒 Files selected for processing (4)
README.mdmetrics/metrics_test.gometrics/middleware.gometrics/task.go
- task.go: skip the faktory.queue.paused emission when PausedQueues returns an error, instead of emitting "0" for every queue (which would falsely report queues as active when we don't actually know). - metrics_test.go: replace the fixed 500ms sleep in the paused-queue subtest with a deterministic channel close via DoAndReturn, plus a 5s timeout fallback.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@metrics/metrics_test.go`:
- Around line 251-282: The test races because pausedEmitted is closed on the
first metric emission; change synchronization to wait for the final
unconditional metric emission instead: stop closing pausedEmitted in the Gauge
expectation for "faktory.queue.paused" and instead close a new (or reuse
pausedEmitted) channel in the Gauge expectation that matches the final
unconditional metric "jobs.enqueued.count" (the Gauge call with name
"jobs.enqueued.count") so the test waits for the goroutine's last mockDoer call
before runSystem returns and pausedCtrl.Finish() runs; update the
mockDoer.EXPECT() that currently DoAndReturn-closes the channel to target the
"jobs.enqueued.count" Gauge call (leave the paused queue Gauge expectation as a
simple Times(1) check).
🪄 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 YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2502e638-0626-4256-9638-e406037994e1
📒 Files selected for processing (2)
metrics/metrics_test.gometrics/task.go
Closing the channel on the paused gauge (the first per-queue emission)
left the goroutine still running through later emissions while
runSystem returned and pausedCtrl.Finish() validated expectations.
Switch the synchronization to fire on Gauge("jobs.enqueued.count", ...),
the last unconditional mock call in Execute, so the goroutine is fully
done before the test returns. The paused gauge expectation goes back
to a plain Times(1).
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
2 participants
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
faktory.queue.pausedgauge (1 if the queue is paused, 0 otherwise) on the existing 30s metrics task tick. Dashboards and alerts can now distinguish "queue has no work" from "queue is paused and won't drain."store.PausedQueues(ctx)and checked in-memory inside the existingEachQueueloop — one Redis round trip per tick regardless of queue count.<namespace>.*metrics inmetrics/task.goandmetrics/middleware.goas deprecated. They are retained for backward compatibility; new metrics should be added only in the canonicalfaktory.*tagged form.Test plan
go build ./metrics/...cleango vet ./metrics/...cleango test -count=1 ./metrics/...passes (existing happy-path subtest extended withfaktory.queue.paused=0expectations for each queue; new "paused queue is reported as 1" subtest pushes a job, callsPause(ctx), and asserts the gauge fires with value 1)q1,PAUSE q1, and confirmfaktory.queue.paused:1|g|#queue:q1,...appears within 30s;RESUME q1flips it back to 0