prometheus-community / node-exporter-textfile-collector-scripts

Scripts for node-exporter's textfile collector
Apache License 2.0
508 stars 189 forks source link

Don't maintain two smartmon scripts #119

Open varac opened 2 years ago

varac commented 2 years ago

I'm confused by the current state that this repo maintains two smartmon scripts (one shell, one pythons script). I'd like to propose to decide on one that should be maintained, and remove/deprecate the other, to bundle the efforts and also not confuse users (which one should I use ? what are the differences ?). Or at least indicate very clearly which one is/will get deprecated. Debian packages (still) use the shell script by default. The last commit to the shell script was 2 years ago, the last commit for the python script 1 year ago.

iustin commented 1 year ago

In the meantime, there have been a few more commits to the shell one. And the python one is not using proper exporter library to generate the output, but instead does manual print() statments. And neither the shell one nor the python one are using the json output from smartctl, relying instead on parsing the text output :(

It looks like it would be time to write a new python (or whatever) script from scratch, that is more clean/modern.

dswarbrick commented 1 year ago

And neither the shell one nor the python one are using the json output from smartctl, relying instead on parsing the text output :(

IIRC, that was due to the fact that smartctl only supports JSON output from v7.x onwards (again, IIRC).

I would definitely be in favour of requiring the use of official Prometheus python library for all the collector scripts here however. Too often I see homebaked implementations choke on quoting or non-ASCII chars, and the quality of implementation is quite variable.

iustin commented 1 year ago

Thanks for the reply. The argument about JSON (made in 2020, as I saw yesterday, but can't find that anymore) was maybe valid 3 years ago, but we're now in 2023. Would it make sense to move now to require modern smartctl, and if older distributions care about new node exporter scripts, they'd have to backport those, so newer smartctl would also be an option?

I ask as there seem to be 6 open pull requests mentioning "smart", and I'm not sure if it's worth trying to maintain two different version that do text parsing of unstable output.

I don't have much free time, but I'd be tempted to write a more modern parser (for my personal use at least), because the current one does have some drawbacks that are relatively easy to fix, but that would just add to the obsolete codebase, so I'm not tempted to do so…

So:

What do you think?

dswarbrick commented 1 year ago

This repo doesn't really have "versions" per se. We could move the legacy collectors to an "attic" directory perhaps, which distros could omit from their packaged releases (e.g. Debian's prometheus-node-exporter-collectors). Being that this is a git repo though, with full version history, I'm not sure if even that would be necessary. If somebody wants the old collectors, they can use an older git snapshot. I doubt that any distros are still publishing new releases with pre-7.x smartmontools.

The main requirement is that a new / replacement collector did not break backwards compatibility in terms of metric names and labels. Unit tests would also be really nice, i.e. include some JSON test fixtures and write the collector in a modular way such that it can be tested against them.

bjornfor commented 2 months ago

After learning that smartmon.py doesn't expose SMART ID 11 (Calibration Retry Count), I migrated to https://github.com/prometheus-community/smartctl_exporter. (That project already parses JSON data from smartctl.)

Why not deprecated both smartmon.{sh,py} and suggest using smartctl_exporter?