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

Don't maintain two smartmon scripts #119

Open
varac opened this issue May 19, 2022 · 5 comments
Open

Don't maintain two smartmon scripts #119

varac opened this issue May 19, 2022 · 5 comments

Comments

@varac
Copy link

varac commented May 19, 2022

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
Copy link

iustin commented Mar 29, 2023

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
Copy link
Member

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
Copy link

iustin commented Mar 29, 2023

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:

  • write a python-based, json-parsing exporter from smartctl.
  • declare the old ones obsolete, and remove in another 2 versions.
  • distributions can start switching to the newer script at will.

What do you think?

@dswarbrick
Copy link
Member

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
Copy link

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?

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

No branches or pull requests

4 participants