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

Skip vendor-specific statistics that lead to duplicate metric labels #28

Merged
merged 2 commits into from
Oct 20, 2022

Conversation

lahwaacz
Copy link
Contributor

@lahwaacz lahwaacz commented Oct 3, 2021

fixes #3

@baryluk
Copy link

baryluk commented Oct 8, 2021

Thanks. This fixed issues on Samsung Exos X16 for me.

And I think it is a good fix. There is no point of collecting the metrics for these statistics, which just would consume space, and never be used.

Lets merge it please. It can be always improved in the future to make sense of these metrics other way.

@kfox1111
Copy link

kfox1111 commented Oct 8, 2021

Nice. We just ordered some Samsung Exos X16's. So good to know that this is a problem / there is a fix for it. Thanks!

Copy link

@vagifzeynalov vagifzeynalov left a comment

Choose a reason for hiding this comment

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

doesn't look right to me.
"avoid duplicates" doesn't mean "ignore all of them"

@vagifzeynalov
Copy link

#32 - alternative solution

@lahwaacz
Copy link
Contributor Author

lahwaacz commented Oct 9, 2021

Thanks. This fixed issues on Samsung Exos X16 for me.

Just for the record, it's Seagate, not Samsung.

"avoid duplicates" doesn't mean "ignore all of them"

Do you have some use case for the Vendor Specific Statistics? As @baryluk said, it does not make sense to collect metrics that we can't make sense of.

@vagifzeynalov
Copy link

No, I personally don't have any use cases for those values.
But removing them also doesn't look good to me. Who knows how they could be used? 😁
As I said in my PR, I'll try to look at them closer...

@baryluk
Copy link

baryluk commented Oct 10, 2021

But removing them also doesn't look good to me. Who knows how they could be used? grin

For wasting storage, memory, CPU cycles and energy....

So, they should be removed.

Good metrics, should be actually useful and designed to be so. I would say all of the smart statistics (not attributes and other stuff) should be behind the command line flag, and be disable by default.

@vagifzeynalov
Copy link

For wasting storage, memory, CPU cycles and energy....

Nowadays hardware is cheap, so it is not an argument to save few megabytes.

Good metrics, should be actually useful and designed to be so. I would say all of the smart statistics (not attributes and other stuff) should be behind the command line flag, and be disable by default.

One thing to check something from the command line, another thing to have historical data and check it only if/when needed.
Perhaps there could be a configuration file where all kinds of metrics could turned ON/OFF.

@retrry
Copy link

retrry commented Sep 2, 2022

Can we get a resolution on this? It is better to drop these statistics, than to try to collect them and fail. smartctl_exporter currently is unusable on some of the drives because of this issue.

@Stogas
Copy link

Stogas commented Sep 6, 2022

@vagifzeynalov Would you consider adding an "ignore duplicates" runtime flag?

You could leave it to default to the current behavior, while allowing affected users who don't mind losing vendor-specific data to easily drop it without recompiling their own exporter

@onedr0p
Copy link

onedr0p commented Sep 17, 2022

@SuperQ any chance we can see some effort on getting this merged?

@k0ste
Copy link
Contributor

k0ste commented Oct 14, 2022

@lahwaacz can you rebase this PR? This should trigger CI
Thanks!

@lahwaacz
Copy link
Contributor Author

@k0ste rebased

@SuperQ
Copy link
Contributor

SuperQ commented Oct 14, 2022

This needs a DCO sign-off. You can use git commit -s --amend to add it.

fixes #3

Signed-off-by: Jakub Klinkovský <j.l.k@gmx.com>
smartctl.go Outdated Show resolved Hide resolved
Signed-off-by: Ben Kochie <superq@gmail.com>

Signed-off-by: Ben Kochie <superq@gmail.com>
@SuperQ SuperQ merged commit b832e55 into prometheus-community:master Oct 20, 2022
@k0ste k0ste mentioned this pull request Nov 2, 2023
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.

server returned HTTP status 500 Internal Server Error
9 participants