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

Fix cputemp rrd #8024

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Fix cputemp rrd #8024

wants to merge 3 commits into from

Conversation

awilkin87
Copy link

This PR addresses Issue #8023, where the CPU temperature graphs stop working after the the recent refactoring.

-Add sysctl flag to ignore unknown oids
-Remove Celsius "C" symbol from output
@awilkin87 awilkin87 marked this pull request as ready for review October 29, 2024 01:15
@AdSchellevis
Copy link
Member

@awilkin87 can you dump the output of both commands?

/sbin/sysctl -n dev.cpu.0.temperature hw.acpi.thermal.tz0.temperature hw.temperature.CPU
/sbin/sysctl -ni dev.cpu.0.temperature hw.acpi.thermal.tz0.temperature hw.temperature.CPU

@awilkin87
Copy link
Author

Sure, here are outputs and exit codes for my system:

root@opnsense:~ $ /sbin/sysctl -n dev.cpu.0.temperature hw.acpi.thermal.tz0.temperature hw.temperature.CPU
54.0C
sysctl: unknown oid 'hw.acpi.thermal.tz0.temperature'
sysctl: unknown oid 'hw.temperature.CPU'
root@opnsense:~ $ echo $?
2
root@opnsense:~ $ /sbin/sysctl -ni dev.cpu.0.temperature hw.acpi.thermal.tz0.temperature hw.temperature.CPU
49.0C
root@opnsense:~ $ echo $?
0

@AdSchellevis
Copy link
Member

@awilkin87 can you revert the changes to Stats/Temperature.php and try again with only the filename shellCmd() should take care of the output.

@awilkin87
Copy link
Author

I will test it later tonight, but I believe the -i flag is needed since shellCmd() requires a zero exit code in order to return the output.

@AdSchellevis
Copy link
Member

@awilkin87 looks like you're right, only strip the sed in that case and we should be good. thanks!

@awilkin87
Copy link
Author

awilkin87 commented Oct 30, 2024

@AdSchellevis I believe the sed is required as well. After removing it, the table/graph view of cputemp is reporting zero for temperature readings. Using rrdtool dump /var/db/rrd/system-cputemp.rrd, those readings are stored as NaN.

image

Just to be sure, I reset the rrd. While the file was recreated, it did not contain any temperature readings; only NaN.

AdSchellevis added a commit that referenced this pull request Oct 31, 2024
@AdSchellevis
Copy link
Member

@awilkin87 can you try 8e3b4b7 ? the data needs to be cleansed indeed, but better keep it in php to prevent changing multiple shell commands.

Remove Celsius symbol from temperature output in PHP code instead of
using sed.
@awilkin87
Copy link
Author

@AdSchellevis I tested with preg_replace instead of sed. It works as expected.

@AdSchellevis
Copy link
Member

@awilkin87 thanks for confirming, I'll ask @fichtner to slip the change into the next release.

fichtner pushed a commit that referenced this pull request Nov 4, 2024
…ysctl collection issue. partially merges #8024

(cherry picked from commit 8e3b4b7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants