Repository navigation
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable opslog bucket SLI collector with stale-series reaping, command/config wiring, tests, and docs; introduces a pg-probe producer that discovers and probes RGW index PGs with Prometheus/NATS output; and adds two shell scripts for PG coverage checks and PG risk ranking. ChangesBucket SLO Collector
PG Probe Producer and Index-PG Tools
Sequence Diagram(s)sequenceDiagram
participant CLI
participant PGProbe
participant RADOS
participant Prometheus
participant NATS
CLI->>PGProbe: StartMonitoring(cfg)
PGProbe->>RADOS: discoverProbeTargets()
loop each probe interval
PGProbe->>RADOS: stat representative shard per PG
RADOS-->>PGProbe: ProbeResult list
PGProbe->>Prometheus: PublishToPrometheus(results)
PGProbe->>NATS: PublishToNATS(results)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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. 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: 3
🧹 Nitpick comments (2)
pkg/producers/opslog/metrics_test.go (2)
388-388: ⚡ Quick winAvoid fixed 5ms sleeps for threshold-based assertions.
These sleeps are close to the 1ms cutoffs and can become flaky under CI scheduling jitter. Prefer
assert.Eventually(or a longer bounded polling window) for deterministic timing tests.Also applies to: 443-443, 470-470
🤖 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 `@pkg/producers/opslog/metrics_test.go` at line 388, Replace the brittle time.Sleep(5 * time.Millisecond) calls in metrics_test.go with a bounded polling assertion (e.g., use testify's assert.Eventually or a small retry loop) that checks the expected condition within a timeout (e.g., 50-200ms) and a short poll interval (e.g., 1-5ms); locate the three occurrences of time.Sleep(5 * time.Millisecond) in the test file and change each to assert.Eventually(t, func() bool { /* check the metric/state you expect after the sleep */ }, time.Millisecond*200, time.Millisecond*5) or an equivalent loop to avoid CI scheduling flakiness.
322-326: ⚡ Quick winIsolate
globalSLICollectorper test with cleanup.These tests mutate package-global state but don’t restore it. Please snapshot and reset via
t.Cleanupto avoid cross-test coupling.Suggested pattern
+prev := globalSLICollector +t.Cleanup(func() { globalSLICollector = prev }) globalSLICollector = newSLICollector(SLICollectorConfig{ StaleTTL: 24 * time.Hour, ColdAfter: 5 * time.Minute, SkipEveryN: 1, })Also applies to: 351-355, 382-382, 410-410, 429-429, 459-459
🤖 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 `@pkg/producers/opslog/metrics_test.go` around lines 322 - 326, Tests mutate the package-global variable globalSLICollector when calling newSLICollector(SLICollectorConfig{...}); to prevent cross-test coupling, in each test that assigns globalSLICollector snapshot the previous value (e.g., old := globalSLICollector), assign the new collector, and register a t.Cleanup(func(){ globalSLICollector = old }) to restore it after the test; apply this pattern to every test that changes globalSLICollector (including the other occurrences where newSLICollector is used) and ensure nil/zero-value restoration works correctly.
🤖 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 `@pkg/producers/opslog/prometheus_sli_test.go`:
- Line 13: The functions collectToDTO and findHistogramByLabels in sliCollector
are unused and failing golangci-lint; either remove both helper functions (and
any related unused imports or references) or add unit tests that call them
(e.g., in prometheus_sli_test.go create tests that instantiate sliCollector,
populate it with sample metrics, call collectToDTO and findHistogramByLabels,
and assert expected DTO/histogram responses) so they are exercised; pick one
approach and apply it consistently to both symbols to clear the lint error.
In `@pkg/producers/opslog/prometheus_sli.go`:
- Around line 328-333: The startup log prints the raw cfg but newSLICollector
may normalize SkipEveryN and ReapInterval, causing misleading diagnostics; after
constructing the collector (via newSLICollector) log the effective/normalized
values (e.g., use the collector's cfg or its fields such as StaleTTL, ColdAfter,
SkipEveryN, ReapInterval) instead of the original cfg so the "SLI collector
registered with tiered emission" message reflects the normalized settings;
update the log to read values from the returned collector (or its normalized
config) and move the log call to after newSLICollector returns.
In `@pkg/producers/opslog/README.md`:
- Line 259: The README line documenting BUCKET_SLO_REAP_INTERVAL uses the
inconsistent default expression `stale_ttl/4`; update that default to use the
flag-style name `stale-ttl/4` to match the rest of the docs and avoid operator
confusion (reference symbol: BUCKET_SLO_REAP_INTERVAL and the setting name
stale-ttl).
---
Nitpick comments:
In `@pkg/producers/opslog/metrics_test.go`:
- Line 388: Replace the brittle time.Sleep(5 * time.Millisecond) calls in
metrics_test.go with a bounded polling assertion (e.g., use testify's
assert.Eventually or a small retry loop) that checks the expected condition
within a timeout (e.g., 50-200ms) and a short poll interval (e.g., 1-5ms);
locate the three occurrences of time.Sleep(5 * time.Millisecond) in the test
file and change each to assert.Eventually(t, func() bool { /* check the
metric/state you expect after the sleep */ }, time.Millisecond*200,
time.Millisecond*5) or an equivalent loop to avoid CI scheduling flakiness.
- Around line 322-326: Tests mutate the package-global variable
globalSLICollector when calling newSLICollector(SLICollectorConfig{...}); to
prevent cross-test coupling, in each test that assigns globalSLICollector
snapshot the previous value (e.g., old := globalSLICollector), assign the new
collector, and register a t.Cleanup(func(){ globalSLICollector = old }) to
restore it after the test; apply this pattern to every test that changes
globalSLICollector (including the other occurrences where newSLICollector is
used) and ensure nil/zero-value restoration works correctly.
🪄 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: 677af566-60dd-4973-9349-96e2e3ddb956
📒 Files selected for processing (9)
pkg/commands/ctl.gopkg/commands/producer_ops_log.gopkg/producers/opslog/README.mdpkg/producers/opslog/config.gopkg/producers/opslog/metrics_test.gopkg/producers/opslog/prometheus.gopkg/producers/opslog/prometheus_sli.gopkg/producers/opslog/prometheus_sli_test.gopkg/producers/opslog/sli.go
|
Actionable comments posted: 0 |
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 `@scripts/check-bucket-index-pg-coverage-v4.sh`:
- Around line 106-114: The PGID lists are being written with sort -V which can
produce order incompatible with comm's expected collating order; update the
writes for POOL_PGS and USED_PGS to force a C locale so comm sees consistent
byte-collation (e.g., prefix the sort invocation with LC_ALL=C: LC_ALL=C sort -V
> "$POOL_PGS" and LC_ALL=C sort -V > "$USED_PGS"), and likewise ensure any comm
invocations run under LC_ALL=C; apply the same change to the second occurrence
at lines 150-153 referenced in the review.
In `@scripts/list-index-pg-risk.sh`:
- Around line 61-94: The per-object bucket name lookup currently runs awk
against BUCKET_ID_NAME_MAP for every object (in the loop using
bucket_name="$(awk -F '\t' -v id="$bucket_instance_id" ...
"$BUCKET_ID_NAME_MAP")"), which is inefficient; preload BUCKET_ID_NAME_MAP once
into a shell associative array (or simple map) before the while-read loop and
then replace the awk call with a direct array lookup (e.g.,
bucket_name="${BUCKET_MAP[$bucket_instance_id]:-}" or set to "<unknown>" if
empty). Initialize the map by reading BUCKET_ID_NAME_MAP (splitting on tab) and
populating BUCKET_MAP, then use bucket_name lookup inside the loop and remove
the per-iteration awk invocation.
🪄 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: dbbd75ea-5e9a-4dd9-bb8b-b695dc3042af
📒 Files selected for processing (2)
scripts/check-bucket-index-pg-coverage-v4.shscripts/list-index-pg-risk.sh
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/producers/opslog/prometheus.go (1)
120-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-positive
bucket_slo_stale_ttlvalues.
time.ParseDurationaccepts0sand negative durations, and this block stores them as-is. That breaks the collector:pkg/producers/opslog/prometheus_sli.gotreats anything older thanStaleTTLas stale, so0s/negative TTLs make bucket SLI series disappear from/metricsalmost immediately even thoughTrackBucketSLOis enabled. Please keep the 24h default ford <= 0and warn instead.Suggested patch
if mc.BucketSLOStaleTTL != "" { if d, err := time.ParseDuration(mc.BucketSLOStaleTTL); err == nil { - cfg.StaleTTL = d + if d > 0 { + cfg.StaleTTL = d + } else { + log.Warn(). + Str("value", mc.BucketSLOStaleTTL). + Msg("bucket_slo_stale_ttl must be > 0, using default 24h") + } } else { log.Warn().Str("value", mc.BucketSLOStaleTTL).Msg("invalid bucket_slo_stale_ttl, using default 24h") } }🤖 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 `@pkg/producers/opslog/prometheus.go` around lines 120 - 125, The parsed BucketSLOStaleTTL may be zero or negative which breaks downstream logic; in the config handling where you parse mc.BucketSLOStaleTTL (time.ParseDuration) and set cfg.StaleTTL, reject non-positive durations by checking parsed d > 0 before assigning; if d <= 0 or parse fails, keep the existing 24h default and emit a warning (use the same log.Warn() path) indicating the invalid non-positive value so callers know the default is used.
🤖 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 `@changes_summary.md`:
- Line 16: The wording claims the pigeonhole principle guarantees full PG
coverage with the "probe bucket" (997 shards) across 128 PGs; change the
rationale text to say coverage was "empirically verified" (or similar) instead
of invoking the pigeonhole principle, and explicitly retain the verification
step that probes all Placement Groups in the `.rgw.buckets.index` pool(s) every
15 seconds using the pre-sharded probe bucket and lightweight `rados stat`
checks; update any phrasing that references "pigeonhole" to reference empirical
verification and keep the explicit verification sentence intact.
- Around line 31-51: The markdown contains unlabeled fenced code blocks; update
each triple-backtick fence that wraps the "prysm local-producer pg-probe"
diagram and the "Tier 1: PG Probe → RADOS-level availability..." list to include
a language identifier (e.g., text or bash) so lint MD040 is satisfied. Locate
the blocks by searching for the exact block contents ("prysm local-producer
pg-probe" diagram and the "Tier 1: PG Probe..." block) and prepend the opening
``` with an appropriate identifier such as ```text for both occurrences.
In `@pkg/producers/pgprobe/mapping.go`:
- Around line 46-100: ListBuckets returns all RGW buckets so the loop in the PG
mapping code (using ListBuckets, getBucketMetadata, getObjectPG and populating
BucketPGMapping / mapping.PGToBuckets) can attach buckets from other pools; fix
by filtering buckets to the current index pool before adding them: after calling
getBucketMetadata(info) check the bucket's placement/zone/pool metadata (or
validate shard object existence in the target ioctx) and skip buckets whose
placement does not match the provided pool or whose shard objects do not exist
in that pool, then only proceed to resolve shards with getObjectPG and update
mapping.Buckets, mapping.BucketToPGs and mapping.PGToBuckets for buckets that
pass this validation.
In `@pkg/producers/pgprobe/pgprobe.go`:
- Around line 61-69: After normalizing zero defaults for cfg.Interval,
cfg.MappingRefreshInterval, and cfg.ProbeTimeoutMs, add explicit validation to
reject negative values (e.g., if cfg.Interval < 0 or cfg.MappingRefreshInterval
< 0 or cfg.ProbeTimeoutMs < 0) and return a clear error (or fail fast) instead
of allowing those values to reach time.NewTicker in the probe loop setup; apply
the same validation in the other initialization block referenced (lines 132-137)
so both probe creation paths validate and fail with a descriptive message before
starting timers.
- Around line 242-287: The discoverProbeTargets function currently selects the
probe bucket by choosing the marker with the most .dir.* shard objects; instead,
consult the configured cfg.ProbeBucket and use it to pick the correct marker.
Modify discoverProbeTargets to accept (or access) the configured probe bucket
name (cfg.ProbeBucket), map that bucket name to its marker via
groupShardsByBucket result (or derive the marker from shard object names), and
set probeBucketMarker and probeBucketObjList based on that configured marker; if
the configured bucket is missing or has no shards, return a clear error. Ensure
references to discoverProbeTargets, cfg.ProbeBucket, groupShardsByBucket,
probeBucketMarker, and probeBucketObjList are updated so discovery explicitly
resolves the configured probe bucket instead of using the largest-shard
heuristic.
- Around line 331-341: runProbes currently calls ioctx.Stat(objName) with no
timeout; wire cfg.ProbeTimeoutMs into the probe path by creating a
context.WithTimeout using time.Duration(cfg.ProbeTimeoutMs)*time.Millisecond for
each probe, then perform the Stat call in a cancellable way (e.g., run
ioctx.Stat(objName) in a goroutine and select on ctx.Done() to detect timeouts
or use any available context-aware Stat API if present), convert ctx
timeout/error into a ProbeResult error/latency, and ensure you call cancel() to
free resources; update runProbes to reference cfg.ProbeTimeoutMs and treat
context deadline exceeded as a probe timeout.
---
Outside diff comments:
In `@pkg/producers/opslog/prometheus.go`:
- Around line 120-125: The parsed BucketSLOStaleTTL may be zero or negative
which breaks downstream logic; in the config handling where you parse
mc.BucketSLOStaleTTL (time.ParseDuration) and set cfg.StaleTTL, reject
non-positive durations by checking parsed d > 0 before assigning; if d <= 0 or
parse fails, keep the existing 24h default and emit a warning (use the same
log.Warn() path) indicating the invalid non-positive value so callers know the
default is used.
🪄 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: 675e3e44-91c9-4075-b275-07c8366359e6
📒 Files selected for processing (16)
changes_summary.mdpkg/commands/local_producer.gopkg/commands/producer_ops_log.gopkg/commands/producer_pg_probe.gopkg/producers/config/producer.gopkg/producers/opslog/config.gopkg/producers/opslog/metrics_test.gopkg/producers/opslog/prometheus.gopkg/producers/opslog/prometheus_sli.gopkg/producers/opslog/prometheus_sli_test.gopkg/producers/opslog/sli.gopkg/producers/pgprobe/config.gopkg/producers/pgprobe/mapping.gopkg/producers/pgprobe/nats.gopkg/producers/pgprobe/pgprobe.gopkg/producers/pgprobe/prometheus.go
💤 Files with no reviewable changes (2)
- pkg/producers/opslog/prometheus_sli_test.go
- pkg/commands/producer_ops_log.go
✅ Files skipped from review due to trivial changes (1)
- pkg/commands/local_producer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/producers/opslog/config.go
- pkg/producers/opslog/sli.go
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/producers/pgprobe/mapping.go (1)
1-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGate
pkg/producers/pgprobe/mapping.go(andpkg/producers/pgprobe/pgprobe.go) behind//go:build ceph.
mapping.goimportshub.lumenfield.work/ceph/go-ceph/rados/rgw/adminbut has no//go:build cephheader (same forpgprobe.go), unlikepkg/producers/config/producer_pgprobe.go, so it will compile in non-Ceph builds.
- Major:
BucketPGMapping.BucketToPGs/PGToBucketsare keyed bybucket_nameonly, discardingTenant/BucketIDeven thoughBucketInfoincludes them, so collisions can’t be disambiguated.Suggested fix
+//go:build ceph + // SPDX-FileCopyrightText: 2025 SAP SE or an SAP affiliate company and prysm contributors // // SPDX-License-Identifier: Apache-2.0🤖 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 `@pkg/producers/pgprobe/mapping.go` around lines 1 - 15, Add a Go build tag to gate Ceph-specific files and fix the map keying collision: add the `//go:build ceph` (and optional `// +build ceph`) header to both pkg/producers/pgprobe/mapping.go and pkg/producers/pgprobe/pgprobe.go so Ceph-only imports (github.com/ceph/go-ceph/...) are excluded from non-Ceph builds; then update BucketPGMapping.BucketToPGs and BucketPGMapping.PGToBuckets to use a disambiguating composite key (e.g., include Tenant and/or BucketID from BucketInfo) instead of just bucket_name so entries are unique — modify where BucketInfo is created/used and any lookup code in methods on BucketPGMapping to construct and parse the composite key consistently.pkg/producers/pgprobe/nats.go (1)
54-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPopulate
covered_pgsexplicitly and stop overloadingtotal_pgs.
CoveredPGsis never assigned here, so every NATS event publishescovered_pgs: 0. At the same time,TotalPGsis derived fromlen(results), which is just the probed set available to this function. That makes the payload misleading for downstream consumers that distinguish pool size from probe coverage. At minimum setCoveredPGstolen(results); iftotal_pgsis intended to mean the pool-wide total, that count needs to be passed intoPublishToNATSfrom the caller rather than inferred here.🤖 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 `@pkg/producers/pgprobe/nats.go` around lines 54 - 67, The ProbeEvent being published never sets CoveredPGs and misuses TotalPGs = len(results), so set CoveredPGs explicitly and stop overloading TotalPGs: either (quick fix) assign CoveredPGs = len(results) in the ProbeEvent construction (keep TotalPGs as-is if it intentionally represents probed count), or (correct fix) change the PublishToNATS/caller contract to accept a poolTotalCount parameter, set ProbeEvent.TotalPGs = poolTotalCount and ProbeEvent.CoveredPGs = len(results); update all call sites of PublishToNATS accordingly.changes_summary.md (1)
157-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate this section to match the collector actually introduced in this PR.
This changelog says the opslog collector was simplified to two tiers and that
--bucket-slo-cold-after/--bucket-slo-skip-every-nwere removed, but the PR objective for this stack adds a hot/cold/stale collector and those exact settings. As written, operators would be told to remove live config and would miss the cold-series behavior entirely.🤖 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 `@changes_summary.md` around lines 157 - 189, The changelog section is incorrect for the collector introduced in this PR; update changes_summary.md so it accurately reflects the implemented hot/cold/stale collector (not the simplified 2-tier active/stale), restore and document the removed CLI flags and env vars that actually exist in the PR (--bucket-slo-cold-after, --bucket-slo-skip-every-n / BUCKET_SLO_COLD_AFTER, BUCKET_SLO_SKIP_EVERY_N), mention the hot/cold/stale behavior and cold-series emission logic (ColdAfter, SkipEveryN, emitCold and any related config fields like BucketSLOColdAfter/BucketSLOSkipEveryN), and ensure the retained sections (counter, reaper, flags) match the real code paths and defaults in the PR so operators do not remove live config by mistake.
♻️ Duplicate comments (1)
changes_summary.md (1)
16-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the PG coverage rationale.
“997 shards over 128 PGs” does not prove every PG is covered; it only guarantees collisions. Please reword this to empirical coverage and keep the explicit verification step below.
🤖 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 `@changes_summary.md` at line 16, Update the statement about PG coverage so it does not claim a mathematical guarantee: replace the phrase asserting that "a single pre-sharded 'probe bucket' (997 shards) covers all 128 PGs via the pigeonhole principle" with wording that describes observed/empirical coverage (e.g., "empirically covers most PGs") and explicitly note this is not a proof; keep the subsequent explicit verification step that probes all Placement Groups in the `.rgw.buckets.index` pool(s) every 15 seconds and retains the specific implementation detail about using a pre-sharded probe bucket (997 shards) and lightweight `rados stat` calls against 128 PGs for SLI derivation.
🤖 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 `@changes_summary.md`:
- Around line 313-330: The fenced ASCII diagram blocks (e.g., the block starting
with "┌─────────────────────────────────────────────────────────────┐" and
containing "builder-base (golang:1.26-alpine)" and the similar block later
around "builder-noceph / builder-ceph") are using bare triple backticks; add a
language identifier "text" to those fenced code blocks (and the other similar
block referenced at the later location) so they become ```text ... ``` to
satisfy markdownlint MD040.
In `@Dockerfile`:
- Around line 17-18: The Dockerfile currently uses floating Alpine tags (e.g.,
FROM golang:1.26-alpine AS builder-base) which can resolve to Alpine releases
that lack ceph19-dev / librados19; pin the Alpine release used by the builder
and every runtime stage (including the runtime-ceph stage) to a known working
release (e.g., replace "alpine" with "alpine3.22" or the specific v3.21/v3.22
tag you validated) so apk package resolution is deterministic; update all FROM
lines that reference "*-alpine" or "alpine" to the chosen pinned tag and ensure
multi-stage names like builder-base and runtime-ceph keep the same aliases.
- Around line 62-76: The Alpine runtime stages are missing the root CA bundle;
update both apk installs so they include ca-certificates: add ca-certificates to
the first RUN apk add --no-cache smartmontools nvme-cli line and to the RUN apk
add --no-cache \ librados19 \ smartmontools \ nvme-cli block (the
runtime/no-ceph and runtime-ceph APK installs referenced in the Dockerfile) so
the container can validate TLS certificates for admin-url, NATS, and remote
producers.
In `@pkg/producers/config/producer_pgprobe.go`:
- Around line 31-36: Validate and clamp the probe timing settings to a safe
positive value before they are used to create tickers: ensure values read via
GetIntSetting (interval and mapping_refresh_interval from producer.Settings) are
>0 (or default to 15 and 3600 respectively) and reject or clamp negatives/zero;
adjust either where interval/mappingRefresh are assigned or immediately in
StartMonitoring before using cfg.Interval and cfg.MappingRefreshInterval with
time.NewTicker to prevent panics from negative durations.
In `@pkg/producers/pgprobe/mapping.go`:
- Around line 28-31: BucketPGMapping currently keys BucketToPGs and PGToBuckets
by bare bucket name which loses Tenant information from BucketInfo and causes
silent collisions; change the map keys to a stable, unique bucket identifier
(e.g., composite key using Tenant + ":" + BucketID or a dedicated BucketKey
string/type) everywhere referenced (BucketPGMapping.BucketToPGs,
BucketPGMapping.PGToBuckets and the code that populates/reads them) and update
all consumers to construct/parse that composite key (or switch to using the
BucketKey type) so identity includes Tenant and BucketID and no longer collides
for buckets with the same visible name.
---
Outside diff comments:
In `@changes_summary.md`:
- Around line 157-189: The changelog section is incorrect for the collector
introduced in this PR; update changes_summary.md so it accurately reflects the
implemented hot/cold/stale collector (not the simplified 2-tier active/stale),
restore and document the removed CLI flags and env vars that actually exist in
the PR (--bucket-slo-cold-after, --bucket-slo-skip-every-n /
BUCKET_SLO_COLD_AFTER, BUCKET_SLO_SKIP_EVERY_N), mention the hot/cold/stale
behavior and cold-series emission logic (ColdAfter, SkipEveryN, emitCold and any
related config fields like BucketSLOColdAfter/BucketSLOSkipEveryN), and ensure
the retained sections (counter, reaper, flags) match the real code paths and
defaults in the PR so operators do not remove live config by mistake.
In `@pkg/producers/pgprobe/mapping.go`:
- Around line 1-15: Add a Go build tag to gate Ceph-specific files and fix the
map keying collision: add the `//go:build ceph` (and optional `// +build ceph`)
header to both pkg/producers/pgprobe/mapping.go and
pkg/producers/pgprobe/pgprobe.go so Ceph-only imports
(github.com/ceph/go-ceph/...) are excluded from non-Ceph builds; then update
BucketPGMapping.BucketToPGs and BucketPGMapping.PGToBuckets to use a
disambiguating composite key (e.g., include Tenant and/or BucketID from
BucketInfo) instead of just bucket_name so entries are unique — modify where
BucketInfo is created/used and any lookup code in methods on BucketPGMapping to
construct and parse the composite key consistently.
In `@pkg/producers/pgprobe/nats.go`:
- Around line 54-67: The ProbeEvent being published never sets CoveredPGs and
misuses TotalPGs = len(results), so set CoveredPGs explicitly and stop
overloading TotalPGs: either (quick fix) assign CoveredPGs = len(results) in the
ProbeEvent construction (keep TotalPGs as-is if it intentionally represents
probed count), or (correct fix) change the PublishToNATS/caller contract to
accept a poolTotalCount parameter, set ProbeEvent.TotalPGs = poolTotalCount and
ProbeEvent.CoveredPGs = len(results); update all call sites of PublishToNATS
accordingly.
---
Duplicate comments:
In `@changes_summary.md`:
- Line 16: Update the statement about PG coverage so it does not claim a
mathematical guarantee: replace the phrase asserting that "a single pre-sharded
'probe bucket' (997 shards) covers all 128 PGs via the pigeonhole principle"
with wording that describes observed/empirical coverage (e.g., "empirically
covers most PGs") and explicitly note this is not a proof; keep the subsequent
explicit verification step that probes all Placement Groups in the
`.rgw.buckets.index` pool(s) every 15 seconds and retains the specific
implementation detail about using a pre-sharded probe bucket (997 shards) and
lightweight `rados stat` calls against 128 PGs for SLI derivation.
🪄 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: 70bf288f-2205-4f27-8e6b-42426581fadb
📒 Files selected for processing (6)
Dockerfilechanges_summary.mdpkg/producers/config/producer.gopkg/producers/config/producer_pgprobe.gopkg/producers/pgprobe/mapping.gopkg/producers/pgprobe/nats.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/producers/config/producer.go
jrse
changed the title
Collaborator
|
Hi Jan, I want to be upfront before requesting changes: after checking what AWS, GCP, Azure, and the Google SRE Workbook actually do, I think we should rethink the framing. None of them derive per-bucket availability from storage-layer health, and your own Swift-vs-S3 example shows why — the metric can't catch RGW or protocol-handler bugs. What we want to achieve: a customer-facing SLI that reflects what customers actually experience (HTTP success rate per tenant), backed by infrastructure leading indicators that page us before the SLI drops. How to get there from your PR(my suggestions):
analysis-industry-comparison.md |
Collaborator
Author
Hi Senol, thanks for feedback. After review i agree, using per bucket metrics for customer is not something we should do, it is much better to use the customer / tenant based approach and move the pg probe to the L3. I changed the following. Aggregated per-tenant (no bucket label)
Stale series reaped (no traffic → no SLI contribution, matching AWS idle handling) 1 - (sum by (tenant)(rate(prysm_rgw_request_total{status_class="5xx"}[5m])) The PG probe is retained as a pure L3 signal: prysm_rgw_index_probe_success{pgid, pool} — per-PG health These detect storage-layer degradation before it manifests as HTTP 5xx. No "SLI" in the naming, no per-bucket gauges, no conflation with the customer-facing metric. i need to check, how to do this one :-) > 3. Add a small blackbox-exporter canary per protocol per region (~160 series) — this is the piece that catches the Swift case. |
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
June 11, 2026 09:06
054f8bb to
67d92c1
Compare
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
2 times, most recently
from
June 17, 2026 09:30
ce82c54 to
ba0e7d5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors RGW SLI/SLO monitoring to (1) produce per-tenant request/latency SLIs from ops logs and (2) introduce a new Ceph build-tag–gated PG probe producer for RGW index-pool infrastructure health (L3 leading indicator), plus packaging/docs updates to support running and releasing both components.
Changes:
- Replaces prior bucket-focused SLI tracking with a per-tenant SLI Prometheus collector (
radosgw_request_total,radosgw_request_duration_seconds) including stale-series reaping and newregionlabeling. - Adds a new
pg-probelocal producer + config-wired producer (pg_probe) that probes representative RGW index shard objects per PG and exports pool/PG health metrics. - Updates CLI flags/env vars, docs/manifests, Docker multi-stage builds (with/without Ceph), and the release workflow to publish a Ceph-enabled image variant.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/producers/pgprobe/prometheus.go | Defines and publishes PG probe Prometheus metrics and starts a metrics HTTP server. |
| pkg/producers/pgprobe/pgprobe.go | Implements Ceph/RADOS connection, target discovery, and per-PG probing loop. |
| pkg/producers/pgprobe/config.go | Introduces configuration struct for the pg-probe producer. |
| pkg/producers/opslog/sli.go | Refactors operation classification to ADR-defined per-tenant SLI operation/protocol categories. |
| pkg/producers/opslog/README.md | Updates ops-log producer documentation for new SLI metrics and stale-series reaping flags/env vars. |
| pkg/producers/opslog/prometheus.go | Wires SLI collector config (region + TTL/reap settings) into Prometheus initialization. |
| pkg/producers/opslog/prometheus_total_requests.go | Fixes parsing of per-bucket request counter key fields. |
| pkg/producers/opslog/prometheus_total_requests_test.go | Adds regression test for per-bucket counter label parsing. |
| pkg/producers/opslog/prometheus_sli.go | Implements custom Prometheus collector for per-tenant SLI metrics with background stale-series reaper. |
| pkg/producers/opslog/prometheus_sli_test.go | Adds test helpers for collecting and locating emitted SLI metrics. |
| pkg/producers/opslog/metrics.go | Routes TrackBucketSLO path to the new per-tenant SLI observer. |
| pkg/producers/opslog/metrics_test.go | Updates SLI classification/protocol tests and adjusts SLI metric update tests. |
| pkg/producers/opslog/config.go | Adds Region and SLI collector tuning fields to ops-log config structures. |
| pkg/producers/config/producer.go | Adds a registry mechanism for build-tag–gated producer handlers. |
| pkg/producers/config/producer_pgprobe.go | Registers and starts the pg_probe producer from YAML/global config (ceph build tag). |
| pkg/commands/producer_pg_probe.go | Adds prysm local-producer pg-probe CLI command and env/flag configuration. |
| pkg/commands/producer_ops_log.go | Adds --region and SLI stale/reap tuning flags/env merging for ops-log CLI. |
| ops-log-k8s-mutating-wh/README.md | Updates webhook deployment examples to new env var names/sets (incl. SLI tracking). |
| ops-log-k8s-mutating-wh/manifest-examples/05-prysm-sidecar-secret.yaml | Updates example secret with new tracking env vars. |
| ops-log-k8s-mutating-wh/manifest-examples/05-prysm-sidecar-configmap.yaml | Updates example configmap with new tracking env vars. |
| docs/ops-log.md | Updates documentation examples to the new env var set including SLI tracking. |
| Dockerfile | Splits builds into no-Ceph and Ceph-enabled targets; adds runtime-ceph stage. |
| .github/workflows/release.yml | Adds build/push + attestation for a Ceph-enabled image variant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
senolcolak
reviewed
Jun 25, 2026
senolcolak
left a comment
senolcolak
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Findings
🔴 Must Fix
1. Stale PG gauge leak — pkg/producers/pgprobe/prometheus.go
GaugeVec.With(labels).Set(1) / Set(0) accumulates label combinations forever. After a PG split or pool reconfiguration, old pgid entries linger indefinitely, polluting cardinality and misleading dashboards.
Fix: reset or delete stale labels before each cycle:
pgProbeSuccess.Reset()
pgProbeLatency.Reset()
for _, r := range results {
// then re-populate
}The sliCollector in opslog already solves this with a reaper — pgprobe needs the same treatment.
🟡 Should Fix
2. globalSLICollector package global causes test races — pkg/producers/opslog/prometheus_sli.go
var globalSLICollector *sliCollector is mutated directly in tests (globalSLICollector = newSLICollector(...)). Under go test -race with parallel tests this is a data race. Consider passing the collector as an explicit dependency via MetricsConfig, or at minimum add t.Cleanup to restore state after each test that mutates it.
3. getTotalPGsFromRadosmon silently returns 0 on error — pkg/producers/pgprobe/pgprobe.go
if err := conn.MonCommand(...); err != nil {
log.Error().Err(err).Msg("failed to get pg_num")
return 0 // caller uses this as TotalPGs
}Returning 0 makes CoveredPGs < TotalPGs false, hiding the failure and silencing the warning that should fire. Return an error and let initPoolState propagate it.
4. Zero-latency observations silently dropped — pkg/producers/opslog/prometheus.go
if logEntry.TotalTime > 0 { // ← drops sub-millisecond requests
globalSLICollector.observeLatency(...)
}TotalTime is in milliseconds, so any request completing in < 1 ms rounds to 0 and is never recorded in the histogram. Change the guard to >= 0 (or remove it entirely — observeLatency(0.0) is valid and lands in the le=0.05 bucket).
🔵 Notes
5. Credential exposure via CLI flags — pkg/commands/producer_pg_probe.go
--access-key and --secret-key are passed as flags, which means they appear in ps aux output. The getEnv fallback in producer_pgprobe.go already supports ACCESS_KEY/SECRET_KEY env vars — document this as the recommended path for production deployments.
6. Duplicate defaults — pkg/commands/producer_pg_probe.go + pkg/producers/config/producer_pgprobe.go
Defaults like Interval: 15 and ProbeTimeoutMs: 5000 are declared twice — once in init() flag defaults, once as if cfg.X <= 0 { cfg.X = default } guards. Pick one authoritative source to avoid drift.
7. No unit tests for pure pgprobe functions
groupShardsByBucket, classifyPGID (hex/PGID parsing), the JSON unmarshalling in getTotalPGsFromRadosmon, and the reap() TTL logic are all testable without RADOS. Table-driven tests for these would give good coverage at zero infrastructure cost.
8. Over-commenting in config struct — pkg/producers/pgprobe/config.go
Inline comments like // Probe interval in seconds (default: 15) restate what the field name and flag description already say. Per project conventions, comments should explain why, not what.
fa-at-pulsit
requested changes
Jul 7, 2026
fa-at-pulsit
left a comment
fa-at-pulsit
left a comment
Collaborator
There was a problem hiding this comment.
check my few comments, few of them are blocker, imho
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
3 times, most recently
from
August 25, 2026 10:42
908d5e1 to
2d12979
Compare
Collaborator
Author
|
removed pgprobe L3 metrics and implementation from this pr, as this needs more thought. |
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
August 25, 2026 11:06
2d12979 to
6c28ef9
Compare
jrse
changed the title
fa-at-pulsit
reviewed
Sep 2, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed SLI classification and metric-emission correctness issues (misclassified ops + float-based counter/histogram accumulators) and multiple CLI/README descriptions that no longer match the new per-tenant SLI behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
pkg/producers/opslog/sli.go:65
list_bucket_multipartsis listed in both the LIST and Multipart cases. Because the switch uses the first matching case,list_bucket_multipartswill always classify aslistand the Multipart branch is unreachable for that operation.
// LIST operations
case op == "list_bucket" || op == "list_buckets" || op == "list_objects" ||
op == "get_bucket_info" || op == "list_bucket_versions" ||
op == "list_bucket_multiparts":
return SLIOperationList, true
pkg/producers/opslog/prometheus_sli.go:55
- Histogram bucket counts and
countare stored asfloat64, which will also lose integer precision over time (and then get truncated when cast touint64during collection). Usinguint64for counts keeps histogram math exact while still keepingsumasfloat64.
type sliLatencyState struct {
// Fixed histogram buckets per ADR: 0.05, 0.1, 0.5, 1, 5, 30, +Inf
buckets [7]float64 // cumulative count per le boundary
sum float64
count float64
pkg/producers/opslog/prometheus_sli.go:169
Collectcurrently castsstate.counttouint64forNewConstHistogram. Ifcountis stored asuint64, pass it directly to avoid truncation/precision issues.
m, err := prometheus.NewConstHistogram(
c.histogramDesc,
uint64(state.count),
state.sum,
buckets,
pkg/producers/opslog/prometheus_sli.go:304
- If
sliCounterState.valueis changed touint64,counterValuemust cast it back tofloat64to match the helper’s return type and keep tests compiling.
key := sliCounterKey{Tenant: tenant, Protocol: protocol, Operation: operation, StatusClass: statusClass}
if s, ok := c.counters[key]; ok {
return s.value
}
- Files reviewed: 16/16 changed files
- Comments generated: 10
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are build-breaking type errors in the new Prometheus collector and unresolved inconsistencies between the stated SLI scope (object-only) and the operations currently included in classification/docs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/producers/opslog/sli.go:65
- This LIST-operation mapping includes bucket/service-management operations like list_buckets and get_bucket_info, which appears to contradict both the nearby function comment and the PR description stating that control-plane operations are excluded from SLI/SLO calculations. Please reconcile by either excluding these operations from classifySLIOperation or updating the comments/ADR docs to reflect the intended scope.
pkg/producers/opslog/prometheus_sli.go:144
- prometheus.NewConstMetric expects the metric value as float64; passing state.value (uint64) will not compile and prevents the collector from building.
state.value,
pkg/producers/opslog/sli.go:71
- list_bucket_multiparts is listed in both the LIST and Multipart cases; because LIST is checked first, it can never be classified as multipart. Remove the duplicate from one branch to avoid dead/unreachable classification logic.
case op == "complete_multipart" || op == "init_multipart" ||
op == "abort_multipart" || op == "upload_part" ||
op == "list_multipart" || op == "list_bucket_multiparts":
return SLIOperationMultipart, true
pkg/producers/opslog/config.go:76
- The TrackBucketSLO field comment still describes GET/LIST bucket SLI metrics, but the implementation now emits per-tenant SLI metrics for multiple operation categories (get/put/list/delete/head/multipart) with protocol+region labels. Update the comment to match the current behavior.
TrackBucketSLO bool `yaml:"track_bucket_slo"` // Dedicated low-cardinality GET/LIST SLI metrics for Prometheus SLOs
pkg/commands/producer_ops_log.go:738
- The flag help text still says 'bucket GET/LIST SLI metrics', but the new collector emits per-tenant SLI metrics across several operation categories and reaps idle tenant series. Update these descriptions to avoid misconfigurations and operator confusion.
opsLogCmd.Flags().BoolVar(&opsTrackBucketSLO, "track-bucket-slo", false, "Track low-cardinality bucket GET/LIST SLI metrics for Prometheus SLOs")
// Bucket SLO collector tuning flags
opsLogCmd.Flags().StringVar(&opsBucketSLOStaleTTL, "bucket-slo-stale-ttl", "24h", "Duration after which idle bucket SLI series are reaped")
opsLogCmd.Flags().StringVar(&opsBucketSLOReapInterval, "bucket-slo-reap-interval", "", "How often the SLI reaper runs (default: stale-ttl/4)")
pkg/producers/opslog/README.md:72
- These CLI flag descriptions still refer to 'bucket GET/LIST' series, but the SLI metrics are now per-tenant and cover multiple operation categories (and the reaper cleans up idle tenant series). Update the wording to match the current behavior.
- `--track-bucket-slo` - Enable low-cardinality bucket GET/LIST SLI metrics for
Prometheus SLOs.
- `--bucket-slo-stale-ttl "24h"` - Duration after which idle bucket SLI series
are reaped and no longer exposed on `/metrics` (default: `24h`).
- `--bucket-slo-reap-interval "6h"` - How often the background reaper runs to
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
There are confirmed correctness/documentation issues in SLI operation classification and configuration messaging, plus reliability/accuracy concerns in the custom collector and tests that should be addressed before approval.
Review details
Suppressed comments (8)
Previously missed (5) — in code that hasn't changed since the last review.
pkg/producers/opslog/metrics_test.go:466
- These tests rely on
time.Sleepwith millisecond-scale TTLs, which can be flaky under load or on slow CI runners. Since the tests are in-package, you can deterministically force staleness by rewriting the series'lastSeentimestamps instead of sleeping.
This issue also appears on line 488 of the same file.
pkg/producers/opslog/prometheus_sli.go:88
newSLICollectordoes not guard againstStaleTTL <= 0. If a caller accidentally passes a zero/negative TTL, all series become immediately stale (and may never be emitted), and the ReapInterval defaulting logic becomes misleading. DefaultingStaleTTLdefensively avoids surprising behavior.
pkg/producers/opslog/sli.go:49classifySLIOperationclaims bucket-management ops are excluded from the SLI, buthead_bucketis currently classified as an SLI HEAD operation. This would include control-plane bucket checks in the availability denominator/numerator and contradict the doc comment.
This issue also appears on line 57 of the same file.
pkg/producers/opslog/sli.go:65
list_bucket_multipartsis matched in both the LIST and Multipart branches; because the LIST case appears first, this operation will never be classified asmultipart. This makes the later Multipart branch partially dead and produces inconsistent labeling for multipart listing.
pkg/commands/producer_ops_log.go:738- The CLI flag help strings still describe the SLI as "bucket GET/LIST" and "idle bucket series", but the implementation and README now describe per-tenant SLI metrics across get/put/list/delete/head/multipart. Updating these messages avoids misleading operators configuring SLO monitoring.
pkg/producers/opslog/prometheus_sli.go:55
- The histogram state stores bucket counts and
countasfloat64. Once counts exceed 2^53, float64 can no longer represent every integer, so histogram bucket/count values can become imprecise in long-running/high-traffic deployments. Usinguint64for bucket accumulators and the observation count avoids this silent precision loss.
// sliLatencyState holds histogram bucket accumulators.
type sliLatencyState struct {
// Fixed histogram buckets per ADR: 0.05, 0.1, 0.5, 1, 5, 30, +Inf
buckets [7]float64 // cumulative count per le boundary
sum float64
count float64
lastSeen time.Time
pkg/producers/opslog/metrics_test.go:490
- This test also relies on a millisecond-scale
time.Sleep, which can be flaky in CI. It can be made deterministic by forcing all series'lastSeentimestamps to be older thanStaleTTLbefore callingreap().
// Wait for series to become stale, then reap
time.Sleep(5 * time.Millisecond)
collector.reap()
pkg/producers/opslog/sli.go:58
classifySLIOperationcurrently treatsdelete_bucketas an SLI DELETE operation, but the function comment says bucket management operations are deliberately excluded from the SLI. Includingdelete_bucketwill skew the tenant availability ratio away from the intended object-data-plane scope.
case op == "delete_obj" || op == "delete_object" || op == "delete_bucket" ||
op == "delete_multi_obj" || op == "multi_object_delete":
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
SLI operation classification appears to miss known RGW HEAD operation names used elsewhere in the repo, which would silently drop HEAD traffic from the SLI metrics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/producers/opslog/sli.go:44
- The comment references docs/ceph/rgw-ops-reference.md as the exhaustive operation list, but that file does not exist in this repository, so the pointer is misleading for future maintainers.
pkg/producers/opslog/README.md:72
- The --track-bucket-slo flag description still says this enables per-bucket GET/LIST SLI metrics, but the implementation now emits per-tenant SLI metrics across multiple data-plane operation categories (get/put/list/delete/head/multipart) and adds protocol/region labels.
- `--track-bucket-slo` - Enable low-cardinality bucket GET/LIST SLI metrics for
Prometheus SLOs.
- `--bucket-slo-stale-ttl "24h"` - Duration after which idle bucket SLI series
are reaped and no longer exposed on `/metrics` (default: `24h`).
- `--bucket-slo-reap-interval "6h"` - How often the background reaper runs to
pkg/producers/opslog/README.md:395
- The radosgw_request_total description says it counts "all operations", but the code explicitly excludes control-plane operations (only get/put/list/delete/head/multipart are counted). The README should match the implementation to avoid incorrect SLO rules.
| `radosgw_request_total` | Counter | `tenant`, `protocol`, `operation`, `status_class`, `region` | Per-tenant SLI request counter for all operations, with protocol label (`s3`/`swift`). |
pkg/commands/producer_ops_log.go:738
- The CLI help for --track-bucket-slo and the reaper tuning flags still refers to "bucket GET/LIST" and "idle bucket SLI series", but the collector is now per-tenant and spans multiple data-plane operation categories. Updating the help text will prevent operator misconfiguration.
opsLogCmd.Flags().BoolVar(&opsTrackBucketSLO, "track-bucket-slo", false, "Track low-cardinality bucket GET/LIST SLI metrics for Prometheus SLOs")
// Bucket SLO collector tuning flags
opsLogCmd.Flags().StringVar(&opsBucketSLOStaleTTL, "bucket-slo-stale-ttl", "24h", "Duration after which idle bucket SLI series are reaped")
opsLogCmd.Flags().StringVar(&opsBucketSLOReapInterval, "bucket-slo-reap-interval", "", "How often the SLI reaper runs (default: stale-ttl/4)")
pkg/producers/opslog/config.go:79
- MetricsConfig.TrackBucketSLO is still documented as "GET/LIST" bucket SLI metrics, but the new SLI pipeline is per-tenant and includes additional operation categories as well as protocol/region labels. Updating this struct comment (and the related settings header) will keep config documentation accurate.
TrackEverything bool `yaml:"track_everything"` // Enables all metrics at all levels
TrackBucketSLO bool `yaml:"track_bucket_slo"` // Dedicated low-cardinality GET/LIST SLI metrics for Prometheus SLOs
// === BUCKET SLO COLLECTOR SETTINGS ===
BucketSLOStaleTTL string `yaml:"bucket_slo_stale_ttl"` // Duration after which idle series are reaped (default: 24h)
- Files reviewed: 16/16 changed files
- Comments generated: 2
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The SLI operation classifier currently drops documented RGW operations (head_obj, head_bucket), which would silently exclude valid requests from the SLI metrics.
Review details
Suppressed comments (3)
pkg/producers/opslog/sli.go:58
classifySLIOperationno longer recognizeshead_obj, even though the file comment (and other parts of the repo) indicate RGW can emithead_objfor HEAD-on-object. This will silently exclude those requests from SLI counting/latency.
// GET — object retrieval.
// Includes HEAD-on-object which RGW handles via RGWGetObj → "get_obj".
case "get_obj":
return SLIOperationGet, true
pkg/producers/opslog/sli.go:84
classifySLIOperationdocuments that bucket HEAD may be logged as eitherhead_bucketorstat_bucket, but onlystat_bucketis currently classified. If RGW emitshead_bucket, those requests will be dropped from the SLI.
// HEAD — metadata-only requests (stat operations).
// stat_bucket: HEAD bucket (RGWStatBucket).
// stat_account: HEAD account, Swift only (RGWStatAccount).
case "stat_bucket", "stat_account":
return SLIOperationHead, true
pkg/producers/opslog/README.md:395
- The README says
radosgw_request_totalcovers “all operations”, but the implementation explicitly excludes control-plane operations from the SLI. This can mislead operators writing SLO queries.
| `radosgw_request_total` | Counter | `tenant`, `protocol`, `operation`, `status_class`, `region` | Per-tenant SLI request counter for all operations, with protocol label (`s3`/`swift`). |
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The SLI operation/protocol classification has confirmed gaps/false-positive behavior and several user-facing docs/flag descriptions are inconsistent with the new tenant-level, multi-operation SLI scope.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/producers/opslog/sli.go:120
- detectProtocol() looks for "/swift/v1/" anywhere in the URI line; this can misclassify valid S3 requests when the bucket/key contains the substring "swift/v1". Protocol detection should be based on the start of the request path instead of a substring search across the whole line.
pkg/producers/opslog/sli.go:58
- classifySLIOperation() documents that HEAD-on-object can be logged as "head_obj", but the switch currently only matches "get_obj"; this will drop HEAD requests from SLI counting on RGW versions/log formats that emit "head_obj".
// GET — object retrieval.
// Includes HEAD-on-object which RGW handles via RGWGetObj → "get_obj".
case "get_obj":
return SLIOperationGet, true
pkg/producers/opslog/sli.go:84
- classifySLIOperation() notes that bucket HEAD may be logged as "head_bucket" or "stat_bucket", but only "stat_bucket" is currently recognized; this can undercount HEAD availability for some RGW versions.
// HEAD — metadata-only requests (stat operations).
// stat_bucket: HEAD bucket (RGWStatBucket).
// stat_account: HEAD account, Swift only (RGWStatAccount).
case "stat_bucket", "stat_account":
return SLIOperationHead, true
pkg/producers/opslog/README.md:73
- The flag docs still describe --track-bucket-slo and its reaping settings as per-bucket GET/LIST metrics, but the implementation/metrics are now per-tenant and include additional data-plane operations (put/delete/head/multipart). This is user-facing documentation drift.
- `--track-bucket-slo` - Enable low-cardinality bucket GET/LIST SLI metrics for
Prometheus SLOs.
- `--bucket-slo-stale-ttl "24h"` - Duration after which idle bucket SLI series
are reaped and no longer exposed on `/metrics` (default: `24h`).
- `--bucket-slo-reap-interval "6h"` - How often the background reaper runs to
remove stale series from memory (default: `stale-ttl/4`).
pkg/producers/opslog/config.go:80
- TrackBucketSLO's comment still says "GET/LIST" only, but the SLI operation classifier includes get/put/list/delete/head/multipart. Update the struct comment so the config/API reflects the actual metric scope.
// === SHORTCUT CONFIGS ===
TrackEverything bool `yaml:"track_everything"` // Enables all metrics at all levels
TrackBucketSLO bool `yaml:"track_bucket_slo"` // Dedicated low-cardinality per-tenant GET/LIST SLI metrics for Prometheus SLOs
// === BUCKET SLO COLLECTOR SETTINGS ===
BucketSLOStaleTTL string `yaml:"bucket_slo_stale_ttl"` // Duration after which idle tenant series are reaped (default: 24h)
BucketSLOReapInterval string `yaml:"bucket_slo_reap_interval"` // How often the reaper runs (default: stale_ttl/4)
pkg/commands/producer_ops_log.go:738
- The --track-bucket-slo help text still says "GET/LIST" only, but the new SLI metrics include additional operations (put/delete/head/multipart). This makes the CLI help inaccurate.
// Shortcut flag
opsLogCmd.Flags().BoolVar(&opsTrackEverything, "track-everything", false, "Enable detailed tracking for all metric types (efficient mode)")
opsLogCmd.Flags().BoolVar(&opsTrackBucketSLO, "track-bucket-slo", false, "Track low-cardinality per-tenant GET/LIST SLI metrics for Prometheus SLOs")
// Bucket SLO collector tuning flags
opsLogCmd.Flags().StringVar(&opsBucketSLOStaleTTL, "bucket-slo-stale-ttl", "24h", "Duration after which idle tenant SLI series are reaped")
opsLogCmd.Flags().StringVar(&opsBucketSLOReapInterval, "bucket-slo-reap-interval", "", "How often the SLI reaper runs (default: stale-ttl/4)")
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new SLI operation classifier and associated docs/tests contain concrete scope mismatches (notably HEAD op variants and user-facing “GET/LIST only” descriptions) that can lead to silently missing SLI data or confusing configuration.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
pkg/producers/opslog/metrics_test.go:293
- TestClassifySLIOperation only asserts HEAD classification for stat_bucket/stat_account, but classifySLIOperation is expected to handle head_obj/head_bucket variants as well. Adding explicit test cases prevents regressions where HEAD operations are unintentionally dropped from SLI.
pkg/commands/producer_ops_log.go:734 - The --track-bucket-slo help text still says "GET/LIST" only, but the SLI classifier includes put/delete/head/multipart as well. This is user-facing and should match the actual metric scope.
pkg/producers/opslog/README.md:71 - The CLI flag docs still describe --track-bucket-slo and --bucket-slo-stale-ttl as bucket-scoped GET/LIST metrics/series, but the implementation is per-tenant and covers more operations. This can confuse operators when they validate expected label cardinality.
pkg/producers/opslog/config.go:76 - The TrackBucketSLO field comment still says "GET/LIST" only, but the new SLI implementation also classifies put/delete/head/multipart operations. This comment is now misleading for config readers.
pkg/producers/opslog/sli.go:84
- classifySLIOperation documents that RGW may log HEAD as "head_obj"/"head_bucket" (and other parts of the repo reference these ops), but the classifier only recognizes "stat_bucket"/"stat_account" for HEAD. This will silently exclude HEAD requests from SLI metrics on clusters/log formats that emit head_* ops.
// HEAD — metadata-only requests (stat operations).
// stat_bucket: HEAD bucket (RGWStatBucket).
// stat_account: HEAD account, Swift only (RGWStatAccount).
case "stat_bucket", "stat_account":
return SLIOperationHead, true
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
September 4, 2026 07:48
7a2f2d9 to
2db2f8c
Compare
Implement the L1/L2 SLI layer: per-tenant HTTP success
rate and latency histogram, replacing the removed per-bucket PG-derived
gauges.
New metrics:
radosgw_request_total{tenant, protocol, operation, status_class, region}
radosgw_request_duration_seconds{tenant, protocol, operation, region}
SLI formula: Availability = 1 - (5xx_requests / total_requests)
Measured per tenant, per 5-minute interval, averaged monthly.
Matches AWS S3 SLA definition.
Key changes:
- Protocol detection from URI path (/swift/v1/ = swift, else s3);
the JSON ops-log does not prefix operations with the protocol.
- Operation classification rewritten against RGW operation name() values
from rgw_op.h: get_obj, put_obj, post_obj, copy_obj, restore_obj,
bulk_upload, delete_obj, multi_object_delete, bulk_delete,
list_bucket, list_buckets, stat_bucket, stat_account,
init_multipart, complete_multipart, abort_multipart,
list_multipart, list_bucket_multiparts.
- SLIOperationHead now reachable via stat_bucket/stat_account.
- list_bucket_multiparts classified as multipart (was dead code
under list due to duplicate switch case).
- delete_bucket excluded from SLI (control-plane).
- Custom Prometheus collector with stale series reaper (configurable
TTL, default 24h) to bound cardinality from idle tenants.
- Fixed radosgw_total_requests_per_bucket label swap (bucket/user).
- Added --region flag and REGION env var for deployment region label.
- Histogram with 7 fixed buckets (0.05, 0.1, 0.5, 1, 5, 30, +Inf)
aligned to SLO latency targets.
Signed-off-by: Jan Radon <jan.fabian.radon@sap.com>
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
October 15, 2026 12:06
2db2f8c to
e0fffd7
Compare
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues can omit valid HEAD requests, distort latency and availability SLI results, and prevent configured stale-series intervals from being honored.
3 open issues
1 resolved since last review
Review effort: Lite
Comment on lines
+138
to
+140
| if now.Sub(state.lastSeen) > c.config.StaleTTL { | ||
| continue // stale — skip (reaper will clean up) | ||
| } |
Comment on lines
+112
to
+119
| // We look for "/swift/v1/" anywhere in the URI to keep it simple | ||
| // and resilient to variations in HTTP method or query parameters. | ||
| lowerURI := strings.ToLower(uri) | ||
| if strings.Contains(lowerURI, "/swift/v1/") || strings.Contains(lowerURI, "/swift/v1 ") { | ||
| return SLIProtocolSwift | ||
| } | ||
|
|
||
| return SLIProtocolS3 |
…xported histograms' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jan Fabian Radon <jan.radon@jaraco.de>
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
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.
RGW SLI/SLO Monitoring: Refactored per ADR-001
Problem (revised)
The original PR derived per-bucket availability from storage-layer PG health. After reviewing what AWS, GCP, Azure, and the Google SRE Workbook actually do, we identified three issues:
What Changed in This PR
L1/L2 SLI Metrics
radosgw_request_totaltenant,protocol,operation,status_class,regionradosgw_request_duration_secondstenant,protocol,operation,regionradosgw_sli_stale_series_reaped_totalLabel Values
protocols3,swiftoperationget,put,list,delete,multipartstatus_class2xx,3xx,4xx,5xxregionqa-de-1,eu-de-2(from--regionflag /REGIONenv var)SLO Queries (PromQL)
With this change i propose the following formulars to calculate SLA per tenant.
SLI Formula
Measured per tenant, per 5-minute interval, averaged monthly. Matches AWS S3 SLA definition.
Intervals with zero traffic contribute 0% error rate (AWS/GCP convention).
L1 SLA: Per-Tenant Monthly Availability
e.g.:
L2 SLO: Per-Tenant x Protocol (catches Swift-vs-S3 divergence)
L2 SLO: Per-Operation Latency (P99)
e.g.: