from:"merrimanr"

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
As long as breakpoints are limited to code we maintain in Metron, I think 
it's just a matter of documenting how to set it up.  For example, I run REST 
locally in Intellij and debugging works fine against the in memory components 
or full dev.  I think the Storm topologies should continue to be run locally 
with LocalCluster so we're covered there too as along as the topologies can 
access the services running in Docker.  The only difference I can think of 
would be that now you can't put a breakpoint in Kafka or Elasticsearch server 
side code without enabling remote debugging.

I have no problem with this being a feature branch.  There is a lot of work 
to be done and design issues to work through.  The tradeoff of having this in a 
feature branch is that it will delay the ability to run the UI e2e tests in our 
build.  The tradeoff of doing it incrementally is that our build times will 
increase until we switch the tests over because we will have to use both in 
memory components and Docker.  I'm happy to do it either way.  


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
Point taken on maintaining 2 different solutions.  Which services would 
people want to enable remote debugging on?  I've only ever debugged Metron code 
so I don't know which services are important to people or why you would need to 
put breakpoints in core service code.  Enabling remote debugging will likely be 
a little bit different for each service since each project has their own 
mechanism for adding extra jvm params.  I'm happy to do a POC for this once I 
understand which services are important.

@ottobackwards I usually just leave mine running but spinning it up/down is 
really fast once all the images are loaded.



---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
@ottobackwards I don't see anything obviously wrong.  Maybe you could try 
pulling the latest commits or copying the one here:  
https://github.com/merrimanr/incubator-metron/blob/e2e-in-travis/metron-interface/metron-alerts/protractor.conf.js.
  Looks like your docker machine host is the same.


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
Docker Compose provides a CLI just like vagrant does:  
https://docs.docker.com/compose/reference/overview/.  I would be happy to wrap 
those commands in easy to understand scripts.


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
I would love to have a Debugging readme that is even more comprehensive 
than just integration tests.  Being able to easily debug is important to me and 
I do it all the time.

I had to do some debugging to convert this test and it was the same process 
I have used before.  Just put a break point in your test and run the test in 
Debug in Intellij.  Debugging Storm is a different animal and I suspect we 
would continue to use LocalCluster.  We would need to ensure the topology 
running locally in an IDE would still be able to interact with services in 
Docker.  I have successfully done this in the past for each of the topologies.

The only other case I can think of would be putting breakpoints in code of 
services we use (Kafka code for example).  Is that something we want to be able 
to do?  I can see some value although I wouldn't think that's a normal thing.

We could always keep the in memory module and offer a way to start them up 
in a separate process.  That way you could still debug everything in Intellij.  
I think I would prefer that option over enabling remote debugging in Docker 
containers.

And again, we don't have to use Docker.  We could just move the in memory 
stuff outside of the integration tests and add a layer to manage them in 
separate processes.  But that means we now have to implement some features 
Docker provides us with custom code.


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
Doesn't necessarily have to be Docker but yes you would have to start 
something up.  That step is now outside of the integration tests.  

We could keep the current in memory code and offer a debug mode.  Is there 
a benefit to using an in-memory component when debugging?


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
@ottobackwards I also added a way to run the e2e tests locally.  Once your 
Docker app is up and running locally, get the $DOCKER_HOST ip address and 
replace "localhost" with that ip address in 
`metron-interface/metron-alerts/protractor.conf.js` (should be 3 places you 
have to update).  Mine looks something like this:
```
exports.config = {
  ...
  baseUrl: 'http://192.168.99.100:4201/',
  ...
  params: {
rest: {
  url: '192.168.99.100:8082'
},
elasticsearch: {
  url: '192.168.99.100:9210'
}
  }
}
```

Then you can run the e2e tests with `cd metron-interface/metron-alerts && 
npm run e2e` 


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
@justinleet latest commit includes the changes I had to make to switch a 
preexisting integration test to the new infrastructure.  I verified it works 
locally, we'll see how it goes with Travis.

It was pretty straightforward and consisted of mainly 2 steps:

1. Remove the in memory components and replace any helper methods for 
setup/teardown
2. Add a namespace to all the test assets (indices and doc types in this 
case)

I believe converting other tests will follow this pattern as well.  To 
answer @ottobackwards's original question about 
[#854](https://github.com/apache/metron/pull/854), adding namespaces will be 
required when running the tests in parallel against shared infrastructure.  
Cleaning up at the beginning/end won't be enough.

This is meant only as an example, happy to revert it and add it in a future 
PR.


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
Sure a discussion on that topic is fine with me.  I'm referring to the 
existing integration tests.  


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
@justinleet I think that is a great idea.  I didn't do it initially to keep 
the size of this PR modest but I can add it in (we can always roll it back and 
move it to another PR if we want).  I'll start with metron-elasticsearch and 
see how it goes.


---

[GitHub] metron issue #858: METRON-1344: Externalize the infrastructural components u...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/858
  
Sorry @ottobackwards, you're right the instructions are a little vague.  
First, `metron-contrib/metron-docker-e2e/compose` is the directory you should 
be in.  I should have added that at the beginning.  Not all steps require you 
be in that directory but steps 4 and 5 do.

So you can't run the tests in the same exact way now because the Alerts UI 
host is hardcoded in the e2e tests (add that to our list of things to address). 
 It is hardcoded to "localhost" which works fine in travis but won't work in 
Mac OSX because of networking limitations (Docker has to run in a vagrant 
machine).  Let me come up with a plan that will allow you to easily run the e2e 
tests locally.


---

[GitHub] metron pull request #858: METRON-1344: Externalize the infrastructural compo...

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

https://github.com/apache/metron/pull/858#discussion_r155339646
  
--- Diff: metron-interface/metron-alerts/pom.xml ---
@@ -130,4 +146,44 @@
 
 
 
+  
+
+
+  e2e
+  
+
+  
--- End diff --

Once we get closer to consensus on what this is going to look like, I 
promise I will add extensive documentation.


---

[GitHub] metron pull request #858: METRON-1344: Externalize the infrastructural compo...

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

https://github.com/apache/metron/pull/858#discussion_r155339627
  
--- Diff: metron-interface/metron-alerts/e2e/utils/e2e_util.ts ---
@@ -46,25 +48,53 @@ export function waitForStalenessOf (_element ) {
 }
 
 export function loadTestData() {
--- End diff --

Agreed


---

[GitHub] metron pull request #857: METRON-1340: Improve e2e tests for metron alerts

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

https://github.com/apache/metron/pull/857#discussion_r155261159
  
--- Diff: metron-interface/metron-alerts/src/app/utils/constants.ts ---
@@ -38,5 +38,6 @@ export let TREE_SUB_GROUP_SIZE = 5;
 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(',') 
: [];
+export let POLLING_DEFAULT_STATE = environment.defaultPollingState;
--- End diff --

Would it be possible to change this to a better name?  Something like 
DISABLE_POLLING?  Setting POLLING_DEFAULT_STATE to true to stop polling doesn't 
make sense to me.


---

[GitHub] metron pull request #858: METRON-1344: Externalize the infrastructural compo...

GitHub user merrimanr opened a pull request:

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

METRON-1344: Externalize the infrastructural components using integration
tests

## Contributor Comments
This PR will add infrastructure to our Travis build that will allow the
Alerts UI e2e test to be run. There are several outstanding issues that still
need to be worked out so DO NOT MERGE this yet.

This is a first pass at a potential Docker-based solution and is meant to
be a POC. The intention is to facilitate further discussion around the general
approach and provide a working example that we can build off of.

A good place to start is the .travis file. This provides a good guide on
how this infrastructure is spun up. It is assumed Docker and Docker Compose
are installed. To use outside of travis in a local dev environment (assuming
Mac OSX):

1. Build Metron with `mvn clean install -DskipTests`
1. Create a Docker machine with
`metron-contrib/metron-docker/scripts/create-docker-machine.sh`
1. Set the Docker env variables with `eval $(docker-machine env
metron-machine)`
1. Build the base Metron image that installs Java: `docker build
./metron-centos/ -t "metron-centos"`
1. Spin up the environment: `cd metron-contrib/metron-docker-e2e/compose
&& docker-compose up -d`

A working environment should now be available. To verify get the Docker
machine address with `echo $DOCKER_HOST`. Services should be available on the
ports specified in
`metron-contrib/metron-docker-e2e/compose/docker-compose.yml`. For example,
assuming my $DOCKER_HOST is "tcp://192.168.99.100:2376", Elasticsearch should
be available at http://192.168.99.100:9210.

At this point only a single e2e test is run due to the significant
refactoring being done in https://github.com/apache/metron/pull/857.

Here are my thoughts so far based on work in this PR:

- The Docker environment creation and startup adds about 3 minutes to the
build. This is about what I expected and hopefully we can get this time back
as we move other tests to a reusable environment.
- I'm not convinced spinning up containers for the Alerts UI and REST are
necessary or desired. We may be able to cut some time off the build by running
them directly on the Travis host instead of in Docker
- I experimented with caching the Docker images but pulling them every time
was actually faster

There is still significant work to be done including:
- I was able to get the full suite of tests to run successfully in previous
commits but have all but e2e tests commented out for now to make it easier to
see what's going on in Travis. These will need to be added back once we get
closer to a final solution.
- Once the e2e test refactoring is done those changes will need merged in
and tested
- The license check is commented out right now because I inadvertently
added some new dependency versions (have no idea why). Still need to track
that down.
- There is an intermittent error that happens when starting up REST. Still
working on tracking this down.

Looking forward to some 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- where 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 de

[GitHub] metron issue #857: METRON-1340: Improve e2e tests for metron alerts

2017-12-05 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/857
  
Took a first pass at this and I feel like the e2e tests are much improved.  
Great progress and good job so far.  I was able to get several successful runs 
whereas before it was difficult to get a single successful run.

A couple of comments/suggestions:

- I noticed a warning message stating I wasn't on a recent enough version 
of nodejs.  I think we should be using the maven frontend plugin to run tests 
so that we guarantee a consistent version is used.  I submitted a 
[PR](https://github.com/iraghumitra/incubator-metron/pull/6) as an example of 
how to do this.

- I noticed there are still several browser.sleep statements throughout the 
tests (I counted 20).  I think our goal should be to remove all of them.  I 
know some of these should definitely be removed (alerts-list.po.ts, line 87) 
and may have just been missed.  If there are cases where we MUST have them, I 
think those cases needed to be discussed and justified.

- I feel like the "should expand all facets" and "should collapse all 
facets" tests in alert-filters.e2e-spec.ts are unnecessary (and would allow us 
to remove a couple browser.sleep statements).  These tests are simply opening 
and closing bootstrap widgets which is controlled by code we don't maintain 
(bootstrap).  I would instead prefer a test that selects a filter and checks 
that the search box and results are properly updated. 

- I have ran into these errors a couple times.  Not sure if I just ended up 
in a bad state somehow or if it's because I'm running tests through Maven:
```
â should have all the steps for meta alerts workflow
- Failed: unknown error: Element  is not clickable at point (1278, 102). Other element 
would receive the click: ...

â should create a meta alert from nesting of more than one level
- Failed: unknown error: Element  is not 
clickable at point (1350, 503). Other element would receive the click: ...
```




---

[GitHub] metron pull request #857: METRON-1340: Improve e2e tests for metron alerts

2017-12-05 Thread merrimanr

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

https://github.com/apache/metron/pull/857#discussion_r155067123
  
--- Diff: metron-interface/metron-alerts/src/app/utils/constants.ts ---
@@ -38,5 +38,6 @@ export let TREE_SUB_GROUP_SIZE = 5;
 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(',') 
: [];
+export let POLLING_DEFAULT_STATE = environment.defaultPollingState;
--- End diff --

What does the POLLING_DEFAULT_STATE setting do?


---

[GitHub] metron pull request #853: METRON-1337: List of facets should not be hardcode...

2017-11-28 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1337: List of facets should not be hardcoded

## Contributor Comments
This PR makes the list of facet fields in the Alerts UI configurable.
There is now a "search.facet.fields" setting in
https://github.com/apache/metron/blob/master/metron-interface/metron-rest/src/main/resources/application.yml
that is a comma-separate list of fields to be used as facets. Originally a
comment was made that the "host" field wasn't commonly used so I removed that
from the default list.

I can think of two options for exposing this configuration:
- through an AlertProfile that is specific to each user, stored in a
relational database and can be edited through REST
- a setting that is exposed in Ambari

I chose to include the first option in this PR to get the conversation
going. Is one of these preferable? The AlertProfile approach allows this
setting to be changed at runtime and each user has their own list of facet
fields. But it is not versioned like it would if it were in Ambari. Do we
prefer one over the other? Do we want both with Ambari being the default when
an AlertProfile doesn't exist for a user? Are there other options I'm not
thinking of?

This works similar to how default search indices work: it is managed in
the REST layer and can be overriden by including facet fields in a search
request. However it seemed useful to allow a way to explicitly NOT include
facets in a query so it works slightly different than indices. A missing
facetFields property in the request will use the defaults while an empty array
will disable facets. A missing indices property or an empty array will use the
default indices. Is this the correct behavior?

This has been tested in full dev and the UI e2e tests pass when run in
isolation. There is currently an effort to stabilize the e2e tests as a follow
on to https://github.com/apache/metron/pull/803 so I did not try to solve that
here.

I will add some documentation around configuring the facet field list and
facetFields behavior in a search request once we come to a consensus on the
solution.

Another issue I would like to point out. When I added facetFields to the
AlertProfile object it required a database update because that new field needed
to be added to the database. This would become an issue if someone were
upgrading from a previous version. Is this acceptable if we document it for
future upgrades? Is a relational database the right solution or should we
consider a more flexible storage option?

## Pull Request Checklist

In order to streamline the review of the contribution we ask you follow
these guidelines and ask you to double check the following:

- [x] Does your PR title start with METRON- where 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:
- [x] Have you included steps to reproduce the behavior or problem that is
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified
and tested manually?
- [x] 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 comm

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

2017-11-27 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
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


---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
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.


---

[GitHub] metron issue #843: METRON-1319: Column Metadata REST service should use defa...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/843
  
Sorry I understand now.  Yes I did end up running this in full dev.  I 
don't mind doing it again to make sure.


---

[GitHub] metron issue #843: METRON-1319: Column Metadata REST service should use defa...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/843
  
The latest commit only includes comments and an additional LOG statement.  
I didn't think rebuilding full dev was necessary.


---

[GitHub] metron pull request #843: METRON-1319: Column Metadata REST service should u...

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

https://github.com/apache/metron/pull/843#discussion_r152347240
  
--- Diff: 
metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/SearchServiceImpl.java
 ---
@@ -96,6 +96,11 @@ public GroupResponse group(GroupRequest groupRequest) 
throws RestException {
   @Override
   public Map<String, FieldType> getColumnMetadata(List indices) 
throws RestException {
 try {
+  if (indices == null || indices.isEmpty()) {
+indices = getDefaultIndices();
+// metaalerts should be included by default in column metadata 
requests
+indices.add(METAALERT_TYPE);
--- End diff --

Meta alerts should not be included in group by queries.


---

[GitHub] metron pull request #843: METRON-1319: Column Metadata REST service should u...

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

https://github.com/apache/metron/pull/843#discussion_r152347150
  
--- Diff: 
metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/SearchControllerIntegrationTest.java
 ---
@@ -132,6 +132,30 @@ public void testDefaultQuery() throws Exception {
 sensorIndexingConfigService.delete("bro");
   }
 
+  @Test
+  public void testDefaultColumnMetadata() throws Exception {
--- End diff --

Done


---

[GitHub] metron pull request #843: METRON-1319: Column Metadata REST service should u...

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

https://github.com/apache/metron/pull/843#discussion_r152347116
  
--- Diff: 
metron-interface/metron-rest/src/main/java/org/apache/metron/rest/service/impl/SearchServiceImpl.java
 ---
@@ -96,6 +96,11 @@ public GroupResponse group(GroupRequest groupRequest) 
throws RestException {
   @Override
   public Map<String, FieldType> getColumnMetadata(List indices) 
throws RestException {
 try {
+  if (indices == null || indices.isEmpty()) {
+indices = getDefaultIndices();
--- End diff --

Done


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r152311299
  
--- Diff: 
metron-interface/metron-alerts/e2e/alerts-list/meta-alerts/meta-alert.po.ts ---
@@ -0,0 +1,43 @@
+/**
+ * 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} from 'protractor';
+
+export class MetaAlertPage {
+
+  getPageTitle() {
+return element(by.css('app-meta-alerts .form-title')).getText();
+  }
+
+  getMetaAlertsTitle() {
+return element(by.css('app-meta-alerts .title')).getText();
+  }
+
+  getAvailableMetaAlerts() {
+return element(by.css('app-meta-alerts .guid-name-container 
div')).getText();
+  }
+
+  selectRadio() {
+return element.all(by.css('app-meta-alerts .checkmark')).click();
+  }
+
+  addToMetaAlert() {
+
element.all(by.css('app-meta-alerts')).get(0).element(by.buttonText('ADD')).click();
+browser.sleep(2000);
--- End diff --

There are several sleep statements throughout the e2e tests.  I would be ok 
with cleaning them all up in follow-on PR.  


---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

2017-11-20 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
I've verified the bug reported by Justin happens when you create a meta 
alert from a group that is nested by more than 1 level.  Creating a meta alert 
from a top level group works.


---

[GitHub] metron pull request #843: METRON-1319: Column Metadata REST service should u...

2017-11-17 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1319: Column Metadata REST service should use default indices on 
empty input

## Contributor Comments
This PR adjusts the Column Metadata REST service to use a list of default 
indices when an empty list is passed in.  This behavior is similar to the 
search method and keeps the Alerts UI from having to track the current list of 
available indices.

I added test cases and verified the relevant tests in our Alerts UI 
end-to-end test now pass.  Still need to spin up full dev and validate there.  
Will report back once I have successfully done that but this can be reviewed in 
the meantime.

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] 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 METRON-1319

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

https://github.com/apache/metron/pull/843.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 #843


commit ac4d68eea31caa1fa1a8f2f97c4d21256d7220e8
Author: merrimanr <merrim...@gmail.com>
Date:   2017-11-17T21:36:00Z

initial commit




---

[GitHub] metron issue #832: METRON-1301 Sorting on Triage Score Unexpectedly Filters ...

2017-11-17 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/832
  
+1 worked as advertised.  Thanks @nickwallen!


---

[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/827
  
The latest commit fixes the bug @justinleet found and adds some test cases 
to cover it.  

I also removed an "ignoredIndices" variable from the ElasticsearchDao 
class.  This was being used to exclude certain indices but I believe we've 
moved towards a white list approach for specifying indices so this no longer 
makes sense.  I can revert if anyone disagrees.


---

[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/827
  
@JonZeolla I think we should just remove it for now and I've done that in 
the latest commit.  I'm not entirely clear on what we should expect in this 
case and we don't need it right now anyways.  Better to implement it the right 
way once we are clear on how it should behave in my opinion.

Good catch @justinleet!  I did not think of this test case and to my 
surprise the Elasticsearch API returns ALL field mappings if you pass in an 
empty array of indices.  I will fix this and add a test case.  You are correct 
in that we should expect correctly formatted ip addresses in "facetCounts".  
Before this PR they would have been in long format when you include an index 
that does not have a mapping for the ip_src_addr field.


---

[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/827
  
Was commenting while your comment posted.  I think your latest suggestion 
is the right answer :)


---

[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/827
  
Actually the more I think about it, I would not consider a field to be 
common between indices when the types mismatch.  Hard to say what the right 
answer is because there is no real requirement driving this.


---

[GitHub] metron issue #827: METRON-1294: IP addresses are not formatted correctly in ...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/827
  
I don't think that method is even used anymore but I will add the mismatch 
handling just in case.


---

[GitHub] metron pull request #825: METRON-1290: Only first 10 alerts are update when ...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #826: METRON-1291: Kafka produce REST endpoint does not ...

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

https://github.com/apache/metron/pull/826#discussion_r151450858
  
--- Diff: 
metron-interface/metron-rest/src/main/java/org/apache/metron/rest/config/KafkaConfig.java
 ---
@@ -108,6 +108,9 @@ public ZkUtils zkUtils() {
 producerConfig.put("key.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("value.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("request.required.acks", 1);
+if 
(environment.getProperty(MetronRestConstants.KERBEROS_ENABLED_SPRING_PROPERTY, 
Boolean.class, false)) {
+  producerConfig.put("security.protocol", "SASL_PLAINTEXT");
--- End diff --

Thanks @cestella.  Turns out this setting was already available so it was a 
simple change.  I also added the default application.yml and adjusted the unit 
test.


---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

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

https://github.com/apache/metron/pull/824#discussion_r150995571
  
--- Diff: 
metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java
 ---
@@ -138,9 +142,17 @@ private Document getDocumentFromResult(Result result) 
throws IOException {
 Map.Entry<byte[], byte[]> entry= columns.lastEntry();
 Long ts = Bytes.toLong(entry.getKey());
 if(entry.getValue()!= null) {
-  Map<String, Object> json = JSONUtils.INSTANCE.load(new 
String(entry.getValue()), new TypeReference<Map<String, Object>>() {
-  });
-  return new Document(json, Bytes.toString(result.getRow()), (String) 
json.get(SOURCE_TYPE), ts);
+  Map<String, Object> json = JSONUtils.INSTANCE.load(new 
String(entry.getValue()),
+  new TypeReference<Map<String, Object>>() {});
+  ByteArrayInputStream baos = new 
ByteArrayInputStream(result.getRow());
--- End diff --

I initially tried to do this but if felt sort of awkward since we need to 
return both the guid and sensorType.  Would it make sense to just return a 
Document with those fields set and set the other fields after that method is 
called?  Or we could accept the json and ts as inputs to that method too.


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
The latest commit fixes the group by bug found by @iraghumitra and 
addresses the feedback 
[above](https://github.com/apache/metron/pull/824#pullrequestreview-76466048).

The group by fix was simple.  We are now just excluding the "metaalert" 
sensor type from group by queries.

Refactoring the HBaseDao was more involved.  One of the side effects of 
storing the sensorType in the row key is that now we require a map of guids to 
sensorTypes for an IndexDao.getAllLatest call.  These means the 
IndexDao.getAllLatest interface needed to change.  It made sense to just create 
a standard interface that can be used for looking up records since it is used 
in many different places and can be confusing as to what is needed to perform 
an operation on a document.  So this PR introduces a GetRequest that has the 
following structure:
```
{
  "guid": "some guid",
  "sensorType" : " some sensor type",
  "index": "some optional index, not required"
}
```

Since now keep the sensorType in the row key it is required for a look up.  
HBase still doesn't have any concept of an index so that is kept optional.  In 
the ElasticsearchDao this is handled by using the index of a GetRequest if 
present or looking it up in Elasticsearch if not.  Now it should be clear what 
is needed to get a document since the interface is more consistent across that 
DAO methods.

Because of the GetRequest change the testing process and REST interface has 
changed slightly.  The PR description has been updated to reflect the changes.  
I am still planning on doing a fresh vagrant build but this should be ready for 
testing.


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
@justinleet looking at your results I would expect the count to go down by 
2 instead of 1:  1 for the alert that was added to the meta alert and another 
for the meta alert itself since they shouldn't be included in group by queries. 
 I think the bug @iraghumitra reported is valid.


---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

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

https://github.com/apache/metron/pull/824#discussion_r150889867
  
--- Diff: 
metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java
 ---
@@ -135,8 +138,9 @@ private Document getDocumentFromResult(Result result) 
throws IOException {
 Map.Entry<byte[], byte[]> entry= columns.lastEntry();
 Long ts = Bytes.toLong(entry.getKey());
 if(entry.getValue()!= null) {
-  String json = new String(entry.getValue());
-  return new Document(json, Bytes.toString(result.getRow()), null, ts);
+  Map<String, Object> json = JSONUtils.INSTANCE.load(new 
String(entry.getValue()), new TypeReference<Map<String, Object>>() {
+  });
+  return new Document(json, Bytes.toString(result.getRow()), (String) 
json.get(SOURCE_TYPE), ts);
--- End diff --

I like this solution much better.


---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

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

https://github.com/apache/metron/pull/824#discussion_r150874132
  
--- Diff: 
metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java
 ---
@@ -135,8 +138,9 @@ private Document getDocumentFromResult(Result result) 
throws IOException {
 Map.Entry<byte[], byte[]> entry= columns.lastEntry();
 Long ts = Bytes.toLong(entry.getKey());
 if(entry.getValue()!= null) {
-  String json = new String(entry.getValue());
-  return new Document(json, Bytes.toString(result.getRow()), null, ts);
+  Map<String, Object> json = JSONUtils.INSTANCE.load(new 
String(entry.getValue()), new TypeReference<Map<String, Object>>() {
+  });
+  return new Document(json, Bytes.toString(result.getRow()), (String) 
json.get(SOURCE_TYPE), ts);
--- End diff --

That seems generally useful but it's a pretty significant change (several 
files and several lines of code) plus it adds another setting to our config.  
There is no unit test for IndexConfig so it also adds untested code (or would 
require writing a new test).

I prefer a simple 2-3 line change that we can easily revert when ES 5 
upgrade makes it in.  This should be a separate PR in my opinion.  This PR is 
already very large and complicated.


---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

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

https://github.com/apache/metron/pull/824#discussion_r150866003
  
--- Diff: 
metron-platform/metron-indexing/src/main/java/org/apache/metron/indexing/dao/HBaseDao.java
 ---
@@ -135,8 +138,9 @@ private Document getDocumentFromResult(Result result) 
throws IOException {
 Map.Entry<byte[], byte[]> entry= columns.lastEntry();
 Long ts = Bytes.toLong(entry.getKey());
 if(entry.getValue()!= null) {
-  String json = new String(entry.getValue());
-  return new Document(json, Bytes.toString(result.getRow()), null, ts);
+  Map<String, Object> json = JSONUtils.INSTANCE.load(new 
String(entry.getValue()), new TypeReference<Map<String, Object>>() {
+  });
+  return new Document(json, Bytes.toString(result.getRow()), (String) 
json.get(SOURCE_TYPE), ts);
--- End diff --

Option 1 is not possible the way things are now.  This constant is already 
in an ES specific class in metron-elasticsearch but metron-elasticsearch 
depends on metron-indexing.  To get access to that constant we would need to 
add metron-elasticsearch as a dependency to metron-indexing thus creating a 
circular dependency.

Option 2 would be my least favorite option and I would rather just change 
the method signature of getAllLatest to include all guid/sensorType 
relationships.

Since we currently have a PR in review for the ES 5 upgrade that will allow 
us to just remove this constant, I don't feel like we should spend much time on 
it.  I would also argue that this was an issue long before this PR so anything 
more than a simple workaround should be a follow-on.


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
I also found what I believe are 2 fairly significant architectural issues 
with our DAO abstraction:
- https://issues.apache.org/jira/browse/METRON-1314
- https://issues.apache.org/jira/browse/METRON-1315



---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
The latest commit fixes the bug in the previous comment.  The root cause 
was that HBaseDao.getAllLatest was not properly returning the sensor type and 
causing subsequent updates in Elasticsearch to fail.  The solution was simple:  
look up the sensor type from the document and add it to the return Document.  I 
added a constant for this field (essentially a copy of 
ElasticsearchMetaAlertDao.SOURCE_TYPE) to the HBaseDao in anticipation of the 
ES 5 upgrade where it will no longer be required to change .'s to :'s.  At that 
point we can just change the constant to Constants.SENSOR_TYPE and it should 
continue to function.  

There are a couple other solutions to this problem that I can think of:
- change the getAllLatest interface to include guid/sensorType mappings 
instead of separate guid and sensorType lists
- add a column family to store the sensor type
- other more complicated ways of getting the correct sensor type field name

I felt this temporary constant was the simplest but feel free to give 
opinions on other options.  I also update the HBaseDaoIntegrationTest to expect 
this field to be returned.

The PR description has been updated to reflect the various interface 
changes and new testing procedure.


---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

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

https://github.com/apache/metron/pull/824#discussion_r150644110
  
--- Diff: 
metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/InMemoryMetaAlertDao.java
 ---
@@ -200,4 +207,23 @@ public MetaAlertCreateResponse 
createMetaAlert(MetaAlertCreateRequest request)
 createResponse.setCreated(true);
 return createResponse;
   }
+
+  @Override
+  public boolean addAlertsToMetaAlert(String metaAlertGuid, 
Collection alertGuids,
+  Collection sensorTypes) throws IOException {
+return true;
--- End diff --

I think @cestella's suggestion is good, I will add some REST integration 
tests that ensure request/responses are serialized/deserialized correctly.  
There are tests included in MetaAlertControllerIntegrationTest that exercise 
these functions.


---

[GitHub] metron pull request #826: METRON-1291: Kafka produce REST endpoint does not ...

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

https://github.com/apache/metron/pull/826#discussion_r150560129
  
--- Diff: 
metron-interface/metron-rest/src/main/java/org/apache/metron/rest/config/KafkaConfig.java
 ---
@@ -108,6 +108,9 @@ public ZkUtils zkUtils() {
 producerConfig.put("key.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("value.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("request.required.acks", 1);
+if 
(environment.getProperty(MetronRestConstants.KERBEROS_ENABLED_SPRING_PROPERTY, 
Boolean.class, false)) {
+  producerConfig.put("security.protocol", "SASL_PLAINTEXT");
--- End diff --

`metron.j2` is always available to REST.  KAFKA_SECURITY_PROTOCOL env 
variable may or may not be depending on if a Kafka Broker or Client is located 
on the same host (am I wrong?).


---

[GitHub] metron pull request #826: METRON-1291: Kafka produce REST endpoint does not ...

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

https://github.com/apache/metron/pull/826#discussion_r150549974
  
--- Diff: 
metron-interface/metron-rest/src/main/java/org/apache/metron/rest/config/KafkaConfig.java
 ---
@@ -108,6 +108,9 @@ public ZkUtils zkUtils() {
 producerConfig.put("key.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("value.serializer", 
"org.apache.kafka.common.serialization.StringSerializer");
 producerConfig.put("request.required.acks", 1);
+if 
(environment.getProperty(MetronRestConstants.KERBEROS_ENABLED_SPRING_PROPERTY, 
Boolean.class, false)) {
+  producerConfig.put("security.protocol", "SASL_PLAINTEXT");
--- End diff --

Is the KAFKA_SECURITY_PROTOCOL env variable guaranteed to be available on 
the the host where REST is installed?  Either way I don't have a problem with 
making the Kafka security protocol configurable vs hard-coded.  I would propose 
we manage it like the other REST properties though and put it in 
`metron-deployment/packaging/ambari/metron-mpack/src/main/resources/common-services/METRON/CURRENT/package/templates/metron.j2`
 to ensure it's always available to REST.  Would it make sense to remove the 
"kerberos.enabled" spring property in this case?


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

2017-11-10 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
The latest commits address some outstanding tasks and address a couple 
other issues:
- disabling updates to meta alert objects means we can't update a meta 
alert name or comments (or other fields that may be added in the future).  The 
patch implementation was added to the ElasticsearchMetaAlertDao to allow 
updates to fields other than `alert` and `status` which require special 
handling and have dedicated endpoints
- a `getAllLatest` implementation was added to the HBaseDao along with an 
integration test
- a bug was discovered where the number of search results returned defaults 
to 10 meaning changes to alerts are only propagated to the first 10 meta alerts 
and vice-versa.  This was corrected in the ElasticsearchDao.searchByGuids by 
explicitly setting the size and fixed in the alert/meta alert lookup queries by 
paging through all the results if necessary.  A test was added to the 
integration test to catch this case in the future.

I feel like this PR is in pretty good shape from a functional perspective.  
Still working on javadocs/documentation but it's ready for functional 
testing/code review.




---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

2017-11-10 Thread merrimanr

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

https://github.com/apache/metron/pull/824#discussion_r150239332
  
--- Diff: 
metron-platform/metron-elasticsearch/src/main/java/org/apache/metron/elasticsearch/dao/ElasticsearchDao.java
 ---
@@ -256,59 +256,91 @@ public Document getLatest(final String guid, final 
String sensorType) throws IOE
 return ret.orElse(null);
   }
 
+  @Override
+  public Iterable getAllLatest(final Collection guids, 
final Collection sensorTypes) throws IOException {
+List documents = searchByGuids(
+guids
+, sensorTypes
+, hit -> {
+  Long ts = 0L;
+  String doc = hit.getSourceAsString();
+  String sourceType = 
Iterables.getFirst(Splitter.on("_doc").split(hit.getType()), null);
+  try {
+return Optional.of(new Document(doc, hit.getId(), sourceType, 
ts));
+  } catch (IOException e) {
+throw new IllegalStateException("Unable to retrieve latest: " 
+ e.getMessage(), e);
+  }
+}
+
+);
+return documents;
+  }
+
+   Optional searchByGuid(String guid, String sensorType,
+  Function<SearchHit, Optional> callback) {
+Collection sensorTypes = sensorType != null ? 
Collections.singleton(sensorType) : null;
+List results = searchByGuids(Collections.singleton(guid), 
sensorTypes, callback);
+if (results.size() > 0) {
+  return Optional.of(results.get(0));
+} else {
+  return Optional.empty();
+}
+  }
+
   /**
* Return the search hit based on the UUID and sensor type.
* A callback can be specified to transform the hit into a type T.
* If more than one hit happens, the first one will be returned.
*/
-   Optional searchByGuid(String guid, String sensorType,
+   List searchByGuids(Collection guids, Collection 
sensorTypes,
   Function<SearchHit, Optional> callback) {
 QueryBuilder query;
-if (sensorType != null) {
-  query = QueryBuilders.idsQuery(sensorType + "_doc").ids(guid);
+if (sensorTypes != null) {
+  String[] types = sensorTypes.stream().map(sensorType -> sensorType + 
"_doc").toArray(String[]::new);
+  query = QueryBuilders.idsQuery(types).ids(guids);
 } else {
-  query = QueryBuilders.idsQuery().ids(guid);
+  query = QueryBuilders.idsQuery().ids(guids);
 }
 SearchRequestBuilder request = client.prepareSearch()
--- End diff --

Good catch.  I will add a fix and test case.


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

2017-11-09 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
The 2nd most recent commits updates the MetaAlertDao interface by adding 
additional methods for interacting with metaalerts.   This should make it 
easier to use and make the underlying implementation simpler and easier to 
understand by eliminating the need for complex logic that must catch all of the 
different use cases in the MetaAlertDao.update.  The high-level changes include:
- dedicated methods that allow adding/removing alerts to/from a metaalert
- dedicated method for changing the status of a metaalert
- direct updates through IndexDao.update are no longer allowed on 
metaalerts, the other MetaAlertDao methods must be used instead

The latest commit updates the ElasticsearchMetaAlertIntegrationTest with 
several changes:
- test data setup and test style is now consistent across all tests
- test cases for the new interface methods were added
- old test cases were reconciled with the new test cases
- test coverage for ElasticsearchMetaAlertDao is now close to 100% with 
tests added for all the bugs found so far

I am still working on adding these changes:
- updated javadocs and comments in all relevant classes and tests
- an HBaseDao.getAllLatest implementation and test
- updated ElasticsearchMetaAlertDaoTest adapted for the new MetaAlertDao 
interface (it's currently all commented out for the purpose of running the 
integration tests)

For now reviewers can start looking at the new MetaAlertDao interface 
methods and the accompanying integration tests in 
ElasticsearchMetaAlertIntegrationTest.  Hope to have the rest pushed out soon.


---

[GitHub] metron issue #824: METRON-1289: Alert fields are lost when a MetaAlert is cr...

2017-11-09 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/824
  
No problem.  I am still actively working on this PR and will add the 
HBaseDao.findAllLatest implementation task to my list.


---

[GitHub] metron issue #829: METRON-1296 Full Dev Fails to Deploy Index Templates

2017-11-07 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/829
  
+1.  I agree with Casey that this should fail instead of WARN.  If 
templates are not installed it puts us in a bad state and requires manual 
intervention that may not be obvious.


---

[GitHub] metron pull request #827: METRON-1294: IP addresses are not formatted correc...

2017-11-06 Thread merrimanr

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

https://github.com/apache/metron/pull/827#discussion_r149177390
  
--- Diff: 
metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/SearchControllerIntegrationTest.java
 ---
@@ -236,17 +236,14 @@ public void test() throws Exception {
 
.andExpect(jsonPath("$.groupResults[0].groupResults[0].score").value(50));
 
 this.mockMvc.perform(post(searchUrl + 
"/column/metadata").with(httpBasic(user, 
password)).with(csrf()).contentType(MediaType.parseMediaType("application/json;charset=UTF-8")).content("[\"bro\",\"snort\"]"))
-.andExpect(status().isOk())
-
.andExpect(content().contentType(MediaType.parseMediaType("application/json;charset=UTF-8")))
-.andExpect(jsonPath("$.*", hasSize(2)))
-
.andExpect(jsonPath("$.bro.common_string_field").value("string"))
-
.andExpect(jsonPath("$.bro.common_integer_field").value("integer"))
-.andExpect(jsonPath("$.bro.bro_field").value("boolean"))
-.andExpect(jsonPath("$.bro.duplicate_field").value("date"))
-
.andExpect(jsonPath("$.snort.common_string_field").value("string"))
-
.andExpect(jsonPath("$.snort.common_integer_field").value("integer"))
-.andExpect(jsonPath("$.snort.snort_field").value("double"))
-.andExpect(jsonPath("$.snort.duplicate_field").value("long"));
+.andExpect(status().isOk())
--- End diff --

Yes the `SearchControllerIntegrationTest` does not actually exercise the 
ElasticsearchDAO and I find this confusing as well.  Really all the 
`SearchControllerIntegrationTest` tests is that serialization/deserialization 
happens correctly (which is still valuable).


---

[GitHub] metron pull request #827: METRON-1294: IP addresses are not formatted correc...

2017-11-06 Thread merrimanr

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

https://github.com/apache/metron/pull/827#discussion_r149176509
  
--- Diff: 
metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/InMemoryDao.java
 ---
@@ -221,12 +222,24 @@ public void batchUpdate(Map<Document, 
Optional> updates) throws IOExcept
 }
   }
 
-  public Map<String, Map<String, FieldType>> 
getColumnMetadata(List indices) throws IOException {
-Map<String, Map<String, FieldType>> columnMetadata = new HashMap<>();
+  @Override
+  public Map<String, FieldType> getColumnMetadata(List indices) 
throws IOException {
+Map<String, FieldType> indexColumnMetadata = new HashMap<>();
 for(String index: indices) {
-  columnMetadata.put(index, new HashMap<>(COLUMN_METADATA.get(index)));
+  Map<String, FieldType> columnMetadata = COLUMN_METADATA.get(index);
+  for (Entry entry: columnMetadata.entrySet()) {
+String field = (String) entry.getKey();
+FieldType type = (FieldType) entry.getValue();
+if (indexColumnMetadata.containsKey(field)) {
+  if (!type.equals(indexColumnMetadata.get(field))) {
+indexColumnMetadata.remove(field);
--- End diff --

As far as I know the only place the UI uses this endpoint directly is when 
it presents a list of fields a user can select to display in the UI.  I think 
we would want it to show up there so including it with a type set to OTHER 
makes sense to me. 

If you're ok with it I will proceed with this change and add a big fat 
`LOG.error` call.


---

[GitHub] metron pull request #827: METRON-1294: IP addresses are not formatted correc...

2017-11-06 Thread merrimanr

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

https://github.com/apache/metron/pull/827#discussion_r149156555
  
--- Diff: 
metron-platform/metron-indexing/src/test/java/org/apache/metron/indexing/dao/InMemoryDao.java
 ---
@@ -221,12 +222,24 @@ public void batchUpdate(Map<Document, 
Optional> updates) throws IOExcept
 }
   }
 
-  public Map<String, Map<String, FieldType>> 
getColumnMetadata(List indices) throws IOException {
-Map<String, Map<String, FieldType>> columnMetadata = new HashMap<>();
+  @Override
+  public Map<String, FieldType> getColumnMetadata(List indices) 
throws IOException {
+Map<String, FieldType> indexColumnMetadata = new HashMap<>();
 for(String index: indices) {
-  columnMetadata.put(index, new HashMap<>(COLUMN_METADATA.get(index)));
+  Map<String, FieldType> columnMetadata = COLUMN_METADATA.get(index);
+  for (Entry entry: columnMetadata.entrySet()) {
+String field = (String) entry.getKey();
+FieldType type = (FieldType) entry.getValue();
+if (indexColumnMetadata.containsKey(field)) {
+  if (!type.equals(indexColumnMetadata.get(field))) {
+indexColumnMetadata.remove(field);
--- End diff --

I agree with you that it would be confusing but I'm not sure what the 
correct behavior should be.  Should we include the field but just set the type 
to OTHER?  This is how the ElasticsearchDao treats fields it doesn't have type 
information for but so it might be better to explicitly state this in the 
column metadata endpoint response.


---

[GitHub] metron pull request #827: METRON-1294: IP addresses are not formatted correc...

2017-11-03 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1294: IP addresses are not formatted correctly in facet and group
results

## Contributor Comments
By default, values in facet and group by results are not formatted
correctly in some cases (IP addresses being one) so the ElasticsearchDao must
correctly format them instead. To do this it must know the type of each
facet/group column to know how to correctly format them. The root cause of
this issue is that the ElasticsearchDao was retrieving "common" column
metadata, or columns that exist in all indices. The addition of the
"metaalert" index broke this because it is missing most of the columns that are
declared in alert indices. This PR corrects this by retrieving all column
metadata across indices so that a missing column in one index does not affect
the results.

The
http://localhost:8080/swagger-ui.html#!/search-controller/getColumnMetadataUsingPOST
changed slightly in that it now just returns a single map with all
columns/types included and no longer segments columns/types by index. This
interface more closely matches how this function is used in the alerts UI and
eliminates the need for the client side code to flatten the result. Column
metadata can still be retrieved for an index by just passing in a single index
to that endpoint. The case of columns existing in multiple indices with
different types is solved by just excluding them from the results.

This can be tested in full dev by executing a search (either with the
Alerts UI or Swagger) and including an ip address field in the facet list. The
results should now include correctly formatted IP addresses in the facetCounts
section of the results. A group query can be verified by including an IP
address in the list of group fields.

I adjusted the SearchControllerIntegrationTest to account for the new
interface and modified cases in the SearchIntegrationTest to catch these issues
in the future.

## Pull Request Checklist

In order to streamline the review of the contribution we ask you follow
these guidelines and ask you to double check the following:

```
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 METRON-1294

Alternatively

[GitHub] metron pull request #826: METRON-1291: Kafka produce REST endpoint does not ...

2017-11-02 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1291: Kafka produce REST endpoint does not work in a Kerberized 
cluster

## Contributor Comments
This PR adds Kerberos support to the 
http://node1:8082/swagger-ui.html#!/kafka-controller/produceUsingPOST endpoint. 
 This can be verified in full dev by enabling Kerberos and using that endpoint 
to produce a message.  The same message should be returned with a call to the 
http://node1:8082/swagger-ui.html#!/kafka-controller/getSampleUsingGET endpoint 
(using the same topic).

I also added a unit test for the KafkaConfig class.

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] 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 METRON-1291

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

https://github.com/apache/metron/pull/826.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 #826


commit 88665069bbadceb918741d385e81ffe450dca129
Author: merrimanr <merrim...@gmail.com>
Date:   2017-11-02T17:43:21Z

initial commit




---

[GitHub] metron issue #825: METRON-1290: Only first 10 alerts are update when a MetaA...

2017-11-02 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/825
  
Looks like there is a 
[bug](https://stackoverflow.com/questions/40984882/mockito-and-powermock-methodnotfoundexception-being-thrown)
 in the powermock version inherited by this module.  Latest commit fixes that 
(and hopefully helps others in the future that want to use powermock in Metron).


---

[GitHub] metron pull request #825: METRON-1290: Only first 10 alerts are update when ...

2017-11-02 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1290: Only first 10 alerts are update when a MetaAlert status is 
changed to inactive

## Contributor Comments
This PR fixes a small bug in the ElasticsearchMetaAlertDao that only 
updates the first 10 alerts in a metaalert when that metaalert status is 
changed to inactive.  This is due to the fact that the 
ElasticsearchMetaAlertDao needs to lookup up the child alerts and a search 
created with `elasticsearchDao.getClient().prepareSearch()` defaults to result 
set size to 10.  This can be reproduced in full dev:
- turn off the sensor stubs so that the number of alerts in ES is fixed
- take note of the number of alerts in the index
- create a metaalert with more than 10 alerts
- the number of alerts should decrease to: # of alerts added - 1 metaalert
- change the metaalert status to inactive 
- the number of alerts will now be less than before where the count should 
be the same

I also added an embarrassingly complex unit test to cover this situation.

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] 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 METRON-1290

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

https://github.com/apache/metron/pull/825.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 #825


commit 9159ee5e2ccba4a762b6f3a455130a1da79d4c70
Author: merrimanr <merrim...@gmail.com>
Date:   2017-11-02T13:25:47Z

initial commit




---

[GitHub] metron pull request #824: METRON-1289: Alert fields are lost when a MetaAler...

2017-11-01 Thread merrimanr

GitHub user merrimanr opened a pull request:

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

METRON-1289: Alert fields are lost when a MetaAlert is created

## Contributor Comments
This PR fixes a bug in the ElasticsearchMetaAlertDao that incorrectly 
updates the included alerts.  To reproduce this error, pick any alert and 
record it's guid.  Then create a metaalert with the 
http://node1:8082/swagger-ui.html#!/meta-alert-controller/createUsingPOST 
endpoint:
```
{
  "groups": [
"string"
  ],
  "guidToIndices": 
{"dcb3afed-1b68-d88a-7adb-f38183867920":"bro_index_2017.11.01.13"}
}
```
Now perform a lookup on the same alert with the 
http://node1:8082/swagger-ui.html#!/search-controller/getLatestUsingPOST 
endpoint:
```
{
  "guid": "dcb3afed-1b68-d88a-7adb-f38183867920",
  "sensorType": "bro"
}
```
The result only contains the "metaalert" field:
```
{
  "metaalerts": [
"87ce1d82-aa09-4a1a-8be9-b9e7b76859b9"
  ]
}
```
This PR corrects this error by updating the alert with the full object 
instead of a partial object.  After this PR, following the steps above should 
return the full object including the new "metaalert" field.

A `getAllLatest` function was added to make IndexDao match the pattern used 
for findOne.  An HBase implementation was not added because it seemed 
unnecessary but it could be if it makes sense.  Test cases were added to catch 
the error above and for the new `getAllLatest` function.  This has been 
verified in full dev.

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] 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 METRON-1289

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

https://github.com/apache/metron/pull/824.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 #824


commit c26a36f8d1a5d0a4cec9166f777013247b6ca199
Author: merrimanr <merrim...@gmail.com>
Date:   2017-11-01T21:09:42Z

initial commit




---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

2017-10-31 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
Yes HBase is running.  The first set of status tests are working (they 
would not if HBase were down).


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r147853082
  
--- Diff: 
metron-interface/metron-alerts/e2e/alerts-list/tree-view/tree-view.po.ts ---
@@ -159,4 +159,34 @@ export class TreeViewPage {
   return column.getText();
 });
   }
+
+  clickOnMergeAlerts(groupName: string) {
+return element(by.css('[data-name="' + groupName + '"] 
.fa-link')).click();
+  }
+
+  getConfirmationText() {
+browser.sleep(1000);
+let dialogElement = element(by.css('.metron-dialog .modal-header 
.close'));
+return waitForElementVisibility(dialogElement).then(() =>  
element(by.css('.metron-dialog .modal-body')).getText());
+  }
+
+  clickNoForConfirmation() {
+browser.sleep(1000);
+let dialogElement = element(by.css('.metron-dialog .modal-header 
.close'));
+let maskElement = element(by.css('.modal-backdrop.fade'));
+waitForElementVisibility(dialogElement).then(() => 
element(by.css('.metron-dialog')).element(by.buttonText('Cancel')).click())
+.then(() => waitForElementInVisibility(maskElement));
+  }
+
+  clickYesForConfirmation() {
+browser.sleep(1000);
--- End diff --

Is this sleep statement absolutely necessary?


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r147853051
  
--- Diff: 
metron-interface/metron-alerts/e2e/alerts-list/tree-view/tree-view.po.ts ---
@@ -159,4 +159,34 @@ export class TreeViewPage {
   return column.getText();
 });
   }
+
+  clickOnMergeAlerts(groupName: string) {
+return element(by.css('[data-name="' + groupName + '"] 
.fa-link')).click();
+  }
+
+  getConfirmationText() {
+browser.sleep(1000);
--- End diff --

Is this sleep statement absolutely necessary?


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r147853067
  
--- Diff: 
metron-interface/metron-alerts/e2e/alerts-list/tree-view/tree-view.po.ts ---
@@ -159,4 +159,34 @@ export class TreeViewPage {
   return column.getText();
 });
   }
+
+  clickOnMergeAlerts(groupName: string) {
+return element(by.css('[data-name="' + groupName + '"] 
.fa-link')).click();
+  }
+
+  getConfirmationText() {
+browser.sleep(1000);
+let dialogElement = element(by.css('.metron-dialog .modal-header 
.close'));
+return waitForElementVisibility(dialogElement).then(() =>  
element(by.css('.metron-dialog .modal-body')).getText());
+  }
+
+  clickNoForConfirmation() {
+browser.sleep(1000);
--- End diff --

Is this sleep statement absolutely necessary?


---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
I am consistently getting these e2e test failures:
```
1) meta-alerts workflow should have all the steps for meta alerts workflow
  - 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.

2) metron-alerts alert status should change alert statuses
  - Expected 'RESOLVE' to equal 'OPEN'.
  - Expected 'NEW' to equal 'OPEN'.
  - Expected 'RESOLVE' to equal 'DISMISS'.
  - Expected 'NEW' to equal 'DISMISS'.
  - Expected 'RESOLVE' to equal 'ESCALATE'.
  - Expected 'NEW' to equal 'ESCALATE'.
  - Expected 'NEW' to equal 'RESOLVE'.

3) metron-alerts alert status should add comments for table view
  - 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) metron-alerts alert status should add comments for tree view
  - Failed: unknown error: Element

[GitHub] metron issue #820: METRON-1287 Full Dev Fails When Installing EPEL Repositor...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/820
  
I was able to successfully build full dev with this change.  +1


---

[GitHub] metron pull request #821: METRON-1281: Remove hard-coded indices from the Al...

GitHub user merrimanr opened a pull request:

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

METRON-1281: Remove hard-coded indices from the Alerts UI

## Contributor Comments
This PR adjusts the Alerts UI and REST app to remove the need to hard-code 
the indices in the Alerts UI source code.  I felt like this feature could be 
kept simple by having the REST SearchService supply the appropriate indices by 
default because:

- the front end code has no knowledge of when the list of available indices 
changes and would have to make an extra call on every search
- the underlying ElasticsearchDao lives in a different module and does not 
have access to the SensorIndexingConfigService which is required to look up the 
available indices

This resulted in an additional "index.writer.name" property because the 
indices lookup requires this information.  The "error" index doesn't make sense 
in an alert search context so it is excluded. 
 The "metaalert" index is added by default in anticipation of 
https://github.com/apache/metron/pull/803.

A SearchServiceTest had not been created yet so it has been added with 
coverage for this PR.  An integration test for a query with default indices was 
also added.  While working on the integration tests I noticed an intermittent 
failure happening frequently so I fixed that in this PR to get the build to 
pass consistently (GlobalConfigControllerIntegrationTest.java).  I can revert 
that if desired but my thinking was it's probably not worth the overhead of a 
separate PR. 

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] Have you included steps or a guide to how the change may be verified 
and tested manually?
- [x] 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 
remove_hardcoded_indices

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

https://github.com/apache/metron/pull/821.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 #821


commit 0570ba18d773688bfc2d71e2f081155583c21a2e
Author: cstella <ceste...@gmail.com>
Date:   2017-10-09T19:03:32Z

Abstracting zookeeper substantially.

commit e59a82266

[GitHub] metron issue #796: METRON-1224: Add time range selection to search control

2017-10-25 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/796
  
I think the test coverage looks pretty good.  I'm +1 conditional on the e2e 
tests passing.


---

[GitHub] metron issue #797: METRON-1243: Add a REST endpoint which allows us to get a...

2017-10-25 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/797
  
Just tested this and worked as expected.  The only question I have is 
whether the "error" index should be included since it is used to store errors 
and not alerts.  I can't think of a case where you would include "error" in a 
search along with other indices.  I'm looking at this from the perspective of 
the Alerts UI so I don't necessarily think it needs to be done in this PR but 
it will need to be filtered out at some point.


---

[GitHub] metron issue #796: METRON-1224: Add time range selection to search control

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/796
  
This issue is still not fixed:

- The first is related to time-range selections that include 'Now' as part 
of the range (Last 7 days, Last 5 minutes, Today so far, etc). This should be a 
sliding window so I would expect the search query to be different every time 
the results are refreshed. - Fixed

I submitted a PR against this PR that should address it (also has the 
latest version of master merged in).  Let me know if you think this is the 
right way to fix it.  Seems kind of strange to import a class (QueryBuilder) in 
alerts-list into a service but I'll let you decide if that is ideal.

Saved search is working now and everything else seems to function 
correctly.  I did notice a regression where if I rename a column, type a query 
with the friendly name in the query box (ie. sourceType:snort) results are no 
longer returned.  Not sure if that was introduced in this PR or not because I 
don't think we are testing for it.

I am still not a fan of how the Time Range selection works.  The "now" 
designation is essentially meaningless because you can't save a range with 
"now" in either FROM or TO.  I still agree with all my previous complaints 
about this too.  If the community feels it is working correctly then I would 
not hold up this PR over it.

The e2e tests are still insufficient and failing.  I gave some 
recommendations 
[here](https://github.com/apache/metron/pull/796#issuecomment-337965321) for 
your reference.


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr reopened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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- where  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?
- [ ] 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)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813


commit c291a9c66a65027ce7cfa5cd4ce9768dff30b644
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T14:03:04Z

fixed assertEventually to wait on broTest config propagation

commit c63b9d98cdf2317811d05640249cfe13ecd7b40d
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T22:41:12Z

fixed assertEventually in SensorParserConfigControllerIntegrationTest




---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr reopened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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- where  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?
- [ ] 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)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813


commit c291a9c66a65027ce7cfa5cd4ce9768dff30b644
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T14:03:04Z

fixed assertEventually to wait on broTest config propagation

commit c63b9d98cdf2317811d05640249cfe13ecd7b40d
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T22:41:12Z

fixed assertEventually in SensorParserConfigControllerIntegrationTest




---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr reopened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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- where  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?
- [ ] 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)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813






---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr reopened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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- where  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?
- [ ] 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)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813


commit c291a9c66a65027ce7cfa5cd4ce9768dff30b644
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T14:03:04Z

fixed assertEventually to wait on broTest config propagation

commit c63b9d98cdf2317811d05640249cfe13ecd7b40d
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T22:41:12Z

fixed assertEventually in SensorParserConfigControllerIntegrationTest




---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

Github user merrimanr closed the pull request at:

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


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr reopened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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- where  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?
- [ ] 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)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813


commit c291a9c66a65027ce7cfa5cd4ce9768dff30b644
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T14:03:04Z

fixed assertEventually to wait on broTest config propagation

commit c63b9d98cdf2317811d05640249cfe13ecd7b40d
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T22:41:12Z

fixed assertEventually in SensorParserConfigControllerIntegrationTest




---

[GitHub] metron issue #813: METRON-1274: Master has failure in StormControllerIntegra...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/813
  
Fixed the other failure at 
`SensorParserConfigControllerIntegrationTest.test:294`.  


---

[GitHub] metron issue #811: METRON-1272: Hide child alerts from searches and grouping...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/811
  
@nickwallen suppose you have a metaalert that contains 2 alerts.  Then 
suppose each alert has a different value for the host field.  If you grouped on 
host, which group would you expect the metaalert to appear in?


---

[GitHub] metron issue #811: METRON-1272: Hide child alerts from searches and grouping...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/811
  
I believe excluding metaalerts from the group by view is the desired 
behavior. 


---

[GitHub] metron pull request #813: METRON-1274: Master has failure in StormController...

GitHub user merrimanr opened a pull request:

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

METRON-1274: Master has failure in StormControllerIntegrationTest

## Contributor Comments
This PR fixes an intermittently failing integration test. I believe the 
root cause is the test is waiting for the wrong config to propagate (that was 
already written earlier) so it never waits.

I've run this several times and am not seeing the failure anymore but will 
continue to run more tests.

## 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:
- [ ] 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).
 
- [ ] Does your PR title start with METRON- where  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](http://www.apache.org/legal/resolved.html#category-a)? 
- [ ] 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](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 METRON-1274

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

https://github.com/apache/metron/pull/813.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 #813


commit c291a9c66a65027ce7cfa5cd4ce9768dff30b644
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-23T14:03:04Z

fixed assertEventually to wait on broTest config propagation




---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r146267005
  
--- Diff: metron-interface/metron-alerts/src/app/service/update.service.ts 
---
@@ -38,22 +41,27 @@ export class UpdateService {
 
   constructor(private http: Http) { }
 
-  public patch(patchRequest: PatchRequest): Observable<{}> {
+  public patch(patchRequest: PatchRequest, fireChangeListner = true): 
Observable<{}> {
 let url = '/api/v1/update/patch';
 return this.http.patch(url, patchRequest, new RequestOptions({headers: 
new Headers(this.defaultHeaders)}))
 .catch(HttpUtil.handleError)
 .map(result => {
-  this.alertChangedSource.next(patchRequest);
+  if (fireChangeListner) {
+this.alertChangedSource.next(patchRequest);
+  }
   return result;
 });
   }
 
-  public updateAlertState(alerts: Alert[], state: string): Observable<{}> {
+  public updateAlertState(alerts: Alert[], state: string, 
fireChangeListner = true): Observable<{}> {
--- End diff --

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?


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r146265870
  
--- Diff: 
metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts
 ---
@@ -0,0 +1,101 @@
+/**
+ * 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 { Component, OnInit } from '@angular/core';
+import {Router} from '@angular/router';
+
+import {MetaAlertService} from '../../service/meta-alert.service';
+import {UpdateService} from '../../service/update.service';
+import {SearchRequest} from '../../model/search-request';
+import {SearchService} from '../../service/search.service';
+import {SearchResponse} from '../../model/search-response';
+import {SortField} from '../../model/sort-field';
+import {META_ALERTS_INDEX} from '../../utils/constants';
+import {AlertSource} from '../../model/alert-source';
+import {PatchRequest} from '../../model/patch-request';
+import {Patch} from '../../model/patch';
+
+@Component({
+  selector: 'app-meta-alerts',
+  templateUrl: './meta-alerts.component.html',
+  styleUrls: ['./meta-alerts.component.scss']
+})
+export class MetaAlertsComponent implements OnInit {
+
+  selectedMetaAlert = '';
+  searchResponse: SearchResponse = new SearchResponse();
+
+  constructor(private router: Router,
+  private metaAlertService: MetaAlertService,
+  private updateService: UpdateService,
+  private searchService: SearchService) {
+  }
+
+  goBack() {
+this.router.navigateByUrl('/alerts-list');
+return false;
+  }
+
+  ngOnInit() {
+let searchRequest = new SearchRequest();
+searchRequest.query = '*';
+searchRequest.from = 0;
+searchRequest.size = 999;
+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'];
+
+this.searchService.search(searchRequest).subscribe(resp => 
this.searchResponse = resp);
+  }
+
+  doAddAlertToMetaAlert(alertSources: AlertSource[]) {
+let patchRequest = new PatchRequest();
+patchRequest.guid = this.selectedMetaAlert;
+patchRequest.sensorType = 'metaalert';
+patchRequest.index = META_ALERTS_INDEX;
+patchRequest.patch = [new Patch('replace', 'alert', alertSources)];
+
+this.updateService.patch(patchRequest).subscribe(rep => {
+  console.log('Meta alert saved');
--- End diff --

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


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r146265654
  
--- Diff: 
metron-interface/metron-alerts/e2e/alerts-list/tree-view/tree-view.e2e-spec.ts 
---
@@ -175,7 +175,7 @@ describe('metron-alerts tree view', function () {
   });
 
 
-  it('should have sort working for group details for multiple sub groups', 
() => {
+  xit('should have sort working for group details for multiple sub 
groups', () => {
--- End diff --

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


---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

2017-10-21 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
I just submitted a 
[PR](https://github.com/iraghumitra/incubator-metron/pull/3) 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.


---

[GitHub] metron pull request #795: METRON-1241: Enable the REST API to use a cache fo...

2017-10-20 Thread merrimanr

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

https://github.com/apache/metron/pull/795#discussion_r145992573
  
--- Diff: 
metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/StormControllerIntegrationTest.java
 ---
@@ -179,6 +181,8 @@ public void test() throws Exception {
 
sensorParserConfig.setParserClassName("org.apache.metron.parsers.bro.BasicBroParser");
 sensorParserConfig.setSensorTopic("broTest");
 sensorParserConfigService.save(sensorParserConfig);
+//we must wait for the config to find its way into the config.
+Thread.sleep(500);
--- End diff --

Would it make sense to use assertEventually here instead of Thread.sleep?


---

[GitHub] metron issue #795: METRON-1241: Enable the REST API to use a cache for the z...

2017-10-20 Thread merrimanr

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/795
  
I tested this thoroughly and everything works as expected.  I made one 
small comment that I feel is optional.  This is an awesome PR.  +1


---

[GitHub] metron pull request #808: METRON-1267: Alerts UI returns a 404 when refreshi...

GitHub user merrimanr opened a pull request:

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

METRON-1267: Alerts UI returns a 404 when refreshing the alerts-list page

## Contributor Comments
This PR fixes a small bug that causes a 404 when refreshing the front 
Alerts UI list page.  To verify spin up full dev and make sure you can refresh 
the Alerts UI.

## 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- where  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:
- [x] Have you included steps to reproduce the behavior or problem that is 
being changed or addressed?
- [x] 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?
- [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 METRON-1267

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

https://github.com/apache/metron/pull/808.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 #808


commit 1d9b739b6433bda491912c57f5f1f542c405b551
Author: merrimanr <merrim...@gmail.com>
Date:   2017-10-19T21:18:32Z

fixed bug in alerts-server.js




---

[GitHub] metron issue #803: Metron-1252: Build ui for grouping alerts into meta alert...

Github user merrimanr commented on the issue:

https://github.com/apache/metron/pull/803
  
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 
(https://github.com/apache/metron/pull/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.


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145816832
  
--- Diff: 
metron-interface/metron-alerts/src/app/alerts/alerts-list/table-view/table-view.component.scss
 ---
@@ -24,4 +24,12 @@
 .configure-table-icon {
   font-size: 16px;
   cursor: pointer;
+}
+
+.fa-chain-broken {
+  color: $piction-blue;
+}
+
+.dropdown-cell {
+  padding-left: 0.6rem;
 }
--- End diff --

need newline


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145816633
  
--- Diff: 
metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.html
 ---
@@ -0,0 +1,48 @@
+
+
+  
+
+  
+Add to Alert
+  
+  
+
+
+   SELECT OPEN ALERT 
+
+  
+
+  
+
+  
+
+  
+  
+ {{ 
alert.source['threat:triage:score'] }} 
+
+   {{(alert.source.name && 
alert.source.name.length > 0) ? alert.source.name : alert.source.guid | 
centerEllipses:20 }}({{ alert.source.count }}) 
+   {{ 
(alert.source.alert_status && alert.source.alert_status.length > 0) ? 
alert.source.alert_status : 'NEW' }} 
+   {{ 
alert.source._timestamp | timeLapse }} 
+
+  
+  
+
+  
+ADD
+CANCEL
+  
+
+  
+
--- End diff --

need newline


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145816548
  
--- Diff: metron-interface/metron-alerts/src/styles.scss ---
@@ -259,4 +259,61 @@ hr {
   padding: 0;
 }
 
-
+/** Custom Radio box **/
+$background_color_1: #eee;
+$background_color_2: #ccc;
+$background_color_3: #2196F3;
+.radio-container {
+  display: block;
+  position: relative;
+  padding-left: 35px;
+  margin-bottom: 12px;
+  cursor: pointer;
+  font-size: 22px;
+  -webkit-user-select: none;
+  -moz-user-select: none;
+  -ms-user-select: none;
+  user-select: none;
+  input {
+position: absolute;
+opacity: 0;
+&:checked {
+  &~.checkmark {
+background-color: $eastern-blue-2;
+&:after {
+  display: block;
+}
+  }
+}
+  }
+  &:hover {
+input {
+  &~.checkmark {
+background-color: $eastern-blue-2;
+  }
+}
+  }
+  .checkmark {
+position: absolute;
+top: 0;
+left: 0;
+height: 12px;
+width: 12px;
+background-color: $mine-shaft-2;
+border: 1px solid $tundora;
+border-radius: 50%;
+
+&:after {
+  top: 2px;
+  left: 2px;
+  width: 6px;
+  height: 6px;
+  border-radius: 50%;
+  background: $white;
+  content: "";
+  position: absolute;
+  display: none;
+}
+  }
+}
+/**/
--- End diff --

need newline


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145816388
  
--- Diff: metron-interface/metron-alerts/src/app/service/update.service.ts 
---
@@ -38,22 +41,27 @@ export class UpdateService {
 
   constructor(private http: Http) { }
 
-  public patch(patchRequest: PatchRequest): Observable<{}> {
+  public patch(patchRequest: PatchRequest, fireChangeListner = true): 
Observable<{}> {
--- End diff --

Could we fix the typo in Listner?


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145816267
  
--- Diff: metron-interface/metron-alerts/src/app/service/update.service.ts 
---
@@ -38,22 +41,27 @@ export class UpdateService {
 
   constructor(private http: Http) { }
 
-  public patch(patchRequest: PatchRequest): Observable<{}> {
+  public patch(patchRequest: PatchRequest, fireChangeListner = true): 
Observable<{}> {
 let url = '/api/v1/update/patch';
 return this.http.patch(url, patchRequest, new RequestOptions({headers: 
new Headers(this.defaultHeaders)}))
 .catch(HttpUtil.handleError)
 .map(result => {
-  this.alertChangedSource.next(patchRequest);
+  if (fireChangeListner) {
+this.alertChangedSource.next(patchRequest);
+  }
   return result;
 });
   }
 
-  public updateAlertState(alerts: Alert[], state: string): Observable<{}> {
+  public updateAlertState(alerts: Alert[], state: string, 
fireChangeListner = true): Observable<{}> {
--- End diff --

is fireChangeListner used in this function?


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145814896
  
--- Diff: 
metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts
 ---
@@ -0,0 +1,101 @@
+/**
+ * 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 { Component, OnInit } from '@angular/core';
+import {Router} from '@angular/router';
+
+import {MetaAlertService} from '../../service/meta-alert.service';
+import {UpdateService} from '../../service/update.service';
+import {SearchRequest} from '../../model/search-request';
+import {SearchService} from '../../service/search.service';
+import {SearchResponse} from '../../model/search-response';
+import {SortField} from '../../model/sort-field';
+import {META_ALERTS_INDEX} from '../../utils/constants';
+import {AlertSource} from '../../model/alert-source';
+import {PatchRequest} from '../../model/patch-request';
+import {Patch} from '../../model/patch';
+
+@Component({
+  selector: 'app-meta-alerts',
+  templateUrl: './meta-alerts.component.html',
+  styleUrls: ['./meta-alerts.component.scss']
+})
+export class MetaAlertsComponent implements OnInit {
+
+  selectedMetaAlert = '';
+  searchResponse: SearchResponse = new SearchResponse();
+
+  constructor(private router: Router,
+  private metaAlertService: MetaAlertService,
+  private updateService: UpdateService,
+  private searchService: SearchService) {
+  }
+
+  goBack() {
+this.router.navigateByUrl('/alerts-list');
+return false;
+  }
+
+  ngOnInit() {
+let searchRequest = new SearchRequest();
+searchRequest.query = '*';
+searchRequest.from = 0;
+searchRequest.size = 999;
+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'];
+
+this.searchService.search(searchRequest).subscribe(resp => 
this.searchResponse = resp);
+  }
+
+  doAddAlertToMetaAlert(alertSources: AlertSource[]) {
+let patchRequest = new PatchRequest();
+patchRequest.guid = this.selectedMetaAlert;
+patchRequest.sensorType = 'metaalert';
+patchRequest.index = META_ALERTS_INDEX;
+patchRequest.patch = [new Patch('replace', 'alert', alertSources)];
+
+this.updateService.patch(patchRequest).subscribe(rep => {
+  console.log('Meta alert saved');
+  this.goBack();
+});
+  }
+
+  addAlertToMetaAlert() {
+let searchRequest = new SearchRequest();
+searchRequest.query = 'guid:"' + this.selectedMetaAlert + '"';
+searchRequest.from = 0;
+searchRequest.size = 1;
+searchRequest.facetFields = [];
+searchRequest.indices =  [META_ALERTS_INDEX];
+searchRequest.sort = [];
+searchRequest.fields = [];
+
+this.searchService.search(searchRequest).subscribe((searchResponse: 
SearchResponse) => {
--- End diff --

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.


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...

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

https://github.com/apache/metron/pull/803#discussion_r145814193
  
--- Diff: 
metron-interface/metron-alerts/src/app/alerts/meta-alerts/meta-alerts.component.ts
 ---
@@ -0,0 +1,101 @@
+/**
+ * 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 { Component, OnInit } from '@angular/core';
+import {Router} from '@angular/router';
+
+import {MetaAlertService} from '../../service/meta-alert.service';
+import {UpdateService} from '../../service/update.service';
+import {SearchRequest} from '../../model/search-request';
+import {SearchService} from '../../service/search.service';
+import {SearchResponse} from '../../model/search-response';
+import {SortField} from '../../model/sort-field';
+import {META_ALERTS_INDEX} from '../../utils/constants';
+import {AlertSource} from '../../model/alert-source';
+import {PatchRequest} from '../../model/patch-request';
+import {Patch} from '../../model/patch';
+
+@Component({
+  selector: 'app-meta-alerts',
+  templateUrl: './meta-alerts.component.html',
+  styleUrls: ['./meta-alerts.component.scss']
+})
+export class MetaAlertsComponent implements OnInit {
+
+  selectedMetaAlert = '';
+  searchResponse: SearchResponse = new SearchResponse();
+
+  constructor(private router: Router,
+  private metaAlertService: MetaAlertService,
+  private updateService: UpdateService,
+  private searchService: SearchService) {
+  }
+
+  goBack() {
+this.router.navigateByUrl('/alerts-list');
+return false;
+  }
+
+  ngOnInit() {
+let searchRequest = new SearchRequest();
+searchRequest.query = '*';
+searchRequest.from = 0;
+searchRequest.size = 999;
+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'];
+
+this.searchService.search(searchRequest).subscribe(resp => 
this.searchResponse = resp);
+  }
+
+  doAddAlertToMetaAlert(alertSources: AlertSource[]) {
+let patchRequest = new PatchRequest();
+patchRequest.guid = this.selectedMetaAlert;
+patchRequest.sensorType = 'metaalert';
+patchRequest.index = META_ALERTS_INDEX;
+patchRequest.patch = [new Patch('replace', 'alert', alertSources)];
+
+this.updateService.patch(patchRequest).subscribe(rep => {
+  console.log('Meta alert saved');
+  this.goBack();
+});
+  }
+
+  addAlertToMetaAlert() {
+let searchRequest = new SearchRequest();
+searchRequest.query = 'guid:"' + this.selectedMetaAlert + '"';
+searchRequest.from = 0;
+searchRequest.size = 1;
+searchRequest.facetFields = [];
+searchRequest.indices =  [META_ALERTS_INDEX];
+searchRequest.sort = [];
+searchRequest.fields = [];
+
+this.searchService.search(searchRequest).subscribe((searchResponse: 
SearchResponse) => {
+  if (searchResponse.results.length === 1) {
+searchResponse.results[0].source.alert = 
[...searchResponse.results[0].source.alert,
--- End diff --

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


---

[GitHub] metron pull request #803: Metron-1252: Build ui for grouping alerts into met...