Metron-1252: Build ui for grouping alerts into meta alerts #803

iraghumitra · 2017-10-17T19:02:24Z

Contributor Comments

The purpose of the PR is to provide GUI for grouping multiple alerts into a meta alert, the rest api for this is already available via METRON-1158

The current implementation has following features

Meta alert can be created from the tree view of alerts
Meta alerts can be viewed in the table view
- The meta alert has a link icon on the left to denote that it is a meta alert
- The meta alert has a expand/collapse icon to see all the alerts within it
- The meta alert has a cumulative score (more on how this score is calculated is available in the original ticket)
Support to add/remove an alert to the meta alert from the table view
Change the state of meta alert
Add comments to a meta alert
Option to name a meta alert (This is just a convenience to refer to alerts)

Caveats
The meta alert index is deleted and recreated few times by e2e test's. I ensured that the index remains available irrespective of the tests succeeding/failing. Just wanted to bring it up.

Limitations

Meta alerts cannot be viewed in the tree view
Adding comments/Status change is restricted to just meta alerts. You cannot perform these actions on alerts contained in meta alert
A meta alert can contain only 999 alerts
Delete of the entire meta alert is not supported yet

Next

It will be nice to have a notification when entities are changed in the UI

I noticed that search on GUID was not working before I fixed it in this PR.

~~E2E tests are incoming I wanted to check if the community has any suggestion on this.~~

~~PS: I had to comment one of the test cases since sort by guid is broke. I will raise a ticket for it~~

Pull Request Checklist

Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines 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:

Is there a JIRA ticket associated with this PR? If not one needs to be created at Metron Jira.
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.
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 
```
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
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:

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 is set up for your personal repository such that your branches are built there before submitting a pull request.

…r-metron into METRON-1252-review

merrimanr · 2017-10-19T20:07:41Z

metron-interface/metron-alerts/e2e/alerts-list/tree-view/tree-view.e2e-spec.ts



-  it('should have sort working for group details for multiple sub groups', () => {
+  xit('should have sort working for group details for multiple sub groups', () => {


What does xit mean here? Just curious.

x skips the execution. Since sort on guid is not working anymore, I wanted to skip the test

Can you sort on a different field instead? Timestamp maybe? I don't think the right answer is to just disable the test.

merrimanr · 2017-10-19T20:09:42Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+    let patchRequest = new PatchRequest();
+    patchRequest.guid = this.selectedMetaAlert;
+    patchRequest.sensorType = 'metaalert';
+    patchRequest.index = META_ALERTS_INDEX;


should be patchRequest.index = META_ALERTS_INDEX + '_index';

merrimanr · 2017-10-19T20:10:29Z

metron-interface/metron-alerts/src/app/utils/constants.ts

 import {environment} from '../../environments/environment';

+export const META_ALERTS_SENSOR_TYPE = 'metaalert';
+export const META_ALERTS_INDEX = 'metaalerts';


should be export const META_ALERTS_INDEX = 'metaalert';

merrimanr · 2017-10-19T20:10:49Z

metron-interface/metron-alerts/src/app/utils/constants.ts

 export let DEFAULT_FACETS = ['source:type', 'ip_src_addr', 'ip_dst_addr', 'host', 'enrichments:geo:ip_dst_addr:country'];
 export let DEFAULT_GROUPS = ['source:type', 'ip_src_addr', 'ip_dst_addr', 'host', 'enrichments:geo:ip_dst_addr:country'];
-export let INDEXES =  environment.indices ? environment.indices.split(',') : ['websphere', 'snort', 'asa', 'bro', 'yaf'];
+export let INDEXES =  environment.indices ? environment.indices.split(',') : ['websphere', 'snort', 'asa', 'bro', 'yaf',  'metaalerts'];


should be export let INDEXES = environment.indices ? environment.indices.split(',') : ['websphere', 'snort', 'asa', 'bro', 'yaf', 'metaalert'];

merrimanr · 2017-10-19T20:14:09Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+    patchRequest.patch = [new Patch('replace', 'alert', alertSources)];
+
+    this.updateService.patch(patchRequest).subscribe(rep => {
+      console.log('Meta alert saved');


should this be here?

As mentioned in the Next Section of CC, we need some kind of notification once actions like add/remove are done otherwise it is confusing. These are placeholders till we have a notification mechanism. If it is not a problem I would suggest we leave them else I can remove them.

If you are planning on replace that with something in the future then I'm ok with it.

merrimanr · 2017-10-19T20:14:25Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+        console.log('Unable to get a single meta alert');
+      }
+    });
+    console.log(this.selectedMetaAlert);


should this be here?

As mentioned in the Next Section of CC, we need some kind of notification once actions like add/remove are done otherwise it is confusing. These are placeholders till we have a notification mechanism. If it is not a problem I would suggest we leave them else I can remove them.

merrimanr · 2017-10-19T20:18:52Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.module.ts

+@NgModule({
+  imports: [ routing,  SharedModule ],
+  declarations: [ MetaAlertsComponent ],
+  providers: [ UpdateService, MetaAlertService ],


Should UpdateService be in the list of providers here? Isn't it application-wide and provided at the app root level as well?

merrimanr · 2017-10-19T20:23:28Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+    searchRequest.facetFields = [];
+    searchRequest.indices =  [META_ALERTS_INDEX];
+    searchRequest.sort = [new SortField('threat:triage:score', 'desc')];
+    searchRequest.fields = ['id', 'alert_status', 'threat:triage:score', 'count', 'guid', 'name'];


should 'alert_status' be 'status'?

merrimanr · 2017-10-19T20:28:15Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+
+    this.searchService.search(searchRequest).subscribe((searchResponse: SearchResponse) => {
+      if (searchResponse.results.length === 1) {
+        searchResponse.results[0].source.alert = [...searchResponse.results[0].source.alert,


Why are you reassigning old and new alerts back to searchResponse.results[0].source.alert? Makes it a little confusing to read.

merrimanr · 2017-10-19T20:30:14Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts

+    searchRequest.sort = [];
+    searchRequest.fields = [];
+
+    this.searchService.search(searchRequest).subscribe((searchResponse: SearchResponse) => {


I think this should be a findOne call instead of a search. Seems inefficient to search and filter down to a single record using a guid.

merrimanr · 2017-10-19T20:35:51Z

metron-interface/metron-alerts/src/app/service/update.service.ts

  }

-  public updateAlertState(alerts: Alert[], state: string): Observable<{}> {
+  public updateAlertState(alerts: Alert[], state: string, fireChangeListner = true): Observable<{}> {


is fireChangeListner used in this function?

It is passed to patch function and is used there. This is specifically needed in the case where we select multiple rows on the table to update the status. There are too many unnecessary find one calls fired.

I agree with you and I think that's a good approach. The problem is you're not actually using that variable in this method. Maybe you meant to pass it to this.patch(patchRequest) on line 69?

merrimanr · 2017-10-19T20:36:19Z

metron-interface/metron-alerts/src/app/service/update.service.ts

  constructor(private http: Http) { }

-  public patch(patchRequest: PatchRequest): Observable<{}> {
+  public patch(patchRequest: PatchRequest, fireChangeListner = true): Observable<{}> {


Could we fix the typo in Listner?

merrimanr · 2017-10-19T20:36:57Z

metron-interface/metron-alerts/src/styles.scss

+    }
+  }
+}
+/**********************/


need newline

merrimanr · 2017-10-19T20:37:17Z

metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.html

+      </div>
+    </div>
+  </div>
+</div>


need newline

merrimanr · 2017-10-19T20:37:48Z

metron-interface/metron-alerts/src/app/alerts/alerts-list/table-view/table-view.component.scss

+
+.dropdown-cell {
+  padding-left: 0.6rem;
 }


need newline

merrimanr · 2017-10-19T20:38:08Z

metron-interface/metron-alerts/src/app/alerts/alert-details/alert-details.component.scss

+.disabled {
+  opacity: 0.5;
+  cursor: not-allowed;
+}


need newline

Added newline

merrimanr · 2017-10-19T21:12:52Z

I did an initial review of this and I see several things we need to work through. It's a pretty significant feature so that's not surprising. I know there are some other PRs being worked on that this PR depends on (#806). Some questions I have:

In this PR description, what does 'Change the state of meta alert' mean?
Can I create a new empty metaalert?
Can I create a metaalert by manually selecting alerts?
How do I create a metaalert from a group that is >1 levels deep?
How do I see all metaalerts? I was able to query with "exists:alert.*" but that is not intuitive.
Can I only remove 1 alert from a metaalert at a time?
Would it be useful to assign a name to your metaalert in the confirmation form rather than having to find it after you create it and rename in the detail view?
Not a fan of vertical scrolling through alerts in metaalert detail, would it be possible to add pagination?

Some initial bugs I found (commented on some of these):

metaalert index name throughout code is wrong, should be 'metaalert'
metaalerts have alert_status set to 'NEW'
when you group by ip address fields, then expand group, count goes to 0 and corrects after the next search (not sure if this existed before this PR)
clicking on a metaalert fires this findOne call:

{
  "guid": "ca80f4fc-0cdb-431c-b972-c460dad022ee",
  "sensorType": "undefined"
}

not obvious from a user perspective what happens when I remove all alerts from a metaalert (I can see rest call that sets metaalert status to inactive)
missing space in alert merge confirmation and 1 alert displays as '1alerts'
need to add metaalert index to e2e npm environment
when metaalerts and alerts are in the same result set, sorting on fields other than timestamp causes metaalerts to be excluded
when I select an alert and then select Add to Alert in bulk actions, metaalerts display 0 alerts ie.
(0)
adding a comment to metaalert is failing because sensorType is undefined in patch request:

{
  "patch": [
    {
      "op": "add",
      "path": "/comments",
      "value": [
        {
          "comment": "test",
          "username": "user",
          "timestamp": 1508446386940
        }
      ]
    }
  ],
  "guid": "b9479340-316b-46db-baa5-0a0376ff015a",
  "index": "metaalert_index",
  "sensorType": "undefined"
}

when I change/assign a metaalert name, reverts back to id in search result list on next search
comment icons in the list view are not appearing after I add a comment even though comments are displayed in the metaalert default view (likely related to failed add comment call, being written only to hbase and not ES)

I'm also assuming more e2e tests are coming soon.

james-sirota · 2017-10-19T21:19:08Z

In this PR description, what does 'Change the state of meta alert' mean?

This should say "Group alerts into meta alert"

Can I create a new empty metaalert?

No, empty meta alert doesn't make sense

Can I create a metaalert by manually selecting alerts?

Yes, you should be able to. This may be beyond the scope of this PR, though

How do I create a metaalert from a group that is >1 levels deep?

This condition should never occur. There should be one meta alert with an inner (1) level of alerts

How do I see all metaalerts? I was able to query with "exists:alert.*" but that is not intuitive.

We should think of a way to make this more intuitive, but for now this works

Can I only remove 1 alert from a metaalert at a time?

I think for now that's fine

Would it be useful to assign a name to your metaalert in the confirmation form rather than having to find it after you create it and rename in the detail view?

You can sort of do that with the comment. But yeah that would be useful. Maybe a follow-on PR?

Not a fan of vertical scrolling through alerts in metaalert detail, would it be possible to add pagination?

Ideally your meta alert should not have enough alerts in it to require pagination

james-sirota · 2017-10-20T13:34:05Z

I filed the following follow-on PRs per your comments:

https://issues.apache.org/jira/browse/METRON-1268
https://issues.apache.org/jira/browse/METRON-1269

merrimanr · 2017-10-21T13:53:54Z

I just submitted a PR against this PR that addresses all of the bugs reported above except 1:

when metaalerts and alerts are in the same result set, sorting on fields other than timestamp causes metaalerts to be excluded

I think we need more discussion on what should happen in that scenario (sorting on fields that doesn't exist in metaalerts). Let me know what you think.

justinleet · 2017-10-21T21:41:28Z

Potentially we want to expose some abstraction for the ES options for missing field sorting (that I admittedly don't know exist in Solr).

https://www.elastic.co/guide/en/elasticsearch/reference/2.3/search-request-sort.html#_missing_values

METRON-1252 bug fixes

- Added better assertions for few tests - Added test specs for 1)Creating a meta alert from a group that is nested by more than 1 level 2)Meta Alert should appear in Filters 3)Meta Alert should not appear in Groups 4)Delete a meta alert from the middle and check the data

iraghumitra · 2017-11-21T13:09:43Z

@justinleet Thanks for taking time to test the PR. I was using an incorrect index to find the alert to delete when I implemented the new API's I fixed it now. Also added tests for all the issues I fixed so far.

iraghumitra · 2017-11-21T13:59:02Z

I noticed that when I am running e2e test's, the comments that were added in the previous run are still visible at times. The e2e tests delete's all the comments that it added and the only way I can see this happening is if in the previous run of tests add comment was successful and remove comment fails. The best fix for this would be to clean the metron_update table before running the tests. Since we don't have a rest API's for Hbase I am looking at finding alternatives. For the time being, if you bump into this issue I request you to truncate the meton_update table manually.

nickwallen · 2017-11-21T14:02:48Z

Hi @iraghumitra - The UI does not refresh itself after a metaalert is deleted. If this is something easy to fix, let's tackle it. Otherwise, I can create a JIRA to track this and we can fix it as a separate PR.

EDIT: More specifically, after I delete a metaalert, I can still see in the Filters panel; under source:type, a count of metaalerts equal to 1. If that panel refreshed, it should show 0 metaalerts in this case. Once the UI is refreshed, either manually or through the background refresh, the Filters panel state is corrected.

EDIT: Same thing happens if I delete a child alert from a metaalert. In the main panel, I do not immediately see the child alert that was removed from the metaalert. After a manual or background refresh, I then see the results I would expect.

iraghumitra · 2017-11-21T14:36:51Z

@nickwallen I am not refreshing on purpose. If I refresh the UI after any operation users would lose the current context and see new alerts (If available). If you think this is not a major concern I can add a call to refresh the UI.

nickwallen · 2017-11-21T14:50:52Z

I am not refreshing on purpose. If I refresh the UI after any operation users would lose the current context and see new alerts (If available).

Ok @iraghumitra. Since you have a valid concern about whether this is the right approach, then let's NOT change this in your PR. We can track this separately and come to the right resolution after your PR goes in. Thanks.

…r-metron into missing-filters

Missing intermediate filters

iraghumitra · 2017-11-21T15:33:23Z

@merrimanr Thanks for iraghumitra#5. It was a pretty significant issue. I merged the fix 👍

nickwallen · 2017-11-21T15:55:18Z

Edit: The issue is with "Recent Searches", not "Saved Searches".

Something seems to be wrong with Recent Searches. When I run through a manual test script of creating metaalerts, deleting and navigating, most (if not all) of my recent searches are these giant query strings that are incorrect.

I am not sure if this is a pre-existing condition or not.

justinleet · 2017-11-21T17:00:02Z

@nickwallen I haven't been able to get into the same sort of state, but I know you've used/tested the UI more than me. Do you know a repeatable way to reproduce this? Or have you been able to determine if it's preexisting, assuming you've looked into it.

iraghumitra · 2017-11-21T17:47:05Z

@nickwallen That looks wicked can you share your manual steps that lead to this?

nickwallen · 2017-11-21T19:14:00Z

@justinleet @iraghumitra I am glad that no one else can reproduce. I will work on trying to get a set of steps to reproduce this. Let's not worry about this on the current PR.

justinleet · 2017-11-21T19:16:05Z

@nickwallen I agree, unless I'm missing something, it seems like a bug with recent searches, since you didn't say searches were weird during the initial attempts.

iraghumitra · 2017-11-21T19:19:33Z

@nickwallen I will raise a jira to track this. Even if we are able to reproduce this issue it is unlikely bcoz this PR.

nickwallen · 2017-11-21T19:26:24Z

IMHO I think the functionality is good on this PR. There are 3 functional tests that failed, but they are minor and can be addressed after we get this PR in.

My only hold-up right now is that I cannot get the e2e tests to pass. I am going to rebuild Full Dev and try again. It might have something to do with stale data.

Run e2e tests.

UPDATE: I am still seeing these e2e failures.

**************************************************
*                    Failures                    *
**************************************************

1) metron-alerts App should have all time range values populated - 1
  - Expected 'Last 30 days,2595600000' to equal 'Last 30 days,2592000000' for last 30 days
  - Expected 'Last 60 days,5187600000' to equal 'Last 60 days,5184000000' for last 60 days
  - Expected 'Last 90 days,7779600000' to equal 'Last 90 days,7776000000' for last 90 days

2) metron-alerts App should have all time-range included while searching
  - Expected 'Alerts (0)' to equal 'Alerts (5)'.
  - Expected 'Alerts (0)' to equal 'Alerts (169)'.
  - Expected 'Alerts (0)' to equal 'Alerts (5)'.

3) metron-alerts tree view drag and drop should change group order
  - Expected '0,alerts_ui_e2e,ALERTS,91' to equal '0,alerts_ui_e2e,ALERTS,169' First Dash Row should be correct
  - Expected '' to equal '0 US (22)' Dash Group Values should be correct for US
  - Expected '' to equal '0 RU (44)' Dash Group Values should be present for RU
  - Expected '' to equal '0 FR (25)' Dash Group Values should be present for FR

4) metron-alerts tree view should have sort working for group details for multiple sub groups
  - Failed: stale element reference: element is not attached to the page document
    (Session info: chrome=62.0.3202.94)
    (Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.13.1 x86_64)

5) metron-alerts tree view should have search working for group details for multiple sub groups
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

6) meta-alerts workflow should create a meta alert from nesting of more than one level
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

The functional tests that I am working with...

merrimanr · 2017-11-21T20:10:51Z

Most of the functional tests worked for me as well. I was able to get the e2e tests to pass after several runs with the exception of a time picker test that I believe is due to a timezone issue and not related to this PR.

I was also able to test that meta alerts show up in search results when a search matches a child alert field. However I don't see this case covered in the e2e tests. I think this is a pretty critical function and needs a test.

…for a alert - Fixed issue with timezone in date picker

iraghumitra · 2017-11-22T14:32:33Z

@merrimanr Added the test case for searching an alert inside meta-alert. Also took the liberty to fix the timezone issue it was a one-liner.

nickwallen · 2017-11-22T16:57:25Z

@iraghumitra Are all these e2e tests passing for you? I am still getting failures. I manually truncated the metron_updates table before running them also.

**************************************************
*                    Failures                    *
**************************************************

1) metron-alerts tree view drag and drop should change group order
  - Expected '0,alerts_ui_e2e,ALERTS,91' to equal '0,alerts_ui_e2e,ALERTS,169' First Dash Row should be correct
  - Expected '' to equal '0 US (22)' Dash Group Values should be correct for US
  - Expected '' to equal '0 RU (44)' Dash Group Values should be present for RU
  - Expected '' to equal '0 FR (25)' Dash Group Values should be present for FR

2) metron-alerts tree view should have sort working for group details for multiple sub groups
  - Failed: stale element reference: element is not attached to the page document
    (Session info: chrome=62.0.3202.94)
    (Driver info: chromedriver=2.33.506106 (8a06c39c4582fbfbab6966dbb1c38a9173bfb1a2),platform=Mac OS X 10.13.1 x86_64)

3) metron-alerts tree view should have search working for group details for multiple sub groups
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

4) meta-alerts workflow should create a meta alert from nesting of more than one level
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
  - Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.

Executed 44 of 44 specs (4 FAILED) in 8 mins 46 secs.

iraghumitra · 2017-11-22T19:13:35Z

@nickwallen This is unfortunate I am not seeing these failures
Spec started

login to application
✓ should display error message for invalid credentials
✓ should login for valid credentials
✓ should logout

metron-alerts App
✓ should have all the UI elements
✓ should have all pagination controls and they should be working
✓ should have all settings controls and they should be working
✓ play pause should start polling and stop polling
✓ should select columns from table configuration
✓ should have all time-range controls
✓ should have all time range values populated - 1
✓ should have all time range values populated - 2
✓ should have all time range values populated - 3
✓ should have all time range values populated - 4
✓ should disable date picker when timestamp is present in search
✓ should have now included when to date is empty
✓ should have all time-range included while searching

metron-alerts configure table
✓ should select columns from table configuration
✓ should rename columns from table configuration

metron-alerts Search
✓ should display all the default values for saved searches
✓ should have all save search controls and they save search should be working
✓ should populate search items when selected on table
✓ should delete search items from search box
✓ should delete first search items from search box having multiple search fields
✓ manually entering search queries to search box and pressing enter key should search

metron-alerts tree view
✓ should have all group by elements
✓ drag and drop should change group order
✓ should have group details for single group by
✓ should have group details for multiple group by
✓ should have sort working for group details for multiple sub groups
✓ should have search working for group details for multiple sub groups

metron-alerts facets
✓ should display facets data
✓ should expand all facets
✓ should have all facet values
✓ should collapse all facets

metron-alerts alert status
✓ should change alert status for multiple alerts to OPEN
✓ should change alert status for multiple alerts to DISMISS
✓ should change alert status for multiple alerts to ESCALATE
✓ should change alert status for multiple alerts to RESOLVE
✓ should change alert status for multiple alerts to OPEN in tree view

metron-alerts alert status
✓ should change alert statuses
✓ should add comments for table view
✓ should add comments for tree view

meta-alerts workflow
✓ should have all the steps for meta alerts workflow
✓ should create a meta alert from nesting of more than one level

Executed 44 of 44 specs SUCCESS in 6 mins 31 secs.
[20:11:19] I/launcher - 0 instance(s) of WebDriver still running
[20:11:19] I/launcher - chrome #1 passed

~/incubator-metron/metron-interface/metron-alerts(METRON-1252*) » Date
Thu Nov 23 00:36:54 IST 2017`

Can you try them running again, UI tests are sometimes flaky. There seems to be timing issue. If you run them independently they work all the time, when we run them together they are flaky.
I am currently working on stabilizing them. If you notice some of the test cases have nothing to do with current PR :(

nickwallen · 2017-11-23T16:50:56Z

Just a quick update... I was working with @iraghumitra yesterday on getting the e2e tests to pass for me. It seems that with older versions of NPM the tests do pass, but then only various shades of success with newer versions. I don't totally understand why that is. Still working through it.

nickwallen · 2017-11-27T13:48:05Z

@iraghumitra Can you describe how you install Node/NPM on your development box? I want to install using the same mechanism (and versions) and see if I can get the e2e tests all working like you. Thanks

nickwallen · 2017-11-27T17:37:56Z

+1

I'd like to see sign-off from at least one other committer (if not more) before this gets merged.

I previously outlined the manual functional testing that I performed. All the core functionality is there. We also have sufficient test coverage for the functionality that was added.

I have been able to get the e2e tests to run good enough (IMO) to move forward with the PR. Based on the experimentation that I describe below, I believe that the test failures are due to the tests and test infrastructure and not the core functionality in the PR.

The intermittent failures are a pre-existing condition to this PR. This makes me comfortable enough to merge this PR and then follow-on with test fixes. @iraghumitra mentioned that he is currently working on improving the tests and will open a separate PR.

I used Brew to install the following versions of Node/NPM.
```
$ node --version
v8.9.1

$ npm --version
5.5.1
```
Before running the tests, truncate the metron_update table. The tests cannot currently clear out old test data stored in HBase.
Enabling/disabling sets of tests in metron-interface/metron-alerts/protractor.conf.js also allowed me to see that all the tests can pass on an intermittent basis. It seems that earlier test failures may cause later tests to fail unnecessarily.

Run the e2e tests using the following steps.

cd metron-interface/metron-alerts
npm install
./node_modules/.bin/webdriver-manager update
./node_modules/.bin/protractor

Thanks for working through this with me @iraghumitra.

merrimanr · 2017-11-27T19:35:22Z

I agree with @nickwallen. I think we're good to merge this as long as e2e tests are being addressed in a separate PR. +1

justinleet · 2017-11-27T21:43:08Z

I agree. I'm fine with going ahead with this, but I'd like to see end to end stability being addressed as the next UI priority, which I believe @iraghumitra is already doing some work on.

+1

nickwallen · 2017-11-27T22:13:36Z

Great. I will get this PR merged. I am glad to see that this one is ready to go.

iraghumitra and others added 4 commits October 17, 2017 19:00

Initial commit for grouping alerts into a meta alert

52d721e

Disallow comments for a alert in meta alert

09747a7

Fixed the existing tests

dba5493

Merge branch 'METRON-1252' of https://github.com/iraghumitra/incubato…

3b48f71

…r-metron into METRON-1252-review

merrimanr reviewed Oct 19, 2017

View reviewed changes

merrimanr added 2 commits October 20, 2017 13:56

Merge remote-tracking branch 'mirror/master' into METRON-1252-review

18c15f8

fixed several bugs

30cc762

merrimanr mentioned this pull request Oct 21, 2017

METRON-1252 bug fixes iraghumitra/incubator-metron#3

Merged

10 tasks

iraghumitra added 2 commits October 22, 2017 19:11

Merge pull request #3 from merrimanr/METRON-1252-review

d96915c

METRON-1252 bug fixes

Merge branch 'master' into METRON-1252

f704a1f

updated test messages

312e8e0

merrimanr and others added 2 commits November 21, 2017 09:28

Merge branch 'METRON-1252' of https://github.com/iraghumitra/incubato…

3fd64ab

…r-metron into missing-filters

Merge pull request #5 from merrimanr/missing-filters

607a6af

Missing intermediate filters

- Added test case to check if a meta alert is returned if we search …

982e833

…for a alert - Fixed issue with timezone in date picker

asfgit closed this in d07833a Nov 27, 2017

merrimanr mentioned this pull request Nov 28, 2017

METRON-1337: List of facets should not be hardcoded #853

Closed

10 tasks



		it('should have sort working for group details for multiple sub groups', () => {
		xit('should have sort working for group details for multiple sub groups', () => {

+                    </div>
+                  </div>
+                </div>
+              </div>

                
                    No newline at end of file

Metron-1252: Build ui for grouping alerts into meta alerts #803

Metron-1252: Build ui for grouping alerts into meta alerts #803

Uh oh!

Conversation

iraghumitra commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor Comments

Pull Request Checklist

For all changes:

For code changes:

For documentation related changes:

Note:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merrimanr commented Oct 19, 2017

Uh oh!

james-sirota commented Oct 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james-sirota commented Oct 20, 2017

Uh oh!

merrimanr commented Oct 21, 2017

Uh oh!

justinleet commented Oct 21, 2017

Uh oh!

iraghumitra commented Nov 21, 2017

Uh oh!

iraghumitra commented Nov 21, 2017

Uh oh!

nickwallen commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

iraghumitra commented Oct 17, 2017 •

edited

Loading

james-sirota commented Oct 19, 2017 •

edited

Loading

nickwallen commented Nov 21, 2017 •

edited

Loading

nickwallen commented Nov 21, 2017 •

edited

Loading

nickwallen commented Nov 21, 2017 •

edited

Loading

Executed 44 of 44 specs SUCCESS in 6 mins 31 secs.
[20:11:19] I/launcher - 0 instance(s) of WebDriver still running
[20:11:19] I/launcher - chrome #1 passed