pmdapmimport, pcp-summary: add pmimport.* metrics for import tools#2632
Open
natoscott wants to merge 1 commit into
Open
pmdapmimport, pcp-summary: add pmimport.* metrics for import tools#2632natoscott wants to merge 1 commit into
natoscott wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces the Changespmimport PMDA: domain registration, implementation, and integration
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over pcp-summary.sh,pmimport PMDA: Metric collection
pcp-summary.sh->>pminfo: query pmcd.* + pmimport.archive/version/args
pminfo->>pmimport PMDA: fetch instanced + singleton metrics
pmimport PMDA->>PCP_IMPORT_DIR: scan status files (opendir/readdir)
PCP_IMPORT_DIR-->>pmimport PMDA: pid/version/args/archive entries
pmimport PMDA->>pmimport PMDA: mtime_changed? + kill(pid,0) liveness
pmimport PMDA-->>pminfo: string values per instance
pminfo-->>pcp-summary.sh: AWK-parsed pmimport_* variables
end
rect rgba(60, 179, 113, 0.5)
Note over pcp-summary.sh,tmp/pmimport_*: Reporting
pcp-summary.sh->>pcp-summary.sh: fallback timezone/zoneinfo from pmimport if pmcd absent
pcp-summary.sh->>_pmimport_tool: read tmp/pmimport_* cache files
_pmimport_tool-->>pcp-summary.sh: formatted sysstat/sadc + other tools report
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
qa/2005 (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one fixture where
argscontains spaces to harden parser coverage.The current fixture (
args=CPU,DISK) won’t catch whitespace-tokenization regressions inpmimport.argsreporting. Adding one tool with spaced args would lock this behavior down.🤖 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/2005` around lines 31 - 36, Add a second stub sidecar fixture after the existing one (after the EOF on line 36) that creates another sadc file with the same structure but includes spaces within the args field value, such as args=CPU, DISK or similar. This will ensure the pmimport.args parser is tested against whitespace-tokenization scenarios and prevent regressions in how spaces are handled during argument parsing.
🤖 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 `@src/pcp/summary/pcp-summary.sh`:
- Line 448: The test expression on line 448 uses the -a operator inside the
square brackets for combining conditions, which is not POSIX compliant. Replace
this by splitting the test into separate test commands connected with the &&
operator instead. Specifically, replace the single test bracket containing [ -n
"$version" -a -n "$agents" ] with two separate test commands chained with &&
(such that each variable is tested individually). This maintains the same
logical behavior while ensuring POSIX shell portability.
- Around line 480-481: The current AWK command extracts only field 3 ($3) which
gets truncated at the first space when values contain whitespace. To preserve
the full pmimport_args value including spaces, modify the AWK print statement to
extract the complete content from field 3 to the end of the line instead of just
printing $3. This can be done by using substring operations on the input record
to get everything after the second field, or by reconstructing fields from
position 3 onwards to capture the entire value with spaces intact.
In `@src/pmdas/pmimport/pmda_pmimport.c`:
- Around line 130-134: The issue is that pmdaCacheStore reactivates an instance
at line 130, but when the mtime check at line 133 determines the file is
unchanged, the code skips the remaining logic with continue, which bypasses the
pid-based stale detection check that occurs later. To fix this, restructure the
control flow so that the pid liveness validation (currently at lines 147-151) is
performed regardless of whether the mtime has changed. Either move the stale
detection logic before the mtime unchanged check, or refactor the conditional to
only skip the file re-reading but still execute the pid validation. This ensures
that stale daemon instances are properly invalidated on subsequent fetches even
when the file modification time hasn't changed.
---
Nitpick comments:
In `@qa/2005`:
- Around line 31-36: Add a second stub sidecar fixture after the existing one
(after the EOF on line 36) that creates another sadc file with the same
structure but includes spaces within the args field value, such as args=CPU,
DISK or similar. This will ensure the pmimport.args parser is tested against
whitespace-tokenization scenarios and prevent regressions in how spaces are
handled during argument parsing.
🪄 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 UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b9e3cc66-7913-4ebd-afac-f6654791c79e
⛔ Files ignored due to path filters (1)
qa/2005.outis excluded by!**/*.out
📒 Files selected for processing (16)
qa/2005qa/common.filtersrc/include/pcp/import.hsrc/libpcp_import/src/archive.csrc/libpcp_import/src/import.csrc/libpcp_import/src/private.hsrc/pcp/summary/pcp-summary.shsrc/pmdas/GNUmakefilesrc/pmdas/pmcd/src/pmcd.csrc/pmdas/pmimport/.gitignoresrc/pmdas/pmimport/GNUmakefilesrc/pmdas/pmimport/helpsrc/pmdas/pmimport/pmda_pmimport.csrc/pmdas/pmimport/root_pmimportsrc/pmlogconf/tools/pcp-summarysrc/pmns/stdpmid.pcp
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
natoscott
force-pushed
the
pcp-summary-pmimport
branch
from
c49890f to
8addeb0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/pcp/summary/pcp-summary.sh`:
- Around line 167-177: The local-context retry path that executes `eval pminfo
-Lf $metrics` lacks error checking for actual pminfo command failures. After the
retry command on line 176, add a check similar to the one performed on the
initial pminfo command to detect `pminfo:` errors in the error output. If such
errors are found in the retry, the script should surface a clear error message
and exit instead of silently proceeding with potentially empty or partial metric
data.
🪄 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: e6fe87e0-e5ee-4f43-b705-675b7bd19001
⛔ Files ignored due to path filters (1)
qa/2005.outis excluded by!**/*.out
📒 Files selected for processing (16)
qa/2005qa/common.filtersrc/include/pcp/import.hsrc/libpcp_import/src/archive.csrc/libpcp_import/src/import.csrc/libpcp_import/src/private.hsrc/pcp/summary/pcp-summary.shsrc/pmdas/GNUmakefilesrc/pmdas/pmcd/src/pmcd.csrc/pmdas/pmimport/.gitignoresrc/pmdas/pmimport/GNUmakefilesrc/pmdas/pmimport/helpsrc/pmdas/pmimport/pmda_pmimport.csrc/pmdas/pmimport/root_pmimportsrc/pmlogconf/tools/pcp-summarysrc/pmns/stdpmid.pcp
✅ Files skipped from review due to trivial changes (5)
- src/pmdas/pmimport/root_pmimport
- src/include/pcp/import.h
- src/pmlogconf/tools/pcp-summary
- src/pmdas/pmcd/src/pmcd.c
- src/pmdas/pmimport/.gitignore
🚧 Files skipped from review as they are similar to previous changes (10)
- src/libpcp_import/src/archive.c
- src/pmdas/pmimport/help
- src/pmns/stdpmid.pcp
- qa/common.filter
- src/pmdas/GNUmakefile
- src/libpcp_import/src/private.h
- qa/2005
- src/libpcp_import/src/import.c
- src/pmdas/pmimport/GNUmakefile
- src/pmdas/pmimport/pmda_pmimport.c
Introduce a generic pmimport.{archive,version,args} metric set with an
instance domain with each instance an active import tool identified by
name.
pmdapmimport scans PCP_IMPORT_DIR on each fetch; each file is a status
file written by an import tool containing key=value lines:
pid= process ID (long-running daemons only, via PMI_PROCESS flag)
version= tool version string
args= tool-specific arguments
archive= current archive base path
Mapping to:
pmimport.archive current archive base path
pmimport.version tool version string
pmimport.args tool-specific argument string
Add PMI_PROCESS flag (0x4) to pmiStart(): long-running import daemons
pass this flag to write their pid= to the status file and have it
removed on clean exit via pmiEnd(). One-shot tools (e.g. sadc invoked
by sa1) omit the flag so the status file persists between invocations.
pmdapmimport reads pid= and uses __pmProcessExists() to skip stale
status files left by daemons that exited uncleanly.
pmi_context gains a flags:16 bitfield (alongside the existing state:16)
to carry PMI_PROCESS across the lifetime of a context.
pcp-summary(1) displays sysstat/sadc with custom formatting; any other
active import tool is shown generically with aligned tool/modules/archive
lines, so new importers appear automatically without script changes.
The PMDA is included in local.conf so it is available in both live pmcd
mode and local context mode (pminfo -L), enabling pcp-summary and other
tools to report import tool status without requiring pmcd.
Signed-off-by: Nathan Scott <nathans@redhat.com>
natoscott
force-pushed
the
pcp-summary-pmimport
branch
from
8addeb0 to
3b895c1
Compare
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
2 participants
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.
Introduce a generic pmimport.{archive,version,args} metric set with an instance domain with each instance an active import tool identified by name.
pmdapmimport scans PCP_IMPORT_DIR on each fetch; each file is a status file written by an import tool containing key=value lines:
pid= process ID (long-running daemons only, via PMI_PROCESS flag)
version= tool version string
args= tool-specific arguments
archive= current archive base path
Mapping to:
pmimport.archive current archive base path
pmimport.version tool version string
pmimport.args tool-specific argument string
Add PMI_PROCESS flag (0x4) to pmiStart(): long-running import daemons pass this flag to write their pid= to the status file and have it removed on clean exit via pmiEnd(). One-shot tools (e.g. sadc invoked by sa1) omit the flag so the status file persists between invocations.
pmdapmimport reads pid= and uses __pmProcessExists() to skip stale status files left by daemons that exited uncleanly.
pmi_context gains a flags:16 bitfield (alongside the existing state:16) to carry PMI_PROCESS across the lifetime of a context.
pcp-summary(1) displays sysstat/sadc with custom formatting; any other active import tool is shown generically with aligned tool/modules/archive lines, so new importers appear automatically without script changes.
The PMDA is included in local.conf so it is available in both live pmcd mode and local context mode (pminfo -L), enabling pcp-summary and other tools to report import tool status without requiring pmcd.