feat: expose worker run options and wire Prometheus metrics by default#87
Conversation
core.Run() invoked workers.Run with no RunOptions, so the workers package
silently fell back to BaseMetrics{} (no-op) and every worker_* Prometheus
metric stayed at zero for any service using CBWorkerProvider.
Add AddWorkerRunOptions for init-time configuration and prepend a default
workers.WithMetrics(workers.NewPrometheusMetrics(AppName)) when APP_NAME
is set and Prometheus is not disabled. User-supplied WithMetrics still
wins because workers.WithMetrics overwrites runConfig.metrics on each
apply.
|
Warning Rate limit exceeded
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 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughA new framework-level mechanism is added to register default worker run options via Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Core as (*cb).Run
participant BuildOpts as buildWorkerRunOpts()
participant WorkersRun as workers.Run()
User->>User: AddWorkerRunOptions(opts...) [during init]
Note over User: Registers options in global workerRunOpts slice
User->>Core: Run(ctx)
activate Core
Core->>BuildOpts: buildWorkerRunOpts()
activate BuildOpts
Note over BuildOpts: 1. Prepend Prometheus metrics if enabled<br/>2. Append registered workerRunOpts<br/>(user options override defaults)
BuildOpts-->>Core: []workers.RunOption
deactivate BuildOpts
Core->>WorkersRun: workers.Run(ctx, workerList, opts...)
activate WorkersRun
WorkersRun-->>Core: error or nil
deactivate WorkersRun
deactivate Core
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 3 minutes and 32 seconds.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers_test.go`:
- Around line 132-150: The test can hang on the blocking receive from errCh and
doesn't detect early exit of Run(); change the logic around errCh/started to use
bounded waits: in the startup loop that currently polls rec.started.Load(), also
select on errCh to fail fast if instance.Run() returned early, and replace the
final blocking receive "err := <-errCh" with a select that waits for either err
from errCh or a timeout/context deadline (e.g., 2–5s) and fails the test on
timeout; reference symbols: errCh, instance.Run, instance.Stop, rec.started,
WorkerStarted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 070039dc-1757-4b85-a0f2-670a937bf7c9
📒 Files selected for processing (4)
README.mdcore.goworkers.goworkers_test.go
There was a problem hiding this comment.
Pull request overview
Exposes framework-level worker run configuration in core and ensures worker Prometheus metrics are wired by default (when enabled) so services using CBWorkerProvider emit worker lifecycle metrics without needing per-worker setup.
Changes:
- Added
core.AddWorkerRunOptions(...workers.RunOption)and abuildWorkerRunOpts()helper to assemble run options (including default Prometheus metrics whenAPP_NAMEis set and Prometheus isn’t disabled). - Wired
core.Run()to pass the assembled run options intoworkers.Run(...). - Added unit + end-to-end tests covering default/disabled paths and verifying that
AddWorkerRunOptionsreachesworkers.Run; regeneratedREADME.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
workers.go |
Introduces global run options + default Prometheus metrics wiring logic. |
core.go |
Passes run options into workers.Run(...) during service startup. |
workers_test.go |
Adds tests for default option behavior and end-to-end wiring through core.Run(). |
README.md |
Documents the new AddWorkerRunOptions API (generated). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- harden TestRun_WorkerMetricsWired: bound the startup wait on errCh, bound the post-Stop receive, and guarantee Stop+drain via t.Cleanup so a failing assertion never leaks the Run goroutine - workers.go: spell "behavior" in American English to match the rest of the package; regenerate README.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds TestRun_UserMetricsOverridesDefaultPrometheus and extracts the shared lifecycle scaffolding into runWithRecorder. The new test runs the full Run lifecycle with a unique AppName (so the default NewPrometheusMetrics is prepended) and a caller-supplied workers.WithMetrics(rec); asserting rec.WorkerStarted fires proves the caller-supplied metrics is the effective implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The cleanup's drain previously fell through silently on the 2s timeout, which could leak the Run goroutine and surface as a less actionable goleak error later. Report the timeout via t.Errorf so the failure points directly at the cleanup path.
* docs: document worker metrics defaults and AddWorkerRunOptions Adds a Metrics subsection under ColdBrew Integration explaining that core wires workers.NewPrometheusMetrics(APP_NAME) automatically when APP_NAME is set and DISABLE_PROMETHEUS is not, and how to override via core.AddWorkerRunOptions for custom metrics backends or run-level options. Tracks go-coldbrew/core#87. * docs: address review feedback on metrics subsection - rename "### Metrics" to "### Metrics defaults" so the anchor doesn't collide with the existing "## Metrics" earlier in the document - update the in-text reference to be unambiguous - pass &myDatadogMetrics{} (pointer) to match the pointer-receiver example shown earlier in the same document * docs: document opt-in worker middleware (Recover, Tracing, Slog, LogContext) Add a "Tracing and observability middleware (opt-in)" subsection to the ColdBrew Integration section explaining that core does not wire workers middleware automatically and showing the AddWorkerRunOptions one-liner to enable middleware.DefaultInterceptors() — plus a conservative subset (Recover + LogContext) for fast-tick workers where per-cycle spans/logs would be noisy. * docs: address review feedback on metrics/middleware section - core.Run() -> cb.Run() to match the canonical entrypoint shown elsewhere in the docs (cb := core.New(cfg); cb.Run()) - "behaviour" -> "behavior" for consistency with the rest of the page * docs: reconcile Slog log-lines-per-cycle wording The middleware section earlier in this document describes Slog as emitting two log lines per cycle (start + end/error). The new opt-in section was paraphrasing as "one log line per cycle" — update to match.
Summary
core.Run()was invokingworkers.Runwith noRunOptions, soworkersfell back toBaseMetrics{}(no-op) and everyworker_started_total,worker_panicked_total,worker_active_count, etc. silently stayed at zero in any service usingCBWorkerProvider.core.AddWorkerRunOptions(opts ...workers.RunOption)for init-time configuration of run-level options (metrics, default jitter, run-level interceptors).workers.WithMetrics(workers.NewPrometheusMetrics(AppName))by default whenAPP_NAMEis set and Prometheus is not disabled. A user-suppliedWithMetricsoverrides the default becauseworkers.WithMetricsoverwritesrunConfig.metricson each apply.APP_NAMEskips the default to avoid ambiguous unprefixed metric names; users can still opt in by callingAddWorkerRunOptions(workers.WithMetrics(workers.NewPrometheusMetrics("")))explicitly.Why run-level (not
Worker.WithMetrics)Worker.WithMetricsexists, but a framework default belongs at run level — otherwise every app adoptingCBWorkerProviderhas to remember to apply metrics on every worker. Per-worker overrides still work for callers that want to.Test plan
make buildmake test(race + cover, 80.4%)make lint(0 issues, govulncheck clean)workers_test.gocovers default Prometheus path,DisablePrometheus/ deprecatedDisablePormetheus/ emptyAppNameskip paths, multi-add append, and an end-to-endcore.Run()test assertingWorkerStartedfires on aworkers.Metricsrecorder injected viaAddWorkerRunOptions.make docregeneratedREADME.md.docs.coldbrew.cloud/howto/workers.md— added a "Metrics" subsection under "ColdBrew Integration" (separate submodule; will land in the docs repo).Manual smoke
Build a service with a
CBWorkerProvider, setAPP_NAME=demo, scrape/metrics, confirmdemo_worker_started_totalis present and non-zero.Summary by CodeRabbit
New Features
Documentation
Tests