-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Update smart input plugin to support more drive types #5765
Conversation
// Power Cycles: 472 | ||
nvmePowerCycleAttr = regexp.MustCompile("^Power Cycles:\\s+(.*)$") | ||
// Power On Hours: 6,038 | ||
nvmePowerOnAttr = regexp.MustCompile("^Power On Hours:\\s+(.*)$") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is okay for now, but I think in the future we shouldn't add any additional fields until we have a more efficient way to parse the output. We don't want to end up running 20+ regex per line. Maybe we will classify the line using a single regex and then parse further once we know which pattern to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this isn't technically an additional field, just the way to parse it from a different device type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is more about the way we are parsing the output, and how it will scale as we add additional regular expressions. Right now it is essentially number-of-lines * number-of-regex
, and for each field/attr we need another regex. It should be possible to change this to number-of-lines * 2
or better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too found that disgusting. I had thought about refactoring that part, but felt the speed difference wouldn't be worth the effort. Maybe we should really evaluate that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is probably fine for this pull request, but I don't think we can continue to add more regex with the current strategy for much longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any Percentage Used included. I suppose this is one of the more important attributes to monitor. I know the regex way is not the way to go and the amount of attributes should be kept to a minimum, but could this very important attribute be added as well? Especially in high-write environments this is a vital measurement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more won't be the end of the world, can you open a new feature request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sg-jorrit I intentionally didn't include percentage used because that is already being collected (and seems to fit better) in the disk
input plugin, but I suppose there's no harm in collecting the same thing in multiple places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, but might be wrong, that the percentage used here is the SSD wear: how much of the expected write cycle of the drive has been used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now that makes sense
Resolves #5740
Resolves #4720
Resolves #5790
Resolves #4707
---
as threshold valueResolves #4169