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

feature: Updated Nfsen integration support #6003

Merged
merged 66 commits into from Mar 2, 2017

Conversation

Projects
None yet
5 participants
@VVelox
Copy link
Contributor

VVelox commented Feb 25, 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

Add support for showing profile configure channels for devices in nfsen. Also clean up the doc on it some, making it a bit more clear about what needs to be in there and the like.

VVelox and others added some commits Feb 15, 2017

kitsune
*rename it so it can be called via the apps page
*setup the apps page to call it
@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Feb 26, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

1 similar comment
@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Feb 26, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Feb 26, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

}
foreach ($config['nfsen_rrds'] as $nfsenrrds) {
if ($nfsenrrds[(strlen($nfsenrrds) - 1)] != '/') {

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

I prefer this way of doing it:

$nfsenrrds = rtrim($nfsenrrds, '/') . '/';
$nfsenrrds .= '/';
}
$nfsenHostname=$device['hostname'];

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

You can put this in an else block, or you should just always replace.

if ($config['nfsen_split_char']) {
    $nfsenHostname = str_replace('.', $config['nfsen_split_char'], $device['hostname']);
} else {
    $nfsenHostname = $device['hostname'];
}
$rrd_list = array();
$nfsen_iter = 1;
foreach ($flowtypes as $flowtype) {
$rrd_list[$nfsen_iter]['filename'] = $rrd_filename;

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

Rather than having to keep track of $nfsen_iter, you can just set a complete array once:

$rrd_list[] = array(
    'filename'  => $rrd_filename,
    'descr' => $flowtype,
    'ds' => $dsprefix.$flowtype
);
$rrd_list[$nfsen_iter]['filename'] = $rrd_filename;
$rrd_list[$nfsen_iter]['descr'] = $flowtype;
$rrd_list[$nfsen_iter]['ds'] = $dsprefix.$flowtype;

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

Code below this should not be in the $flowtypes foreach loop

$unit_text = $dsdescr;
$scale_min = '0';
if ($_GET['debug']) {

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

d_echo($rrd_list); should be used instead.

if (is_dir($hostDir)) {
$nfsenRRDchannelGlob=$hostDir.'*.rrd';
foreach (glob($nfsenRRDchannelGlob) as $nfsenRRD) {
$channel=str_replace($hostDir, '', $nfsenRRD);

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

You can combine these into one str_replace:

$channel = str_replace(array($hostDir, '.rrd'), '', $nfsenRRD);
include 'includes/print-device-graph.php';
echo generate_link('General', $link_array, array('nfsen' => 'general'));
$printedChannel='0';

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

Using false and true for this variable makes it more obvious what it is for.

@@ -10,7 +10,8 @@ The following is the configuration that can be used:
```php
$config['nfsen_enable'] = 1;
$config['nfsen_split_char'] = "_";
$config['nfsen_rrds'] = "/var/nfsen/profiles-stat/live/";
$config['nfsen_rrds'][] = "/var/nfsen/profiles-stat/live/";

This comment has been minimized.

Copy link
@murrant

murrant Mar 1, 2017

Member

Make all of these use the same quotes for consistency. I don't care which one.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Mar 1, 2017

Looks good, just some suggestions to make the code more readable.

@laf laf changed the title Nfsen feature: Add Nfsen application support Mar 1, 2017

@laf laf changed the title feature: Add Nfsen application support feature: Updated Nfsen integration support Mar 1, 2017

VVelox added some commits Mar 1, 2017

@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Mar 1, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

1 similar comment
@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Mar 1, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Mar 1, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Mar 1, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

@LibreNMS-CI

This comment has been minimized.

Copy link

LibreNMS-CI commented Mar 2, 2017

Auto-Deploy finished, Test PR at http://6003.ci.librenms.org or https://6003.ci.librenms.org

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Mar 2, 2017

The inspection completed: No new issues

@murrant murrant merged commit 41bf70d into librenms:master Mar 2, 2017

2 checks passed

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

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

@VVelox VVelox deleted the VVelox:nfsen branch Mar 11, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.