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

Sanitize strings from /sys/class/power_supply #1984

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

SuperQ
Copy link
Member

@SuperQ SuperQ commented Mar 2, 2021

Avoid panic on invalid UTF-8 from /sys/class/power_supply by
sanitizing strings parsed from the kernel.

Fixes: #1979

Signed-off-by: Ben Kochie superq@gmail.com

@SuperQ
Copy link
Member Author

SuperQ commented Mar 2, 2021

CC @hhoffstaette, could you give this patch a try?

@SuperQ SuperQ force-pushed the superq/power_supply_panic branch from 966849a to 723e9d9 Compare March 2, 2021 15:52
@SuperQ
Copy link
Member Author

SuperQ commented Mar 2, 2021

Ok, so I am not liking the simple sanitize by dropping invalid runes. I'm going to figure out a better string sanitizer.

@hhoffstaette
Copy link
Contributor

Ok, so I am not liking the simple sanitize by dropping invalid runes. I'm going to figure out a better string sanitizer.

Why not? There is IMHO no other good way to handle this. Replacing them with other characters ('X' or '?') is bound to be misleading, and they are not likely to appear in the middle of the name.
Thanks for your effort. ❤️

@SuperQ
Copy link
Member Author

SuperQ commented Mar 2, 2021

What I would like is to replace the letters with something like \xc0, exactly how the panic prints it out. Finding the right way to convert a rune to an escaped ascii is not being obvious to me.

@hhoffstaette
Copy link
Contributor

hhoffstaette commented Mar 2, 2021

What I would like is to replace the letters with something like \xc0, exactly how the panic prints it out.

🤔 having escaped characters in the string is IMHO not helpful. I'd rather just have the value reduced to ASCII (printable characters) and be done, I don't see why this needs to be more complicated than necessary; remember that this will almost never trigger anyway.

@hhoffstaette
Copy link
Contributor

When I fix the patch to use ToValidUTF8(value, "") everything seems fine:

$curl -s http://localhost:9100/metrics | grep \^node_power
node_power_supply_capacity{power_supply="BAT0"} 96
node_power_supply_current_ampere{power_supply="ucsi-source-psy-USBC000:001"} 3
node_power_supply_current_ampere{power_supply="ucsi-source-psy-USBC000:002"} 0
node_power_supply_current_max{power_supply="ucsi-source-psy-USBC000:001"} 3.25
node_power_supply_current_max{power_supply="ucsi-source-psy-USBC000:002"} 0
node_power_supply_cyclecount{power_supply="BAT0"} 3
node_power_supply_energy_full{power_supply="BAT0"} 44.48
node_power_supply_energy_full_design{power_supply="BAT0"} 45
node_power_supply_energy_watthour{power_supply="BAT0"} 42.72
node_power_supply_info{power_supply="AC",type="Mains"} 1
node_power_supply_info{power_supply="ucsi-source-psy-USBC000:001",type="USB",usb_type="C [PD] PD_PPS"} 1
node_power_supply_info{power_supply="ucsi-source-psy-USBC000:002",type="USB",usb_type="[C] PD PD_PPS"} 1
node_power_supply_info{capacity_level="Normal",manufacturer="LGC",model_name="LNV-5B10W13894",power_supply="BAT0",serial_number="3482",status="Unknown",technology="Li-poly",type="Battery"} 1
node_power_supply_online{power_supply="AC"} 1
node_power_supply_online{power_supply="ucsi-source-psy-USBC000:001"} 1
node_power_supply_online{power_supply="ucsi-source-psy-USBC000:002"} 0
node_power_supply_power_watt{power_supply="BAT0"} 0
node_power_supply_present{power_supply="BAT0"} 1
node_power_supply_voltage_max{power_supply="ucsi-source-psy-USBC000:001"} 20
node_power_supply_voltage_max{power_supply="ucsi-source-psy-USBC000:002"} 0
node_power_supply_voltage_min{power_supply="ucsi-source-psy-USBC000:001"} 5
node_power_supply_voltage_min{power_supply="ucsi-source-psy-USBC000:002"} 0
node_power_supply_voltage_min_design{power_supply="BAT0"} 11.1
node_power_supply_voltage_volt{power_supply="BAT0"} 12.197
node_power_supply_voltage_volt{power_supply="ucsi-source-psy-USBC000:001"} 5
node_power_supply_voltage_volt{power_supply="ucsi-source-psy-USBC000:002"} 0

..works for me? ¯\_(ツ)_/¯

@SuperQ
Copy link
Member Author

SuperQ commented Mar 2, 2021

Yea, I suppose. My usual preference is to preserve as much data as possible. I dislike silently eating data.

@SuperQ SuperQ force-pushed the superq/power_supply_panic branch from 723e9d9 to d197a2f Compare March 2, 2021 19:05
@SuperQ
Copy link
Member Author

SuperQ commented Mar 2, 2021

That's weird, this shouldn't impact the scrape duration that much. Is that for node_scrape_collector_duration_seconds{collector="powersupplyclass"}?

@hhoffstaette
Copy link
Contributor

That's weird, this shouldn't impact the scrape duration that much. Is that for
node_scrape_collector_duration_seconds{collector="powersupplyclass"}?

I deleted my comment. The picture showed the complete scrape time (up by ~50ms), but then I realized that the additional time was caused by the powersupply class collector (regardless of sanitizing). procfs/sysfs values are pulled on-demand and talking to the HW (in this case reading voltages etc. from the battery/sensors via ACPI and whatnot) is just slow.

Avoid panic on invalid UTF-8 from /sys/class/power_supply by
sanitizing strings parsed from the kernel.
* Add a broken string to the test fixtures.

Fixes: #1979

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ force-pushed the superq/power_supply_panic branch from 6deb047 to 23e5b24 Compare March 3, 2021 17:05
Copy link
Member

@discordianfish discordianfish left a comment

Choose a reason for hiding this comment

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

LGTM

@SuperQ SuperQ merged commit 40e9d29 into master Mar 3, 2021
@SuperQ SuperQ deleted the superq/power_supply_panic branch March 3, 2021 19:38
SuperQ added a commit that referenced this pull request Mar 5, 2021
* [BUGFIX] Handle errors from disabled PSI subsystem #1983
* [BUGFIX] Sanitize strings from /sys/class/power_supply #1984
* [BUGFIX] Silence missing netclass errors #1986

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ mentioned this pull request Mar 5, 2021
SuperQ added a commit that referenced this pull request Mar 5, 2021
* [BUGFIX] Handle errors from disabled PSI subsystem #1983
* [BUGFIX] Sanitize strings from /sys/class/power_supply #1984
* [BUGFIX] Silence missing netclass errors #1986

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ mentioned this pull request Mar 5, 2021
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* [BUGFIX] Handle errors from disabled PSI subsystem prometheus#1983
* [BUGFIX] Sanitize strings from /sys/class/power_supply prometheus#1984
* [BUGFIX] Silence missing netclass errors prometheus#1986

Signed-off-by: Ben Kochie <superq@gmail.com>
oblitorum pushed a commit to shatteredsilicon/node_exporter that referenced this pull request Apr 9, 2024
* [BUGFIX] Handle errors from disabled PSI subsystem prometheus#1983
* [BUGFIX] Sanitize strings from /sys/class/power_supply prometheus#1984
* [BUGFIX] Silence missing netclass errors prometheus#1986

Signed-off-by: Ben Kochie <superq@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic: label value is not valid UTF-8
3 participants