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

fix: skip duplicate data source names in ValueList #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hoesler
Copy link

@hoesler hoesler commented Jun 28, 2019

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

@hoesler hoesler force-pushed the fix/duplicate_metrics branch 2 times, most recently from 35c2db7 to 7cc6c0d Compare July 1, 2019 10:06
@juliusv
Copy link
Member

juliusv commented Feb 10, 2020

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

Signed-off-by: Christoph Hösler <[email protected]>
@hoesler
Copy link
Author

hoesler commented Feb 10, 2020

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

juliusv commented Feb 10, 2020

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

hoesler commented Feb 11, 2020

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.

@stale stale bot added the stale label Apr 11, 2020
@juliusv
Copy link
Member

juliusv commented May 8, 2020

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

octo commented May 11, 2020

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

juliusv commented May 11, 2020

Thank you for the background, sounds good!

Separately I would suggest that #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

error "*** was collected before with the same name and label values"
3 participants