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

feature: Added rrd step conversion script #6081

Merged
merged 4 commits into from Mar 11, 2017

Conversation

Projects
None yet
7 participants
@laf
Member

laf commented Mar 3, 2017

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.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

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

Works for me :)

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 3, 2017

Thank you for submitting a PR @laf! We have found the following @geordish based on the history of these files to review this PR.

Thank you for submitting a PR @laf! We have found the following @geordish based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@wriedel

This comment has been minimized.

Show comment
Hide comment
@wriedel

wriedel Mar 6, 2017

Hi Laf, it's a little bit cumbersome to convert 90 devices that way but it's being worth every minute to get this 1 minute high resolution graphs ;-) Script worked like a charm and I hope we can get a wildcard option in the near future.

wriedel commented Mar 6, 2017

Hi Laf, it's a little bit cumbersome to convert 90 devices that way but it's being worth every minute to get this 1 minute high resolution graphs ;-) Script worked like a charm and I hope we can get a wildcard option in the near future.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 7, 2017

Member

Added -h all.

Don't see the point in adding a wildcard.

Member

laf commented Mar 7, 2017

Added -h all.

Don't see the point in adding a wildcard.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated scripts/rrdstep.php
$files = glob($rrd_path . '/' . $hostname . '/*.rrd');
$run = readline("Are you sure you want to run this command [N/y]: ");
if ($run != 'y' || $run == 'Y') {

This comment has been minimized.

@murrant

murrant Mar 9, 2017

Member

Looks like this should be:
+if (!($run == 'y' || $run == 'Y'))

@murrant

murrant Mar 9, 2017

Member

Looks like this should be:
+if (!($run == 'y' || $run == 'Y'))

Show outdated Hide outdated scripts/rrdstep.php
$rrd_file = array_pop($tmp);
echo "Converting $file: ";
$command = "$rrdtool dump $file > $random;
sed -i 's/<step>300/<step>$step/' $random;

This comment has been minimized.

@murrant

murrant Mar 9, 2017

Member

Perhaps use && in between these commands instead of ;. That way if one fails the rest are not executed.

@murrant

murrant Mar 9, 2017

Member

Perhaps use && in between these commands instead of ;. That way if one fails the rest are not executed.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Mar 9, 2017

Member

@laf I know you mentioned this elsewhere, but why can't you use rrdtune?

Member

murrant commented Mar 9, 2017

@laf I know you mentioned this elsewhere, but why can't you use rrdtune?

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 9, 2017

Member

Because it doesn't work, try it. If you tune a file and then dump it you will see the step will remain the same.

I'm unsure if this is all versions but (iirc) I tested on 1.4.7 and 1.6.x

Member

laf commented Mar 9, 2017

Because it doesn't work, try it. If you tune a file and then dump it you will see the step will remain the same.

I'm unsure if this is all versions but (iirc) I tested on 1.4.7 and 1.6.x

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Mar 9, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@nrm21

This comment has been minimized.

Show comment
Hide comment
@nrm21

nrm21 Mar 9, 2017

Hi,

This script works great! However my graphs only update every 5 mins and they update with 5 new values (instead of one as before). On further inspection it appears the code is doing some sort of:
what is the current unix timestamp? ->
ok now lets round to the nearest 5 min step of that timestamp ->
we'll use that in the URL request for 'from=' and 'to=' ->
now we generate the graph using those values

...or it appears something like that based on studying web behavior.

Maybe the graph portion of code could be changed to round to the nearest minute of the current timestamp instead of 5 min, once the script is applied?

Otherwise it might appear to a regular user after running the script that it isn't working or did nothing (as I originally thought when trying it out).

nrm21 commented Mar 9, 2017

Hi,

This script works great! However my graphs only update every 5 mins and they update with 5 new values (instead of one as before). On further inspection it appears the code is doing some sort of:
what is the current unix timestamp? ->
ok now lets round to the nearest 5 min step of that timestamp ->
we'll use that in the URL request for 'from=' and 'to=' ->
now we generate the graph using those values

...or it appears something like that based on studying web behavior.

Maybe the graph portion of code could be changed to round to the nearest minute of the current timestamp instead of 5 min, once the script is applied?

Otherwise it might appear to a regular user after running the script that it isn't working or did nothing (as I originally thought when trying it out).

@nrm21

This comment has been minimized.

Show comment
Hide comment
@nrm21

nrm21 Mar 9, 2017

Found it... in ./includes/definitions.inc.php

Change: $config['time']['now'] -= ($config['time']['now'] % 300);
from mod 300 to mod 60

and voila! Graphs now show by the min. :)

nrm21 commented Mar 9, 2017

Found it... in ./includes/definitions.inc.php

Change: $config['time']['now'] -= ($config['time']['now'] % 300);
from mod 300 to mod 60

and voila! Graphs now show by the min. :)

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 9, 2017

Member

Thanks but this has nothing to do with this script.

Member

laf commented Mar 9, 2017

Thanks but this has nothing to do with this script.

@nrm21

This comment has been minimized.

Show comment
Hide comment
@nrm21

nrm21 Mar 10, 2017

Ok maybe it doesn't but, I assume that others like me might want to not just poll every min but also see an up to the min graph of their bandwidth data. In that case, it might be necessary to do that to the default code to achieve it.

nrm21 commented Mar 10, 2017

Ok maybe it doesn't but, I assume that others like me might want to not just poll every min but also see an up to the min graph of their bandwidth data. In that case, it might be necessary to do that to the default code to achieve it.

@murrant murrant merged commit 8ca1f16 into librenms:master Mar 11, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laf laf deleted the laf:step-convert branch Mar 11, 2017

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Mar 11, 2017

Member

Feature requests need to be posted on https://community.librenms.org/c/feature-requests

Member

laf commented Mar 11, 2017

Feature requests need to be posted on https://community.librenms.org/c/feature-requests

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