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

S.M.A.R.T visualization improvements #8310

Merged
merged 1 commit into from Mar 13, 2018

Conversation

Projects
None yet
4 participants
@enilfodne
Contributor

enilfodne commented Feb 28, 2018

This PR follows the changes merged in librenms/librenms-agent#164 and does as follows:

  • Change get_disks_with_smart to enum drives from rrd files instead of the Database.

While this seems some-what hackish, all the changes are limited to only one function and allows to cleanly transition from disk id to disk sn without rewrite/changes to polling.

  • Increase description length in graphs

The new changes introduced longer descriptions and since there was enough space, the length was increased where applicable.

  • Add SMART attr177, previously recorded, but not displayed

This is another variant of attr173, but it wasn't displayed and some SSDs use this instead of 173.

  • Change the multi-graph titles to better reflect their content

Some titles were extremely long and titles like Big 5 weren't exactly informative. I'm not sure these changes are good, hopefully someone might come up with a better version.

  • Change SMART attr numbers to a more human-friendly description

Accessibility changes to make graphs more readable

  • Minor text corrections

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

@laf laf closed this Mar 1, 2018

@laf laf reopened this Mar 1, 2018

@laf

This comment has been minimized.

Member

laf commented Mar 2, 2018

@enilfodne You might need to open a new one of these. I'm not sure why travis isn't starting here :(

@zapotah

This comment has been minimized.

Contributor

zapotah commented Mar 2, 2018

I tested the patch yesterday since this is something that ive wanted to have since long ago due to the very reasons you submitted the patch. It works flawlessly and is exactly what is needed.

@laf

This comment has been minimized.

Member

laf commented Mar 2, 2018

I've mailed travis-ci so they can take a look.

@laf

This comment has been minimized.

Member

laf commented Mar 5, 2018

Travis have said "log into travis-ci.org and click the "Sync account" button"

Can you do that please @enilfodne and I'll message them back when done.

@laf

This comment has been minimized.

Member

laf commented Mar 5, 2018

Done, but i must say this is one of the most unpleasant workarounds I've ever seen. Having to register for a service i have no use for, just to send a PR? I think there are better CI systems out there.

You normally don't have to. They normally auto create the user in there system by default but in this case it's not happened.

@enilfodne

This comment has been minimized.

Contributor

enilfodne commented Mar 9, 2018

Is there another issue with this PR? Should i rebase?

@laf

This comment has been minimized.

Member

laf commented Mar 9, 2018

The last few PRs going through are now ok so I suggest you rebase this from our master and push the changes.

I'd highly recommend not submitting pull requests from your master branch by the way for future reference.

* Change "get_disks_with_smart" to enum drives from rrd files
* Increase description length in graphs
* Add SMART attr177, previously recorded, but not displayed
* Change the multi-graph titles to better reflect their content
* Change SMART attr numbers to a more human-friendly description
* Minor text corrections
@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 9, 2018

The inspection completed: No new issues

@laf laf merged commit b70ff54 into librenms:master Mar 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

inetAnt added a commit to criteo-forks/librenms that referenced this pull request Mar 19, 2018

webui: S.M.A.R.T visualization improvements (librenms#8310)
* Increase description length in graphs
* Add SMART attr177, previously recorded, but not displayed
* Change the multi-graph titles to better reflect their content
* Change SMART attr numbers to a more human-friendly description
* Minor text corrections
@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.