Smartctl Exporter

[akremer]

Pull Request (PR) description

This PR adds the smartctl prometheus exporter (https://github.com/prometheus-community/smartctl_exporter)

The exporter is largely based on similar prometheus-community exporters, and has been tested on both Ubuntu and Darwin systems.

[TheMeier]

Why set the defaults for all these parameters to undef and then override them here. Instead remove the Optional and set the default value directly in the parameter definition.

[akremer]

That's not quite what's happening here. The intent here was to support smartctl's optional parameters that, when passed, require them to be passed with specific named flags.

To be clear about the section you highlighted:

If the user sets a smartctl_path, then pass it using --smartctl.path=
If the user does not set a smartctl_path, then don't pass anything.

In other words, smartctl_path remains an optional flag. If I removed Optional and set the default value in the parameter definition per your suggestion, I would be sending invalid (empty) flags to the exporter.

I tried to follow the pattern set in other exporters in this project. See e.g. this:

puppet-prometheus/manifests/openvpn_exporter.pp

Lines 95 to 98 in 44e45dd

	$_status_paths = $status_paths ? {
	undef => '',
	default => "-openvpn.status_paths=${join(unique($status_paths), ',')}",
	}

If you have another preferred pattern to use in instances like this, let me know.

[TheMeier]

right, i overlooked that.

The usual pattern is to just pass in extra_options just search for it in the code multiple example

That being said, I already started a discussion about moving all the exporters out of this module. I am quite reluctant to to the ever growing list of exporters in this module.

So if you want to have exporter specific parameters, maybe a dedicated module is a better option. It could still be depending on puppet-prometheus and re-use all of the shared bits.
But I that's just my opinion at the moment

[TheMeier]

REFERENCE.md needs to be updated, see https://voxpupuli.org/docs/how_to_run_tests/#referencemd-update

[TheMeier]

Please adapt the tests

[TheMeier]

Great, LGTM.
But I am still not very happy with those repetitive use of selectors and adding a new exporter in general. So I let someone else approve that

[akremer]

I'll comment first on the issue I actually have input on :)

Unlike most prom exporters, the smartctl exporter requires certain flags to run properly at all, which is why I included the named options and didn't just leave it up to the user to figure out via extra_options. While using extra_options certainly makes for prettier code, it puts a larger implementation and research onus on the user. I tried to err on the user-friendly side on this issue.

On the other issue: I see the maintenance burden of including all these exporters in this module, but since exporters don't update that often, having them included here sure is handy. That said, it wouldn't be a huge deal to spin out into a (sub)module.

[TheMeier]

Is there a set of sensible defaults for at least some of those options?

[akremer]

The exporter itself sets those defaults already so this code doesn't need to set anything special if the user doesn't want to override anything. The idea here was that if the user does wish to override the defaults, they can do so by setting the named variables offered to them in this module. The reason they are there is that in most instances, the user will need to override the defaults to get this exporter running properly.

[TheMeier]

Well if there is already sensible defaults in place I'd say extra_options i sufficient.