You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@metron.apache.org by merrimanr <gi...@git.apache.org> on 2017/09/15 14:06:24 UTC

[GitHub] metron pull request #762: METRON-1189: Add alert escalation to the Alerts UI

GitHub user merrimanr opened a pull request:

    https://github.com/apache/metron/pull/762

    METRON-1189: Add alert escalation to the Alerts UI

    ## Contributor Comments
    This PR adds the ability to escalate alerts from the Alerts UI.  This is a follow on to https://github.com/apache/metron/pull/711.
    
    This can be tested locally against full dev.  Follow the instructions in "Development Setup" section in "metron-interface/metron-alerts/README.md" to start the alerts UI.  Then in a separate session, ssh to vagrant and start the Kafka console consumer for the "escalate" topic.  
    
    Alerts can be escalated from 2 different views:  alerts list view and alerts detail view.  After logging in, a list of alerts will initially be displayed in a table.  Select multiple alerts with the checkboxes on the right side, then select the "Escalate" option from the "Actions" dropdown.  This will escalate these alerts by sending them to the "escalate" Kafka topic and update the "alert_status" field on these records in Elasticsearch.  Verify the alert_status column now shows "ESCALATE" for the selected alerts and that the alerts were produced to the Kafka console consumer.
    
    Next drill into a single alert by clicking anywhere on a row that's not a link.  This will display a separate window with that alert's details.  At the top there will be several status options that can be selected (OPEN, DISMISS, RESOLVE, ESCALATE, etc).  Selecting any of these should update the alert_status in Elasticsearch and have that status reflected in the UI.  Selecting "ESCALATE" should produce a message to the Kafka console consumer for that alert.
    
    This PR includes several improvements that were needed to complete this task.  These include:
    
    - Switching the "alert_status" update call to use the REST layer instead of going directly against Elasticsearch.  The REST endpoint does more than just update the index so this is absolutely necessary for the system to work correctly.
    - Slightly refactored angular services to match the REST layer.
    - Changed the backend used for e2e tests from mock services to a dedicated e2e index in Elasticsearch.  I found the mock services were insufficient for testing this feature and would require a lot of work to implement the various services needed (that won't scale well as we add more search features).  I switched to a dedicated e2e ES index instead and am now leveraging the REST service as it is deployed in full dev.  I'm happy to discuss this approach as needed but I feel it's an improvement and makes writing the e2e tests easier.  Scripts and javascript functions are provided to facilitate easy setup/teardown of this index.
    - The indices included in the alerts UI can now be specified in an environment setting so that different indices can be configured for different environments (for example "alerts_ui_e2e" index is used for e2e tests).  This may not end up being the final solution but they are currently hardcoded in a javascript file and I feel this is incremental progress towards something better.
    
    This is not ready for primetime yet so please DO NOT MERGE.  The e2e tests are still failing intermittently in unrelated tests.  I was seeing this before these changes and the switch to using REST in full dev (instead of mock services) likely made it worse.  I am still working on making this more robust and also adding documentation for the e2e testing framework.  However, the functionality is there and I wanted to give people a chance to use it, review the changes and give some initial feedback.
    
    ## Pull Request Checklist
    
    Thank you for submitting a contribution to Apache Metron.  
    Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.  
    Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.  
    
    
    In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
    
    ### For all changes:
    - [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel). 
    - [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
    - [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
    
    
    ### For code changes:
    - [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
    - [ ] Have you included steps or a guide to how the change may be verified and tested manually?
    - [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
      ```
      mvn -q clean integration-test install && build_utils/verify_licenses.sh 
      ```
    
    - [x] Have you written or updated unit tests and or integration tests to verify your changes?
    - [x] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)? 
    - [x] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
    
    ### For documentation related changes:
    - [x] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
    
      ```
      cd site-book
      mvn site
      ```
    
    #### Note:
    Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
    It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
    


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/merrimanr/incubator-metron escalate

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/metron/pull/762.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #762
    
----
commit 07a438de18e21b12b36da6ec22e9ebf8a0b93905
Author: merrimanr <me...@gmail.com>
Date:   2017-09-14T22:34:28Z

    initial commit

----


---

[GitHub] metron pull request #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by iraghumitra <gi...@git.apache.org>.

Github user iraghumitra commented on a diff in the pull request:

    https://github.com/apache/metron/pull/762#discussion_r140751012
  
    --- Diff: metron-interface/metron-alerts/e2e/alert-details/alert-details.po.ts ---
    @@ -0,0 +1,60 @@
    +/**
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *     http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing, software
    + * distributed under the License is distributed on an "AS IS" BASIS,
    + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
    + * See the License for the specific language governing permissions and
    + * limitations under the License.
    + */
    +
    +import {browser, element, by, protractor} from 'protractor';
    +
    +export class MetronAlertDetailsPage {
    +
    +  navigateTo() {
    +    browser.waitForAngularEnabled(false);
    +    return browser.get('/alerts-list(dialog:details/alerts_ui_e2e/c4c5e418-3938-099e-bb0d-37028a98dca8)');
    +  }
    +
    +  clickNew() {
    +    element.all(by.css('.metron-slider-pane-details table tbody tr')).get(1).all(by.css('td')).get(0).click();
    +  }
    +
    +  clickOpen() {
    +    element.all(by.css('.metron-slider-pane-details table tbody tr')).get(1).all(by.css('td')).get(1).click();
    +  }
    +
    +  clickDismiss() {
    +    element.all(by.css('.metron-slider-pane-details table tbody tr')).get(1).all(by.css('td')).get(2).click();
    +  }
    +
    +  clickEscalate() {
    +    element.all(by.css('.metron-slider-pane-details table tbody tr')).get(0).all(by.css('td')).get(1).click();
    +  }
    +
    +  clickResolve() {
    +    element.all(by.css('.metron-slider-pane-details table tbody tr')).get(2).all(by.css('td')).get(1).click();
    +  }
    +
    +  getAlertStatus(previousText) {
    +    let alertStatusElement = element.all(by.css('.metron-slider-pane-details .form .row')).get(0).all(by.css('div')).get(1);
    +    return this.waitForTextChange(alertStatusElement, previousText).then(() => {
    +      return alertStatusElement.getText();
    +    });
    +  }
    +
    +  waitForTextChange(element, previousText) {
    --- End diff --
    
    Should we move this to e2e_utils


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by merrimanr <gi...@git.apache.org>.

Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/762
  
    The latest commit merges in master and addresses your feedback @iraghumitra.  I solved the issue in my previous comment by pausing anytime a checkbox is checked and resuming when all checkboxes are unchecked or a bulk acton finishes (checkboxes for alerts included in a bulk action are unchecked after it finishes anyways).  Let me know what you think.


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by iraghumitra <gi...@git.apache.org>.

Github user iraghumitra commented on the issue:

    https://github.com/apache/metron/pull/762
  
    I wanted to suggest as a follow-on PR too, my bad missed it during the first round of review.


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by iraghumitra <gi...@git.apache.org>.

Github user iraghumitra commented on the issue:

    https://github.com/apache/metron/pull/762
  
    Should we use saveRefreshState() and restoreRefreshState() when the escalation rest API's are in progress? This will ensure that the page doesn't get refreshed and users would not lose track of their selected alerts.


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by iraghumitra <gi...@git.apache.org>.

Github user iraghumitra commented on the issue:

    https://github.com/apache/metron/pull/762
  
    @merrimanr I uncovered the issue when I was trying to integrate METRON-1189 to METRON-1123. I handled it in PR #768, the fix is to empty the selection list whenever we get new data. This would ensure that previous selections are not lingering around.  But as for selections getting removed due to higher refresh interval, I understand the issue the only option I see is pause refresh if any rows are selected on the UI. If you have a better solution in mind I would be glad to implement that too.


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by iraghumitra <gi...@git.apache.org>.

Github user iraghumitra commented on the issue:

    https://github.com/apache/metron/pull/762
  
    +1


---

[GitHub] metron issue #762: METRON-1189: Add alert escalation to the Alerts UI

Posted by merrimanr <gi...@git.apache.org>.

Github user merrimanr commented on the issue:

    https://github.com/apache/metron/pull/762
  
    This PR is already fairly large and I don't see the "Hide Dismissed/Resolved Alerts" feature as being a trivial addition.  Can we do that as a follow-on?


---