feat: RGW index pool PG probe for per-bucket SLI/SLO monitoring#52
feat: RGW index pool PG probe for per-bucket SLI/SLO monitoring#52jrse wants to merge 10 commits into
Conversation
|
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.goimportsgithub.com/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
|
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 |
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
054f8bb to
67d92c1
Compare
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
40a012e to
ce82c54
Compare
Implement a three-layer observability architecture for RGW object storage:
L1/L2 SLI (opslog producer):
- radosgw_request_total{tenant, protocol, operation, status_class, region}
- radosgw_request_duration_seconds histogram (7 fixed le buckets)
- Protocol detection (S3/Swift), 6 operation classes, stale series reaper
- Activated via --track-bucket-slo --region <region>
L3 PG Probe (new local-producer, requires -tags ceph):
- Probes all PGs in RGW index pools via rados stat every 15s
- radosgw_index_probe_success, radosgw_index_probe_latency_seconds
- radosgw_index_pool_available_pgs_ratio (leading indicator)
- Multi-pool support, hourly mapping refresh, pigeonhole-based coverage
Build:
- Multi-stage Dockerfile: runtime-noceph (default) and runtime-ceph
- Conditional compilation via //go:build ceph
- CI workflow for pg-probe container
Bug fix:
- Correct user/bucket label swap in radosgw_total_requests_per_bucket
Includes unit tests for SLI classification, protocol detection, status
parsing, stale reaping, and per-bucket label correctness.
jrse
force-pushed
the
feature/bucket-GET-LIST-SLI-metrics-tiered
branch
from
ce82c54 to
ba0e7d5
Compare
Signed-off-by: Jan Fabian Radon <jan.fabian.radon@sap.com>
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.
| func StartPrometheusServer(port int) { | ||
| go func() { | ||
| http.Handle("/metrics", promhttp.Handler()) | ||
| log.Info().Int("port", port).Msg("starting prometheus metrics server for pg-probe") | ||
| if err := http.ListenAndServe(fmt.Sprintf(":%d", port), nil); err != nil { | ||
| log.Fatal().Err(err).Msg("prometheus metrics server failed") | ||
| } | ||
| }() | ||
| } |
| if len(results) > 0 { | ||
| pgPoolAvailableRatio.With(poolLabels).Set(float64(successCount) / float64(len(results))) | ||
| } |
| // 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 | ||
|
|
||
| // Multipart operations | ||
| case op == "complete_multipart" || op == "init_multipart" || | ||
| op == "abort_multipart" || op == "upload_part" || | ||
| op == "list_multipart" || op == "list_bucket_multiparts": | ||
| return SLIOperationMultipart, true |
| // Counter is now keyed by tenant+protocol+operation+status_class (no bucket) | ||
| beforeCounter := globalSLICollector.counterValue("tenant-slo-test", "s3", "get", "2xx") | ||
| assert.NotPanics(t, func() { | ||
| NewMetrics().Update(logEntry, config) | ||
| }) | ||
| afterCounter := readCounterValue(t, sliRequestsTotal, "tenant-slo-test", "bucket-slo-test", "get", "2xx") | ||
| afterHist := readHistogramSampleCount(t, sliRequestDuration, "tenant-slo-test", "bucket-slo-test", "get") | ||
| afterCounter := globalSLICollector.counterValue("tenant-slo-test", "s3", "get", "2xx") | ||
|
|
||
| assert.Equal(t, beforeCounter+1, afterCounter, "SLI counter should increment") | ||
| assert.Equal(t, beforeHist+1, afterHist, "SLI histogram should record a sample") | ||
| } |
| beforeCounter := globalSLICollector.counterValue("tenant-slo-zero", "s3", "get", "2xx") | ||
| NewMetrics().Update(logEntry, config) | ||
| afterCounter := readCounterValue(t, sliRequestsTotal, "tenant-slo-zero", "bucket-slo-zero", "get", "2xx") | ||
| afterHist := readHistogramSampleCount(t, sliRequestDuration, "tenant-slo-zero", "bucket-slo-zero", "get") | ||
| afterCounter := globalSLICollector.counterValue("tenant-slo-zero", "s3", "get", "2xx") | ||
|
|
||
| assert.Equal(t, beforeCounter+1, afterCounter, "SLI counter should increment even with zero latency") | ||
| assert.Equal(t, beforeHist+1, afterHist, "SLI histogram should record a sample even with zero latency") | ||
| } |
| // List all .dir.* and .bucket.meta.* objects in the index pool | ||
| var shardObjects []string | ||
| var probeBucketMarker string | ||
|
|
||
| metaPrefix := ".bucket.meta." + probeBucket + ":" | ||
|
|
||
| iter, err := ioctx.Iter() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create object iterator: %w", err) |
| func PublishToPrometheus(results []ProbeResult, targets *ProbeTargets, cfg PGProbeConfig) { | ||
| successCount := 0 | ||
| var totalLatency float64 | ||
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Jan Fabian Radon <jan.radon@jrse.de>
senolcolak
left a comment
senolcolak
left a comment
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.
GaugeVec.With(labels).Set() accumulates label combinations forever. After a PG split or pool reconfiguration, old pgid entries would linger indefinitely, polluting cardinality and misleading dashboards. Reset pgProbeSuccess and pgProbeLatency at the start of each probe cycle so only current PG IDs are exposed on /metrics.
- fix(pgprobe): return error from getPoolPGCount instead of silent 0 Returning 0 hid failures and suppressed the coverage warning. Caller now propagates the error to initPoolState. - fix(opslog): record sub-millisecond latencies in SLI histogram TotalTime=0 (sub-ms) was silently dropped by > 0 guard. Changed to >= 0 so 0ms requests land in the le=0.05 bucket. Only negative values (corrupted entries) are excluded. - fix(opslog): prevent test race on globalSLICollector Added t.Cleanup to save/restore the package global in all tests that mutate it. Passes go test -race. - chore(opslog): remove unused prometheus/dto imports from metrics_test
CLI flags are visible in ps aux, exposing credentials to other users on the same host. Credentials are now exclusively sourced from ACCESS_KEY and SECRET_KEY environment variables. The config-file path (use-config mode) still supports access_key and secret_key in YAML settings, which is not exposed in the process list.
Define DefaultInterval, DefaultProbeTimeoutMs, DefaultCephConfigPath, etc. as constants in config.go. Reference them from StartMonitoring guards, CLI flag defaults, and config-file dispatch — eliminating three duplicate copies of the same magic numbers.
Table-driven tests for groupShardsByBucket and extractShardIndex covering edge cases (empty input, wrong marker, non-numeric shard, multiple buckets). Also tests default constant values and default-application logic. These tests require -tags ceph (librados headers) so they run in CI Docker builds, not on developer macOS.
Field names and constants are self-documenting. Only keep comments that explain why (env-only credentials, multi-pool rationale).
fa-at-pulsit
left a comment
fa-at-pulsit
left a comment
There was a problem hiding this comment.
check my few comments, few of them are blocker, imho
| latency: make(map[sliLatencyKey]*sliLatencyState), | ||
| config: cfg, | ||
| counterDesc: prometheus.NewDesc( | ||
| "radosgw_request_total", |
There was a problem hiding this comment.
Naming mismatch between code and the ADR / your PR reply. Here the metrics are emitted as
radosgw_request_total and radosgw_request_duration_seconds (line 105), but ADR-001 and
your comment on this PR call them prysm_rgw_request_total / prysm_rgw_request_duration_seconds.
|
|
||
| name: Release Artifacts | ||
| on: | ||
| push: |
There was a problem hiding this comment.
release.yml is the only workflow in the repo and it fires solely on push: tags: v*.
That means the entire pgprobe producer (everything behind //go:build ceph:
pkg/producers/pgprobe/* and pkg/commands/producer_pg_probe.go) plus its -tags ceph tests
are compiled only during the tag/release Docker build — never on a PR. The only check on
this PR is the CodeRabbit bot; there's no go build / go test / go vet in CI at all.
So a plain compile error or a broken test in the ceph path won't surface until someone cuts
a release. Please add a PR-triggered job that runs go build -tags ceph ./... and
go test -tags ceph ./... inside a container with librados/ceph-dev headers (the same deps
the runtime-ceph Dockerfile stage installs). Ideally a non-ceph go test ./... job too.
| } | ||
| pgPoolTotalPGs.With(poolLabels).Set(float64(targets.TotalPGs)) | ||
| pgPoolCoveredPGs.With(poolLabels).Set(float64(targets.CoveredPGs)) | ||
| probeCycleDuration.With(poolLabels).Set(totalLatency / 1000.0) |
There was a problem hiding this comment.
totalLatency is the sum of every per-PG probe latency, not the wall-clock duration of
the cycle. Published under a metric named ..._cycle_duration_seconds, that's misleading —
under any concurrency it has no physical meaning, and even sequentially it double-counts vs.
"how long did the cycle take."
runProbes already measures the real thing (time.Since(now)); thread that value through
and Set it here instead of the latency sum..... imho
| return | ||
| } | ||
|
|
||
| sliOperation, ok := classifySLIOperation(logEntry.Operation) |
There was a problem hiding this comment.
Any operation outside the six recognized classes makes classifySLIOperation return
false (sli.go:68), so observeSLI bails and the request never enters the SLI — including
its 5xx. That drops ACL/policy/admin/create-bucket and anything unmapped from both
numerator and denominator of the availability ratio.
If that's a deliberate "data-plane object ops only" SLI, fine — but it should be documented
next to the metric, because an availability figure that silently excludes whole operation
classes can hide real customer-visible errors. Alternatively, bucket everything unmatched
into an operation="other" class so the denominator stays complete.
|
|
||
| existingOpsLogPreRunE := opsLogCmd.PreRunE | ||
| opsLogCmd.PreRunE = func(cmd *cobra.Command, args []string) error { | ||
| if opsTrackBucketSLO && !opsPromEnabled { |
There was a problem hiding this comment.
WIP: check if tracked - is enabled via TRACK_BUCKET_SLO env or via the TrackEverything
| continue | ||
| } | ||
|
|
||
| pgID, err := getObjectPG(conn, pool, obj) |
There was a problem hiding this comment.
getObjectPG issues an osd map mon command per shard, so a 997-shard probe bucket = ~997
mon round-trips per pool at startup and again on every mapping refresh (default hourly,
pgprobe.go:131). That's fine at hourly cadence, but worth a comment so it isn't a surprise
on a busy mon - and note the failure mode: a per-shard error here is warn+continue, so a
flaky mon silently shrinks CoveredPGs (and thus the available-ratio denominator) without
failing the cycle. Consider a summary warn if CoveredPGs drops materially between refreshes.
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 (Opslog Producer — Tenant-Level)
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,head,multipartstatus_class2xx,3xx,4xx,5xxregionqa-de-1,eu-de-2(from--regionflag /REGIONenv var)L3 PG Probe Metrics (New Producer — Pool-Level Infrastructure Health)
radosgw_index_probe_successpgid,pool,node,instanceradosgw_index_probe_latency_secondspgid,pool,node,instanceradosgw_index_pool_available_pgs_ratiopool,node,instanceradosgw_index_pool_total_pgspool,node,instanceradosgw_index_pool_covered_pgspool,node,instanceradosgw_index_probe_cycle_duration_secondspool,node,instanceradosgw_index_probe_cycles_totalpool,node,instance