fix: skip duplicate data source names in ValueList

[hoesler]

The DSName of each Value in a ValueList is appended to the metric name. Data source names might appear multiple times. As these are not part of the identifier string, however, and therefore not part of the map key, this might lead to duplicate metric names and the following error from prometheus: ... was collected before with the same name and label values.

This PR adds a condition to the Collect function which skips metrics with a DSName that has already been processed.

fixes #68

[juliusv]

@hoesler Whoops sorry, I never saw this. Could you rebase this on top of latest master, if it's still applicable?

[hoesler]

@hoesler Whoops sorry, I never saw this. Could you rebase this on top of latest master, if it's still applicable?

I think it is. It took me some time, however, to remember what was causing the issue. I will add some more details to the description.

[juliusv]

@hoesler Thanks! So I don't know much about collectd (especially anymore) and kind of became the maintainer here by accident :) Just want to make sure I understand correctly: if we get two metrics with the same data source name from collectd, are they strictly redundant or does one have a different value than the other, so we would lose information by dropping one? If the latter, is there a different way we should be differentiating between them if we keep both?

[hoesler]

I don't know much about collectd

So, that makes two of us ;)

I think you're probably right that we might loose information, and by looking at my changes after so long it actually feels more like a workaround than a fix. The main problem is that a collectd value list contains multiple values, but the dsname might not be unique inside such a list. I don't know if the exporter could distinguish these metrics by some other factor. Currently, I am no longer in an urgent need for a fix, but maybe I can find some spare time to look into this again.

[juliusv]

@hoesler Ok, happy to let this sit here for a while. Maybe someone else feels motivated to look into it, or you get to it again at some point :)

[octo]

Heya, collectd maintainer here.

Specifying one data set (metric) with two data sources (values) of the same name is an error. You can reject those and don't need to handle them gracefully.

I realized that this is not documented properly and opened collectd/collectd#3458 to fix this. I'm also happy to add some sanity checking to collectd.org/api, so that the code here would become something like this:

if err := vl.Check(); err != nil {
  return fmt.Errorf("value list fails sanity check: %w", err)
}

What do you think?

Best regards, —octo

[juliusv]

Thank you for the background, sounds good!

Separately I would suggest that https://github.com/prometheus/collectd_exporter/pull/95 marks the last release of the collectd exporter and we mark it as deprecated in favor of the upstream Prometheus support in collectd now.