Don't return success for collectors that have no data to return

[SuperQ]

We currently return true in node_scrape_collector_success for collectors that are enabled, but the underlying kernel feature is not. This is confusing for users because they can't tell that the collector returned no data.

This is especially problematic for Prometheus data model where lack of data is not an alarm.

We previously considered adding a separate metric for "enabled but empty" status, but decided against it due to the additional metrics load from the already heavy (> 500) metrics returned by this exporter.

Proposal

Collectors that are enabled but fail to gather any data should also return false for node_scrape_collector_success.

[daenney]

I'm wondering if Update should be changed to return (ok bool, err error) instead or something like that. An error might not necessarily occur, like some precondition fails, but that would still let us signal that "hey this didn't work" and have node_scrape_collector_success return the right thing.

In that same spirit, it might be nicer if we can signal not meeting certain preconditions, like a kernel feature, from New...Collector() instead, so we don't have to bubble this up from Update() and can avoid calling that entirely. Right now if you return an error from New...Collector() it'll terminate the node exporter.

One other way might be to add a Precondition method (or Validate or whatever naming) to the Collector interface that is called after New...Collector() but before Update(), and if that doesn't return true not proceed.

[SuperQ]

Those things would help, but this is mostly about deciding if the Precondition/OK should make the success status be not ok for no data.

[AvengerMoJo]

@discordianfish thanks for adding me into the loop. However I don't understand our solution for enabled modules which may not have available data e.g. kernel features not available. Do we need someone to fix this by adding a "Precondition" function or update existing workflow to stop requesting data if return false in Update? Thanks

[brian-brazil]

A collector hasn't failed merely because it has returned no data, that's a perfectly valid response in many cases. Making a fundamental change in the definition of a metric from "if this is 0 there's definitely something wrong" to "if this is 0 maybe something meant wrong, maybe it didn't - it depends on your setup" classifies as a breaking change, and any existing alerts on this metric will start spamming users.

Thus semantics indicate towards a new metric name such as node_scrape_collector_no_data (or more generally the number of returned samples if you want to graph that while you're at it.).

This is confusing for users because they can't tell that the collector returned no data.

Anyone know knows which collectors to enable/disable can also create alerts using https://www.robustperception.io/absent-alerting-for-scraped-metrics, which doesn't require changes to metrics.

[SuperQ]

We originally made the node_exporter not fail full scrapes for single failed collectors. Now that we have things setup so that collectors don't error on no data, we could change the behavior to fail the whole scrape in the case of a single collector error.

This would mean we could eliminate the whole collector success metric. But it would mean users need to look at the log to see which collector is causing the problem.

[brian-brazil]

We were talking at one point about exposing 5xx output from failed scrapes on the Prometheus side.

[hoffie]

We originally made the node_exporter not fail full scrapes for single failed collectors. Now that we have things setup so that collectors don't error on no data, we could change the behavior to fail the whole scrape in the case of a single collector error.

I think this takes control about prioritization and choice of tradeoffs away from users.

For example, I currently have a generic alert on node_scrape_collector_success != 0 to catch any collector problems. It's not actually that seldom that it triggers. The two most common reasons for me are:

1. cpufreq collector fails (because the kernel seems to return strange data on specific hardware/firmware occasionally; haven't investigated deeper so far)
2. Bad textfile collector data.

Both are things which should be fixed. Neither of them are critical for me, though, and fixing them may take time (debugg hardware/firmware/kernel) and may need other teams to fix (bad textfile). node_exporter going down completely because of such an issue, would make it critical for me though, without having a way to change that.

I really like the idea of introducing a no_data metric (or a metric with the number of collected "items"). This way, the user stays in control. They can choose to alert on it or they can choose to ignore it (as it is now). Alerting can even be very fine-grained such as "alert on no_data{collector="zfs"} only on storage servers" etc.

[SuperQ]

Thanks @hoffie. These are exactly the kinds of user use cases that I'm interested in.

One option, is we could make a flag to switch the behavior. I'm not a big fan of excessive control flags, because it makes support of the exporter, and behavior less predictable.

But we could do something like --collector.scrapes_fatal, enabled by default, that would do the new behavior. But if you use --no-collector.scrapes_fatal, the current behavior would be preserved. That way most users would see the "best practice" behavior, but allow the existing behavior for orgs that have issues with flaky data.

As the maintainer, I would like to get more info on why collectors fail, so having them hard fail is better for me. Getting more feedback so we can make the collectors more reliable is a good thing. 😄

[discordianfish]

Not sure why we suddenly discussing failing the scrape completely. I don't think we should do that.

Let's do:

• (ok bool, err error) signature
• node_scrape_collector_success = 0 on error, else 1
• node_scrape_collector_active = 0 on !ok, else 1

If we're on it, we could also node_scrape_collector_enabled - so we have all states of a collector covered.

[brian-brazil]

Failing the scrape is consistent with our exporter guidelines, as that's the easiest for users to deal with as existing up alerts will cover it.

If we're on it, we could also node_scrape_collector_enabled - so we have all states of a collector covered.

We've already enough cardinality from these metrics without adding another one, which seems to have more a fleet management use case.