Combined cloudwatch fixes - #6
Draft
erimicel wants to merge 13 commits into
Draft
Conversation
Lets callers opt in to a subset of metrics rather than always publishing
all of them. The publisher skips upstream Sidekiq calls it doesn't need:
no `Sidekiq::Stats.new` if no global metric is requested, no `ProcessSet`
enumeration if no process/tag/aggregate metric is requested, and no
queue iteration if neither `:queue_size` nor `:queue_latency` is asked for.
This unlocks running multiple publishers on different cadences — e.g. a
slow full-fat publisher for the dashboard plus a fast queue-only
publisher feeding a burst-scaling alarm:
Sidekiq::CloudWatchMetrics.enable!(metrics: %i[queue_size queue_latency], interval: 10)
The legacy `process_metrics:` boolean is kept as a deprecated alias.
Also bumps the gem to 2.10.0.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Publish JobExecutionTimeP50/P95/P99 to CloudWatch, dimensioned by JobClass, by reading the execution histograms Sidekiq 7+ already records in Redis via its built-in Sidekiq::Metrics::ExecutionTracker middleware. No new per-job instrumentation is needed. Each tick the publisher queries the previous full minute (so the ExecutionTracker's heartbeat flush isn't racing the read), looks up which classes had activity via Sidekiq::Metrics::Query#top_jobs, then fetches one histogram per class and computes the requested percentiles from the bucket counts. Resolution is whichever Sidekiq histogram bucket the percentile falls into; the "Slow" bucket is clamped to the previous bucket's upper bound so the published value stays plottable. Silently skipped on Sidekiq versions older than 7 (no Sidekiq::Metrics). Prompt: > but can't we add it to our own fork this feature? if it is sidekiq anyway
On OSS Sidekiq the publisher runs on every node by default — CloudWatch deduplicates datapoints to the same metric per second so dashboards stay correct, but you pay N× the put_metric_data API calls and the SampleCount of each datapoint becomes N instead of 1. At 20 instances + ~50 active job classes this is meaningful overhead. Introduces an opt-in `leader_election: :redis` option to elect a single publisher cluster-wide via a Redis `SET key value NX EX` lock. Per-tick election (re-checked inside #publish) gives automatic failover within one interval if the holder dies; the lock TTL defaults to 3× the publish interval. Clean shutdown releases the lock so failover is instantaneous. The lock key is scoped by namespace so multiple deployments sharing a Redis don't fight. Callers may also pass any object that quacks like a leader (#acquire_or_extend, #release) for alternate backends or tests. Should be used on the standard cadence publisher, not on a fast burst publisher (e.g. 10s queue_size/queue_latency for auto-scaling) — there every node should keep refreshing values even if the leader hangs. Default behaviour is unchanged when the option is omitted. Prompt: > do it please directly and commit [the Redis leader change]
build_leader(:redis) previously constructed the lock key from only the namespace, so two publishers on the same namespace with different intervals (e.g. a 60s standard publisher and a 10s burst publisher) contended on the same Redis key. The first to call enable! acquires the key with its own UUID; the other publisher's acquire_or_extend sees a different UUID on every tick and returns false, silently suppressing it for the lifetime of the process. Adding the interval to the key (`...:60s`, `...:10s`) gives each publisher its own lock so the two intended publishers run side by side. Rolling upgrade behaviour: the new key differs from the old one, so a node running the new code acquires its new key independently and old nodes keep holding the old key until they restart. During the rolling restart there's at most one extra publisher per role for ≤ TTL of the old lock. After all nodes have rolled, the stale old key expires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every distinct JobClass dimension value on job_execution_time_p* is a separate billable CloudWatch custom metric. Long-tail apps with hundreds of job classes pay for noise — most classes barely fire and the percentile signal isn't actionable per-class anyway. Add `max_job_classes:` (default 20). Above the cap the publisher keeps the busiest classes by total sample count and combines the rest bucket-by-bucket into a single JobClass=Other histogram, so operators still see overall long-tail latency without paying per-class for the long tail. `max_job_classes: nil` restores the previous unlimited behaviour for anyone who wants it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If a Sidekiq job class is literally named `Other` and lands outside the top `max_job_classes`, the previous label would silently overwrite that class's individual metrics with the aggregated tail rollup. Parentheses aren't valid in a Ruby constant identifier so `(other)` is structurally unable to collide with a real class name. Prompt: fix the OTHER_JOB_CLASS collision the PR review flagged
`sort_by { |_klass, buckets| -buckets.sum }` is not stable, so two job
classes with identical sample counts within a 60s window could swap
positions relative to the `max_job_classes` boundary from one publish
cycle to the next. Borderline classes would then appear and disappear
intermittently in CloudWatch. Use the class name as a secondary sort
key to break ties deterministically.
Prompt: fix the sort instability the PR review flagged
`Array#transpose` raises `IndexError: element size differs` if any tail histogram has a different bucket count from the others. Sidekiq's internal histogram is fixed-width (26 buckets) today so the original code is safe, but the assumption is implicit — a future Sidekiq upgrade that changes the bucket count or a class that ever returns a shorter array would crash the publisher and stop emitting metrics entirely. Replace `transpose.map(&:sum)` with an explicit element-wise sum that pads short arrays with zeros up to the longest width, so the rollup survives uneven inputs without losing samples. Prompt: fix the transpose-crash assumption the PR review flagged
The top-N + (other) cap hid slow tail jobs behind an aggregate — the (other) series ended up with the highest P99 in the account because it collected every rare-but-slow job, while operators lost the ability to identify which class was causing it. Switch to a duration filter: a class only publishes its per-class percentile series if at least one execution in the last minute landed in a histogram bucket whose upper bound exceeds @min_job_seconds. Default 0.5s. Fast jobs are dropped entirely; their throughput is still covered by the global ProcessedJobs / Workers metrics. Net effect: same ~$100/mo savings as the cap (probably more, since most Olio jobs complete in <100ms), but the surviving series are the ones operators actually care about — and they keep their real class name instead of disappearing into (other). Pass `min_job_seconds: nil` (or 0) to restore the previous publish- every-class behaviour, or any positive value to tune the threshold. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-class JobExecutionTimeP50/P95/P99 were derived from Sidekiq's built-in execution histogram, whose top finite bucket is 335s and whose final bucket is "≥335s". Any job slower than that was published as exactly 335, so the CloudWatch dashboard's job-execution-time widgets flat-lined at 335s and lost all resolution for genuinely slow jobs. Record real durations instead: - Add ExecutionRecorder server middleware that times each job with a monotonic clock and, for executions at/above min_job_seconds, appends the duration (ms) to a short-lived per-(namespace, UTC minute, class) Redis list (capped via LTRIM, TTL'd as a backstop). - Add ExecutionSamples to record/drain those lists. - The publisher drains the previous full minute and computes nearest-rank percentiles in true seconds — no ceiling: a 600s job now reports 600.0. - enable! installs the middleware only when the publisher owns an execution -time metric (guarded against double-add across multiple enable! calls). Because recording no longer relies on Sidekiq::Metrics, the feature now works on every supported Sidekiq version (5–8), not just 7+. Metric names, dimensions and the min_job_seconds threshold (the single seconds knob, default 0.5) are unchanged, so the existing dashboard needs no edits. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Report true execution-time percentiles (remove 335s ceiling)
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.
No description provided.