add power units for pmda-denki metrics#2641
Conversation
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesPower unit metadata for denki metrics
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 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 `@src/pmdas/denki/denki.c`:
- Around line 897-900: The bat.energy_now metric definition in denki.c still
uses dimensionless PMDA_PMUNITS(0,0,0,0,0,0) instead of the required Wh
annotation. Update the PMDA_PMUNITS value for the bat.energy_now entry in the
metric table to use the same watt-hour capacity unit convention as the related
battery metrics, keeping the change localized to that metric definition.
- Around line 892-896: The RAPL metric definitions in denki.c are using
PM_UNIT_POWER/PM_POWER_W for denki.rapl.sysfs and denki.rapl.msr, but these
counters are cumulative energy readings rather than power. Update the unit
metadata in the RAPL entries within the PMDA_EXTRAUNITS/PMDA_PMID definitions to
use a unitless or more appropriate descriptor, or add an explicit comment in the
RAPL section explaining any intentional approximation. Keep the change localized
to the RAPL metric table entries so the symbols denki.rapl.sysfs and
denki.rapl.msr remain easy to identify.
🪄 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: fc0a8a5d-c27b-45ba-8efa-b16cc8eece6d
📒 Files selected for processing (1)
src/pmdas/denki/denki.c
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pmdas/denki/denki.c (1)
1274-1281: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRAPL labels incorrectly describe energy counters as "watt".
RAPL_SYSFS_INDOMandRAPL_MSR_INDOMbackrapl.sysfs/rapl.msr, which arePM_SEM_COUNTERcumulative energy readings (see metrictab lines 892-896, still dimensionless). Adding{"units":"watt"}here mislabels them as instantaneous power, which is misleading to consumers of these labels — the prior discussion on this PR converged on using "joules"/"millijoules" for these counters instead, mirroring theBAT_ENERGYNOW_INDOM→"watt hours"pattern used just below.🐛 Proposed fix
case RAPL_SYSFS_INDOM: pmdaAddLabels(lpp, "{\"indom_name\":\"rapl sysfs\"}"); - pmdaAddLabels(lpp, "{\"units\":\"watt\"}"); + pmdaAddLabels(lpp, "{\"units\":\"joules\"}"); break; case RAPL_MSR_INDOM: pmdaAddLabels(lpp, "{\"indom_name\":\"rapl msr\"}"); - pmdaAddLabels(lpp, "{\"units\":\"watt\"}"); + pmdaAddLabels(lpp, "{\"units\":\"joules\"}"); break;Based on learnings, "RAPL sysfs and MSR ... are cumulative energy counters, not power readings ... follow the existing convention ... state the real unit (e.g., joules/millijoules) via pmdaAddLabels/metadata rather than pmUnits."
🤖 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/pmdas/denki/denki.c` around lines 1274 - 1281, The RAPL label metadata in the RAPL_SYSFS_INDOM and RAPL_MSR_INDOM cases is misreporting cumulative energy counters as “watt”; update the pmdaAddLabels entries in this switch so the labels describe the actual energy unit instead (for example joules or millijoules), matching the existing convention used for BAT_ENERGYNOW_INDOM and the PM_SEM_COUNTER semantics. Keep the change localized to the denki label setup around these RAPL case labels and ensure the metadata reflects energy rather than instantaneous power.Source: Learnings
🤖 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.
Duplicate comments:
In `@src/pmdas/denki/denki.c`:
- Around line 1274-1281: The RAPL label metadata in the RAPL_SYSFS_INDOM and
RAPL_MSR_INDOM cases is misreporting cumulative energy counters as “watt”;
update the pmdaAddLabels entries in this switch so the labels describe the
actual energy unit instead (for example joules or millijoules), matching the
existing convention used for BAT_ENERGYNOW_INDOM and the PM_SEM_COUNTER
semantics. Keep the change localized to the denki label setup around these RAPL
case labels and ensure the metadata reflects energy rather than instantaneous
power.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b26fdd9-391d-4837-a5f5-948349abbb0a
📒 Files selected for processing (1)
src/pmdas/denki/denki.c
|
@christianhorn sorry, I disagree, we don't need Wh, the canonical way of specifying this now is which gets reported like this so not quite Wh, but close enough I think. Adding another slice of "extra" units (like for energy) is a big deal QA and code wise, and I'd like to keep a tight control over the number we're willing to support. |
|
Nice, works for me. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pmdas/denki/denki.c (1)
1276-1280: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse an energy unit for the RAPL indom labels.
denki.rapl.sysfsanddenki.rapl.msrare cumulative energy counters (PM_SEM_COUNTERwith dimensionlesspmUnits), so{"units":"watt"}is misleading here. Label them with the actual energy unit (joules/millijoules) instead.🤖 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/pmdas/denki/denki.c` around lines 1276 - 1280, The RAPL indom labels in denki.c are using the wrong unit string for cumulative energy counters. Update the pmdaAddLabels calls in the RAPL branch handling RAPL_MSR_INDOM (and the corresponding sysfs case nearby) to use an energy unit label such as joules or millijoules instead of watts, keeping the existing indom_name labels intact.Source: Learnings
🤖 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.
Outside diff comments:
In `@src/pmdas/denki/denki.c`:
- Around line 1276-1280: The RAPL indom labels in denki.c are using the wrong
unit string for cumulative energy counters. Update the pmdaAddLabels calls in
the RAPL branch handling RAPL_MSR_INDOM (and the corresponding sysfs case
nearby) to use an energy unit label such as joules or millijoules instead of
watts, keeping the existing indom_name labels intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2121c4c0-24ec-4eda-933f-ce4fdfa02117
📒 Files selected for processing (1)
src/pmdas/denki/denki.c
kmcdonell
left a comment
kmcdonell
left a comment
There was a problem hiding this comment.
LGTM, thanks @christianhorn
2 participants
Adding the new power units.
For metric denki.bat.energy_now, unit Watthour (Wh) would be appropriate, capacity. Should we add that?
Once could argue "it's just power delivered over a time".. but I think here in this instance we can not use that property, and would vote that we add a unit Wh.