Conversation
Metric time series data expires via the pcp:values:series: stream TTL
(stream.expire, default 86400s), but the ~10 ancillary keys per series
(pcp:desc:series:, pcp:series:metric.name:, pcp:series:label.*.value:,
etc.) accumulated indefinitely, causing unbounded key server growth.
Add pmSeriesGC() to libpcp_web and a --gc / --dryrun mode to pmseries
that scans pcp:desc:series:* for all known series, checks whether each
stream is still alive, and for stale series removes:
- pcp:desc:series:H, pcp:metric.name:series:H
- pcp:instances:series:H, pcp:labelvalue:series:H, pcp:labelflags:series:H
- membership in pcp:series:metric.name:*, pcp:series:inst.name:*,
pcp:series:label.<name>.value:*, pcp:series:context.name:*
- orphaned pcp:inst:series:* hashes when their inst-name set empties
All key server operations are fully async using the existing
baton/callback infrastructure. Three new pmproxy MMV metrics track GC
activity: series.gc.calls, series.gc.scanned, series.gc.cleaned.
Output uses the standard info callback so results appear on stdout for
the CLI and in the pmproxy log when invoked from a timer.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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
🤖 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 `@man/man1/pmseries.1`:
- Around line 748-771: Update the --gc documentation near its key-value storage
description to state that garbage collection currently scans only the first
key-server node, rather than implying cluster-wide cleanup. Preserve the
existing descriptions of expired-series and orphaned-data removal.
In `@qa/1790`:
- Line 79: Update the dry-run log filtering command in qa/1790 to match the
actual “GC dryrun:” prefix instead of searching for “GC:”. Preserve the existing
prefix removal and golden-output comparison so dry-run records are retained and
grep can find them.
In `@src/pmseries/pmseries.c`:
- Around line 1502-1503: Update the option-validation logic in pmseries main
processing so --gc rejects any positional operands instead of silently ignoring
them; require opts.optind == argc when PMSERIES_OPT_GC is set, while preserving
existing missing-argument and PM_OPTFLAG_EXIT behavior. Add a QA regression
covering pmseries --gc followed by a query or series argument and verify it
fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 27604123-a0cb-4f18-99b4-2a47f8f35be2
⛔ Files ignored due to path filters (1)
qa/1790.outis excluded by!**/*.out
📒 Files selected for processing (11)
man/man1/pmseries.1qa/1790qa/common.checkqa/groupsrc/bashrc/pcp_completion.shsrc/include/pcp/pmwebapi.hsrc/libpcp_web/src/batons.hsrc/libpcp_web/src/exports.insrc/libpcp_web/src/schema.csrc/libpcp_web/src/schema.hsrc/pmseries/pmseries.c
| \fB\-G\fR, \fB\-\-gc\fR | ||
| Scan for expired time series and remove their ancillary data from the | ||
| key-value cluster. | ||
| A series is considered expired when its | ||
| .B pcp:values:series: | ||
| stream key no longer exists (i.e. the | ||
| .B stream.expire | ||
| TTL has elapsed without new data being ingested). | ||
| The following keys are removed for each expired series: | ||
| .B pcp:desc:series:, | ||
| .B pcp:metric.name:series:, | ||
| .B pcp:instances:series:, | ||
| .B pcp:labelvalue:series:, | ||
| and | ||
| .B pcp:labelflags:series:. | ||
| The series identifier is also removed from the forward-lookup sets | ||
| .B pcp:series:metric.name:, | ||
| .B pcp:series:inst.name:, | ||
| .B pcp:series:label.<name>.value:, | ||
| and | ||
| .B pcp:series:context.name:. | ||
| Orphaned | ||
| .B pcp:inst:series: | ||
| hashes are cleaned up when their associated instance-name set becomes empty. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the current single-node GC scope.
Lines 749-750 promise cleanup across the “key-value cluster,” but this implementation currently scans only the first key-server node. State that limitation so operators do not assume a cluster-wide cleanup completed.
🤖 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 `@man/man1/pmseries.1` around lines 748 - 771, Update the --gc documentation
near its key-value storage description to state that garbage collection
currently scans only the first key-server node, rather than implying
cluster-wide cleanup. Preserve the existing descriptions of expired-series and
orphaned-data removal.
| pmseries $options --gc --dryrun > $tmp.dryrun 2>&1 | ||
| ndryrun=`grep -c 'GC dryrun:' $tmp.dryrun` | ||
| echo "dryrun reported $ndryrun series" | ||
| grep 'GC:' $tmp.dryrun | sed 's/pmseries: \[Info\] //' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match the dry-run log prefix.
GC dryrun: does not contain GC:. This prints no dry-run records, and the failed grep status is ignored. Match the actual prefix to retain the intended golden-output coverage.
Proposed fix
-grep 'GC:' $tmp.dryrun | sed 's/pmseries: \[Info\] //'
+grep 'GC dryrun:' $tmp.dryrun | sed 's/pmseries: \[Info\] //'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grep 'GC:' $tmp.dryrun | sed 's/pmseries: \[Info\] //' | |
| grep 'GC dryrun:' $tmp.dryrun | sed 's/pmseries: \[Info\] //' |
🤖 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 `@qa/1790` at line 79, Update the dry-run log filtering command in qa/1790 to
match the actual “GC dryrun:” prefix instead of searching for “GC:”. Preserve
the existing prefix removal and golden-output comparison so dry-run records are
retained and grep can find them.
| if (opts.optind == argc && !opts.errors && !(opts.flags & PM_OPTFLAG_EXIT) && | ||
| !(flags & PMSERIES_OPT_GC)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject positional arguments for --gc.
Line 1502 only bypasses the missing-argument check; pmseries --gc <query-or-series> proceeds and silently ignores the operand. For a destructive whole-store operation, reject operands explicitly and add a QA regression.
Proposed fix
+ if ((flags & PMSERIES_OPT_GC) && opts.optind != argc) {
+ pmprintf("%s: error - --gc does not accept query or series arguments\n",
+ pmGetProgname());
+ opts.errors++;
+ }
- if (opts.optind == argc && !opts.errors && !(opts.flags & PM_OPTFLAG_EXIT) &&
+ else if (opts.optind == argc && !opts.errors && !(opts.flags & PM_OPTFLAG_EXIT) &&
!(flags & PMSERIES_OPT_GC)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (opts.optind == argc && !opts.errors && !(opts.flags & PM_OPTFLAG_EXIT) && | |
| !(flags & PMSERIES_OPT_GC)) { | |
| if ((flags & PMSERIES_OPT_GC) && opts.optind != argc) { | |
| pmprintf("%s: error - --gc does not accept query or series arguments\n", | |
| pmGetProgname()); | |
| opts.errors++; | |
| } | |
| else if (opts.optind == argc && !opts.errors && !(opts.flags & PM_OPTFLAG_EXIT) && | |
| !(flags & PMSERIES_OPT_GC)) { |
🤖 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 `@src/pmseries/pmseries.c` around lines 1502 - 1503, Update the
option-validation logic in pmseries main processing so --gc rejects any
positional operands instead of silently ignoring them; require opts.optind ==
argc when PMSERIES_OPT_GC is set, while preserving existing missing-argument and
PM_OPTFLAG_EXIT behavior. Add a QA regression covering pmseries --gc followed by
a query or series argument and verify it fails.
2 participants
Summary
pcp:desc:series:,pcp:series:metric.name:,pcp:series:label.*.value:, etc.) accumulated indefinitely after stream TTL expiry, causing unbounded key server growthpmSeriesGC()to libpcp_web andpmseries --gc/--dryrunto scan for expired series and remove all associated keys asynchronouslyseries.gc.calls,series.gc.scanned,series.gc.cleanedDetails
When
pcp:values:series:Hexpires (viastream.expire, default 86400s) the raw data is reclaimed but ~10 ancillary keys per series remain.--gcscanspcp:desc:series:*, checks each stream's liveness, and for dead series removes:pcp:desc:series:H,pcp:metric.name:series:H,pcp:instances:series:H,pcp:labelvalue:series:H,pcp:labelflags:series:Hpcp:series:metric.name:*,pcp:series:inst.name:*,pcp:series:label.<name>.value:*,pcp:series:context.name:*pcp:inst:series:*hashes when their inst-name set becomes emptyOutput goes through the standard info callback (stdout for CLI, pmproxy log for timer use). A
--dryrunflag logs what would be removed without writing anything.Tested against a local Valkey instance: 9547 series scanned, 8402 identified for cleanup.
Known limitation: SCAN is sent to the first node only (
keySlotsRequestFirstNode); full cluster support is future work.Test plan
pmseries --gc --dryrun— verify series listed without any key server writespmseries --gc— verify ancillary keys removedseries.gc.*MMV metrics visible via pmproxy after a GC run--gcis mutually exclusive with--load,--query,--values,--window--dryrunwithout--gcis rejected with a clear error🤖 Generated with Claude Code