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 NVME support to smartmon.py #113

Closed

Conversation

psi-4ward
Copy link

TBDC: NVME-Metrics are more like nvme_metrics.sh like than smartmon.py.

closes #110

Signed-off-by: Christoph Wiechert <wio@psitrax.de>
Signed-off-by: Christoph Wiechert <wio@psitrax.de>
smartmon.py Outdated Show resolved Hide resolved
Co-authored-by: Linus Heckemann <git@sphalerite.org>
Signed-off-by: Christoph Wiechert <wio@psitrax.de>
'--attributes', *device.smartctl_select()
)

# replace multiple occurrences of whitespaces with a singel whitespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in comment

@dswarbrick
Copy link
Member

The smartmon.py script has recently undergone non-trivial refactoring to use the official Prometheus client_python library, and this PR would need to be updated accordingly.

I still feel that the best place to implement NVMe metrics collection is in the nvme_metrics collector. Yes, smartctl now supports NVMe also. However, nvme-cli is the de facto tool for managing / querying NVMe devices, and goes a lot further than smartctl, since it also supports NVMeoF devices and vendor extensions which likely won't land in smartctl anytime soon (or ever).

If we implement NVMe metrics in two completely different collectors, this is going to be confusing for users. I expect that many setups will run both collectors anyway, which would essentially result in duplicated metrics if the smartmon collector also gathered NVMe metrics. I think it's best to leave smartctl to do what it is best at - handling ATA / SCSI / SAS drives and hardware RAID controllers.

The only way I would condone adding NVMe metrics to the smartmon collector would be if it were behind a CLI switch, and disabled by default.

@SuperQ WDYT?

@SuperQ
Copy link
Contributor

SuperQ commented May 23, 2023

If we implement NVMe metrics in two completely different collectors, this is going to be confusing for users.

We can provide more than one way to do things in this repo.

The only way I would condone adding NVMe metrics to the smartmon collector would be if it were behind a CLI switch, and disabled by default.

This is probably a good idea.

equinox0815 added a commit to spreadspace/node-exporter-textfile-collector-scripts that referenced this pull request Jul 18, 2023
This is an updated version of prometheus-community#113. By default NVMe devices will be ignored but gathering
of metrics for NVMEe devices can be enabled with the command-line option `--include-nvme`.

Signed-off-by: Christian Pointner <equinox@spreadspace.org>
equinox0815 added a commit to spreadspace/node-exporter-textfile-collector-scripts that referenced this pull request Jul 18, 2023
This is an updated version of prometheus-community#113. By default NVMe devices will be ignored but gathering
of metrics for NVMEe devices can be enabled with the command-line option `--include-nvme`.

Signed-off-by: Christian Pointner <equinox@spreadspace.org>
@dswarbrick
Copy link
Member

Closing due to inactivity and unresolved merge conflicts.

@dswarbrick dswarbrick closed this May 8, 2024
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.

smartmon.py won't collect nvme drive metrics
5 participants