Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add power consumption metric to power supply monitoring module #17329

Merged
merged 9 commits into from
Apr 22, 2024

Conversation

eyusupov
Copy link
Contributor

@eyusupov eyusupov commented Apr 5, 2024

Summary

Add a new metric for battery discharge rate (/sys/class/battery/<BAT>/power_now).
image

Test Plan
  • Single battery when charging
  • Single battery when discharging
Additional Information

There is a new metric that allows to track the machine (usually laptop) power consumption.
While it is possible to gauge this using existing power capacity metric, it is not very granular does not show the rate of discharge very clearly.
With this change it should possible to correlate other metrics with power consumption.

There seem to be some issues with the sections (BAT0 has appeared as the section heading and capacity metric is displayed under charge section). I think the reason might be in the dashboard code which source is not included in the repo.

@CLAassistant
Copy link

CLAassistant commented Apr 5, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/docs area/web area/collectors Everything related to data collection collectors/proc area/metadata Integrations metadata labels Apr 5, 2024
@ilyam8
Copy link
Member

ilyam8 commented Apr 8, 2024

Hey, @eyusupov 👋 Thanks for the contribution!

There seem to be some issues with the sections

The subsection name is the 4th parameter in rrdset_create_localhost(). I fixed them in #17338.

@ilyam8
Copy link
Member

ilyam8 commented Apr 8, 2024

@eyusupov I merged #17338, please

  • Rebase/resolve merge conflicts.
  • Update the screenshot in the PR description.

@eyusupov
Copy link
Contributor Author

eyusupov commented Apr 8, 2024

I rebased my branch and updated the section (and priority) for the new metric, but it seems like it has stopped displaying completely now.

@eyusupov
Copy link
Contributor Author

eyusupov commented Apr 8, 2024

image

@eyusupov
Copy link
Contributor Author

eyusupov commented Apr 8, 2024

This is the new screenshot:
image

I was expecting power to be showing after charge due to the priority NETDATA_CHART_PRIO_POWER_SUPPLY_POWER. Am I missing or misunderstanding something? Showing it the way it is is also fine for me, but the code should reflect it then.

@ilyam8
Copy link
Member

ilyam8 commented Apr 8, 2024

@eyusupov I see that the priority is NETDATA_CHART_PRIO_POWER_SUPPLY_CAPACITY for charge, energy, and voltage.

You can add priority to struct ps_property and use it later when creating charts.

@eyusupov
Copy link
Contributor Author

eyusupov commented Apr 8, 2024

Updated the priorities, but for some reason energy is still reported before power

image

}
}

if (likely(prop->fd != -1))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest instead to test prop->fd != -1 again, you write it as an else for previous block.


if (likely(prop->fd != -1))
{
ssize_t r = read(prop->fd, buffer, 30);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you create a macro (define) above, please, do not forget to change here.

@eyusupov
Copy link
Contributor Author

eyusupov commented Apr 8, 2024

I prepared another PR with the same change, where I have unified the code for single and multi-dimensional properties #17346. Let me know if you think it's better or if I should finish that one instead.

@eyusupov eyusupov force-pushed the add-power-consumption branch 2 times, most recently from 0860237 to 2d1b0b9 Compare April 10, 2024 15:16
@eyusupov
Copy link
Contributor Author

Screenshot from the latest test run:
image

@thiagoftsm
Copy link
Contributor

I prepared another PR with the same change, where I have unified the code for single and multi-dimensional properties #17346. Let me know if you think it's better or if I should finish that one instead.

Hello,

I am sorry for the delay, I got some days off.
I suggest we finish this first, so you can st the other as draft for a while.

@thiagoftsm
Copy link
Contributor

@ilyam8 I cannot see on my host the charts, can you see on yours?

Copy link
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not have the file power_now on my environments, but the plugin is running as expected with these changes. I am going to approve waiting for other reviewers that have conditions to see the chart.
Thank you @eyusupov !

@thiagoftsm thiagoftsm enabled auto-merge (squash) April 22, 2024 11:29
@thiagoftsm thiagoftsm merged commit 632f7a3 into netdata:master Apr 22, 2024
150 of 153 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/collectors Everything related to data collection area/docs area/metadata Integrations metadata area/web collectors/proc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants