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

webui: New eventlog severity classification #5830

Merged
merged 14 commits into from Feb 12, 2017

Conversation

Projects
None yet
6 participants
@InsaneSplash
Copy link
Contributor

commented Feb 8, 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?

The idea behind this change is :

  • Provide the ability to tag an event with a severity at the time it is written to the eventlog table
  • Be able to modify the colours of the events displayed in one place.
  • Not have to try run a regex against the text to determine the icon type.
  • Provide 5 levels of severity (based on defcon) :)
@mention-bot

This comment has been minimized.

Copy link

commented Feb 8, 2017

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

@InsaneSplash

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

I have updated some of the log_event statements with a severity to kick things off in this PR.

@@ -0,0 +1 @@
ALTER TABLE `librenms`.`eventlog` ADD COLUMN `severity` INT(1) NULL DEFAULT 0 AFTER `reference`;

This comment has been minimized.

Copy link
@laf

laf Feb 8, 2017

Member

You can't specify the database here, not everyone uses librenms.

I also still think default should be 1:

`ALTER TABLE `eventlog` ADD COLUMN `severity` INT(1) NULL DEFAULT 1 AFTER `reference`;
@@ -796,7 +796,7 @@ function get_astext($asn)
}
# Use this function to write to the eventlog table
function log_event($text, $device = null, $type = null, $reference = null)
function log_event($text, $device = null, $type = null, $reference = null, $severity = 0)

This comment has been minimized.

Copy link
@laf

laf Feb 8, 2017

Member

I think default should be 1.

alerts.php Outdated
@@ -349,13 +349,13 @@ function ExtTransports($obj)
$prefix[4] = &$prefix[0];
if ($tmp === true) {
echo 'OK';
log_event('Issued '.$prefix[$obj['state']]." for rule '".$obj['name']."' to transport '".$transport."'", $obj['device_id']);
log_event('Issued '.$prefix[$obj['state']]." for rule '".$obj['name']."' to transport '".$transport."'", $obj['device_id'],'','',1);

This comment has been minimized.

Copy link
@laf

laf Feb 8, 2017

Member

Don't specify '', instead use null

@laf

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Quite a few coding style fixes needed: https://travis-ci.org/librenms/librenms/jobs/199679004

@InsaneSplash

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2017

Ok. Let me run through these changes.

If you don't think the default should be grey (0) I think it should rather be information (2) as we should keep ok (1) for OK events rather.

@laf

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

2 Is fine with me :)

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 8, 2017

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

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 8, 2017

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

} else {
$icon = "<i class='fa fa-bookmark-o fa-lg' style='color:black' aria-hidden='true'></i>";
}
$icon = "<i class='fa fa-bookmark fa-lg' style='color:$severity_colour' aria-hidden='true'></i>";

This comment has been minimized.

Copy link
@laf

laf Feb 8, 2017

Member

actually sorry to be a pain, one last change. Rather than doing style='color' can you not just create styles in styles.css called log-$colourname and then tack log-darkgrey on the end of fa-lg?

This comment has been minimized.

Copy link
@InsaneSplash

InsaneSplash Feb 9, 2017

Author Contributor

Hmm worth a shot! Let me give it a bash, i don't mind.

This comment has been minimized.

Copy link
@InsaneSplash

InsaneSplash Feb 9, 2017

Author Contributor

OK, giving it a little thought, I dont think that would work as it would not allow for any future customization of the colours (mine might not be so great!). I think from what I can see we have 2 options,....

  1. Leave it as is
  2. Move the "style=color:xxx" attribute in to the function that determines the colour and just have the $severity_colour variable included?
@InsaneSplash

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2017

For completeness, this references the issue #5698 created.

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 9, 2017

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

@laf

This comment has been minimized.

Copy link
Member

commented Feb 9, 2017

Works for me :)

@librenms/reviewers

@laf

laf approved these changes Feb 9, 2017

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 10, 2017

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

@murrant

This comment has been minimized.

Copy link
Member

commented Feb 10, 2017

Honestly, it looks like severity should be before reference in the function call.

If I get a chance later I can do that it is about a 30s refactor with phpstorm.

@InsaneSplash

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2017

Great! We will just need to revise the severity of each log_event depending on what the type is. There about about 90+ events that need classifying from what I initially looked at.

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 10, 2017

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

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 10, 2017

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

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 11, 2017

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

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 11, 2017

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

@scrutinizer-notifier

This comment has been minimized.

Copy link

commented Feb 11, 2017

The inspection completed: 7 new issues, 1 updated code elements

@laf

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

trigger ci

@LibreNMS-CI

This comment has been minimized.

Copy link

commented Feb 12, 2017

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

@laf laf merged commit 5bfd23e into librenms:master Feb 12, 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 19, 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.