You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by justinleet <gi...@git.apache.org> on 2018/03/21 20:48:51 UTC
[GitHub] metron pull request #970: METRON-1421: Create a SolrMetaAlertDao
GitHub user justinleet opened a pull request:
https://github.com/apache/metron/pull/970
METRON-1421: Create a SolrMetaAlertDao
## Contributor Comments
Initial pass of adding meta alert functionality to the Solr implementation. It's extremely similar in style to the ES implementation (metaalerts field on an individual alert that list the metaalerts it belongs to, alerts field on metaalerts that has a denormalized copy of the individual alerts).
I need to do whatever running up we're set up for on this branch at this point. I got the build running and all the tests working. Sidenote, if anyone can tell me what exactly should be run up at this point, I would appreciate it.
Main things that are in this PR:
- `SolrMetaAlertDao` and integration tests.
- Schema changes as appropriate to support meta alerts.
- Lot of refactoring to have the ES and Solr implementations share a lot of metaalert code, seen in `AbstractMetaAlertDao`
- Lot of refactoring to have the ES and Solr integration tests share a lot of common code, seen in `MetaAlertIntegrationTest`
- There are some minor behavior differences, e.g. a metaalert with no alerts might return either no field or an empty list depending on Solr vs. ES. These (should) be limited to things we should be handling robustly elsewhere anyway.
- Some minor bugfixes (e.g. some leftover hardcoding, added some retry based result lookups, and so on).
- The index config needs to be available to Solr, so that we can determine what collection a given sensor belongs to.
- The `SolrComponent` can now have collections added while running, and tear them down between tests.
Things to look out for during review
- The use of a lot of the constants can be tricky to track, care should be given to make sure the appropriate choices are made (e.g. for some of the typing and such).
- The correctness of Solr queries. They're finicky, so make sure they make sense.
- Any stray` System.out`s.
- Stuff tends to be deferred to the implementations in certain cases. If there are cleaner ways to handle some of that, let me know.
- Any refactorings that could happen to merge anything else between impls.
Miscellaneous Notes
- Child documents in Solr do not get a field name. This implementation chooses to treat any child as a the child alert, and pass it back up the stack as the alert field to match ES. Similarly, going down the stack, Documents will have their alert field translated to the Solr field.
- Deleting all children of a parent in Solr seems to not work. I've seen references that this operation is undefined online, but hadn't seen it in the docs (although adding/removing in general is. It's weird). Instead, if all children would be gone, they are manually deleted before the parent update is set. If anyone knows better, I'd be thrilled.
- There's a bit of minor cleanup through some of the files that got touched. The test schema files in particular got autoformatted, because it was a pain to deal with sometimes when things were at different indentations and such.
- We're still apparently using ':' for all of our field names (e.g. source:type). Is there a particular reason other than inertia? Seems like we could be using '.' and my life would be easier here.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [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 && dev-utilities/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)?
- [ ] 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/justinleet/metron METRON-1421
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/970.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 #970
----
commit 17748f59c8eb2c5fa6cd77571588b4f2d7527427
Author: justinjleet <ju...@...>
Date: 2018-02-06T12:50:55Z
refactoring tests wip
commit be1883480eb5eb4b38ced1f7619d62a70fe40fbc
Author: justinjleet <ju...@...>
Date: 2018-02-09T14:27:49Z
WIP meta alerts stuff
commit 6c98c49ed14a3d8f2a40aafc5a2033cd06d22b82
Author: justinjleet <ju...@...>
Date: 2018-02-21T14:12:02Z
fix get all meta alerts
commit 988d9a3eade55e12adbf783473d1e563ef90da5d
Author: justinjleet <ju...@...>
Date: 2018-02-27T20:13:31Z
Fix some things
commit 9f9ad4efc13bbb6a65de2d9a3492371376fc6413
Author: justinjleet <ju...@...>
Date: 2018-02-28T16:25:05Z
Hack hardcoded fix to some update problems
commit 549a783bd17d78ccae5b322bac9b98efe64f612e
Author: justinjleet <ju...@...>
Date: 2018-02-28T19:36:26Z
More correct, but not actually correct lookup of sensor output collection
commit d442d6707cd8c224049376e6aa4b1bc8529f7d73
Author: justinjleet <ju...@...>
Date: 2018-03-01T16:15:24Z
WIP on getting search and remove working
commit 94215284d3a706213e79e409f7beb5fdeeee65e9
Author: justinjleet <ju...@...>
Date: 2018-03-05T14:28:15Z
more wip stuff, query parser issues
commit 86f9892cc7b2cfc6578451fc7f743e23d9389133
Author: justinjleet <ju...@...>
Date: 2018-03-05T15:42:28Z
Merging in from parent feature branch
commit a17132326d0d20f1944a3b422eda5fd42a3e063b
Author: justinjleet <ju...@...>
Date: 2018-03-05T18:42:00Z
fixing incorrect cast
commit f16362506d13454aac4f173805aed1ddc8c7e611
Author: justinjleet <ju...@...>
Date: 2018-03-13T12:37:28Z
fix plus some cleanup
commit 683643a9b7d4b967ffbb32014c6474c86663ebfe
Author: justinjleet <ju...@...>
Date: 2018-03-18T23:57:58Z
Parent Child Query fixes
commit dded631d6ff956a3dc33c248a0fc7b1557ee74eb
Author: justinjleet <ju...@...>
Date: 2018-03-19T15:19:47Z
Fixing child clause
commit 3361e7c0007801849ea9e1ab5a5f690425d451e2
Author: justinjleet <ju...@...>
Date: 2018-03-20T13:38:10Z
Fixes to nested alerts
commit 24fe1aa1ce7115fdaf69eff133cb4b699519730b
Author: justinjleet <ju...@...>
Date: 2018-03-20T16:00:00Z
Grouping working
commit ee848ad63fa10a8864c2eb963fc5286d97acc200
Author: justinjleet <ju...@...>
Date: 2018-03-20T16:26:31Z
cleanup and fixing some of the ES ripples
commit 2781ec7994ce6798cf0608aa30deb50f07d86e4b
Author: justinjleet <ju...@...>
Date: 2018-03-20T18:02:23Z
Fixing more ES
commit 305ae38724b1fd18fe2e1a342ede4eb2ad1755e2
Author: justinjleet <ju...@...>
Date: 2018-03-20T20:12:33Z
Fixing some index name shenanigans now that we aren't doing wildcard indices
commit d3610ddff9c88755335aa484b9637c8b06068cf0
Author: justinjleet <ju...@...>
Date: 2018-03-20T20:41:36Z
WIP indexing test fix
commit 17c7b93fc5e5150bfbe3c8f95571ba34062b3c97
Author: justinjleet <ju...@...>
Date: 2018-03-20T20:57:16Z
Merge in feature branch and fix integration test
commit cd888977ca2535b1677ef7302f859ae2abbd5ca2
Author: justinjleet <ju...@...>
Date: 2018-03-20T20:58:46Z
deleting old comment
commit 8f607adfcde58ee36079969141995329e3d50519
Author: justinjleet <ju...@...>
Date: 2018-03-20T21:06:35Z
Bunch of cleanup
commit f0fdd71090b298fea7c661ca97fbf916ea979b2f
Author: justinjleet <ju...@...>
Date: 2018-03-21T01:45:30Z
fixing ES remove to match nonfinite Double values
commit 063edf5dc51811616fa8857571eec9fb249030ec
Author: justinjleet <ju...@...>
Date: 2018-03-21T02:43:55Z
hacky workaround to the delete remaining docs problem
commit 5b7bec42e02f03f40afd4c6e01fc00fee05970d8
Author: justinjleet <ju...@...>
Date: 2018-03-21T02:54:17Z
Fixing create test to use proper retries
commit bdb16ea70838f75f892b467c8bd0484b3c0df9a2
Author: justinjleet <ju...@...>
Date: 2018-03-21T12:17:28Z
workaround for classpath issues, along with patch function fix for solr
commit 82dc8099a8338e4f4832ac46fa94c724ad3299fb
Author: justinjleet <ju...@...>
Date: 2018-03-21T13:17:54Z
Ensuring SolrComponent cleans up after itself.
commit 41811e05e7d38ddf3b94cfe210a8b8fe0a0e3c02
Author: justinjleet <ju...@...>
Date: 2018-03-21T14:00:47Z
Added missing annotations
commit e92e9e54705ad49de2a3f0a49a38e07ff0ff641e
Author: justinjleet <ju...@...>
Date: 2018-03-21T16:22:26Z
Lot of cleanup and guava fix for json patch
commit 896d26f2353c8602fddf7a5d1c42443f3318838a
Author: justinjleet <ju...@...>
Date: 2018-03-21T16:46:39Z
Merge branch 'feature/METRON-1416-upgrade-solr' into METRON-1421
----
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll add a couple tests for those and see if I can get them reproduced. Since some of that is in the AbstractLucene dao I'm wondering if it also happens against master ES. When I take a look I'm also going to see if it's relevant to master, and see what approach we want to take from there.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
I've been looking into the Travis failure that occurred. I was able to reproduce it, intermittently, locally. Best speculation is that it's a Lucene bug, given that it occurs seemingly randomly (~ every 20 times or so, but I've seen it go 50+).
After playing around a bit, opening up the parentFilter on the child doc transformer seems to take care of the problem by avoiding the rare situation where it treats the document inconsistently and throws and error.
The code used to test this was
```
@Test
public void shouldSearchByNestedAlertAlot() throws Exception {
for (int i = 0; i < 500; ++i) {
System.out.println("\nRound : " + i);
if (i != 0) {
setup();
}
try {
shouldSearchByNestedAlert();
} catch (Exception e) {
throw new IllegalStateException(e);
}
reset();
}
}
```
Which just basically cleaned things up and ran the test a whole lot. Setup/reset are nontrivial in terms of time, so I let this run a couple times with the change. Over 1000 rounds completed successfully without error.
If anyone wants to see more testing or has ideas on how to improve it, I'm definitely open to suggestions.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
I agree with your plan @justinleet . I am on-board. Thanks for laying that out.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@ottobackwards Correct me if I'm misunderstanding your view, @nickwallen, but the discussion is around what's the required level of iteration / refactoring where this meets expectations for testing and maintainability before this can go into the feature branch at all. I'm fine iterating afterwards (and that may include things like overall DAO restructuring an other things, depending on what ends up happening here). What we're trying to avoid is a situation where we immediately want to do large scale refactoring immediately (whether as part of the feature branch itself or master). This is (potentially) one of the final (and largest + most complicated) components of the feature branch, so I think that's where the concern from both myself and Nick is.
Once this is landed (or before or whenever), we can throw whatever PRs at the feature branch.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
I'm going to run the Travis tests a few in my own repo in addition to this, to make sure we aren't hitting the issue intermittently.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
The workaround for the intermittent failure described above was changed. The workaround didn't produce complete results for children. Instead, I've moved to a second query that pulls in the complete metaalert (for any present) and merges the results. The pro is that this entirely avoids the issue by making the child doc transformer only act on parent metaalerts, but at the cost of another network hop and an iteration through results.
The test has been updated to pick up the original failure.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@ottobackwards You're absolutely not confusing things. It's important to have that perspective, and I am 100% appreciative of you hopping in and offering up your opinion. I think you're right that we probably need to reach a solid compromise position rather than boiling the ocean.
@nickwallen What if, for this PR, we do the following:
1. Mostly keep the interface refactoring from the branch.
2. Instead of pushing everything directly into SolrUpdateMetaAlertDao and ElasticsearchUpdateMetaAlertDao, we push to AbstractLuceneUpdateMetaAlertDao minimally as needed to prevent duping (which is admittedly substantial)? Everything else pushes down to the specific DAO which extends the abstract DAOs. As in the test branch, the SolrMetaAlertDao just uses and passes along the appropriate impls as needed.
3. Repeat (2) for the other DAOs implementations
4. I take another pass at tests to make sure both the common and specific impls are properly tested.
At that point, it should somewhat clean up the type hierarchy which will enable us to make things more pluggable in the future (including potentially working to eliminate the aggregate marker interfaces). At the very least, I think it makes it more straightforward, although it's not perfect. This should improve maintainability and testing. The testing pass should make sure we're at least better positioned to be confident in future refactorings.
I 100% admit this doesn't completely address the composition vs. extension, but it at least makes progress relative towards that goal relative to where we were. It has the benefit of being a doable fairly low-risk refactoring that enables the PR to make it into the branch.
Hopefully this has the elements of a good compromise: Nobody is overjoyed, but nobody feels cheated. In addition, we at least have set ourselves up to make it easier for everyone to be happier in the future.
---
[GitHub] metron pull request #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on a diff in the pull request:
https://github.com/apache/metron/pull/970#discussion_r181186669
--- Diff: metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrSearchDao.java ---
@@ -238,19 +204,19 @@ protected SearchResponse buildSearchResponse(
return searchResponse;
}
- protected SearchResult getSearchResult(SolrDocument solrDocument, Optional<List<String>> fields) {
- SearchResult searchResult = new SearchResult();
- searchResult.setId((String) solrDocument.getFieldValue(Constants.GUID));
- Map<String, Object> source;
- if (fields.isPresent()) {
- source = new HashMap<>();
- fields.get().forEach(field -> source.put(field, solrDocument.getFieldValue(field)));
- } else {
- source = solrDocument.getFieldValueMap();
- }
- searchResult.setSource(source);
- return searchResult;
- }
+// protected SearchResult getSearchResult(SolrDocument solrDocument, Optional<List<String>> fields) {
--- End diff --
My bad. The merge from the feature branch was messy and I didn't clean up after myself.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Sidenote, after the remove alert section, make sure to add it back if you want to reuse the same metaalert for next section. I left it out, since you'll need your own guids for everything.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
This is merged into the feature branch. Closing PR manually, accordingly.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Just to summarize my position, my suggested refactorings are just one of many ways to improve the testability of all this. We don't have to take this approach.
For this PR, I will be happy if we think we have sufficient unit test coverage on the functionality here. If @justinleet thinks we are good on unit tests now, I will take another look at the unit tests today.
> @justinleet: What we're trying to avoid is a situation where we immediately want to do large scale refactoring immediately (whether as part of the feature branch itself or master).
To be honest, ultimately, I would like to refactor this (iteratively) to be simpler and more testable, but it doesn't have to happen on this PR or even on this feature branch or even in the near-term. I don't know when I'd be able to get to that.
@justinleet What do you think is a good path forward? I want to keep an open mind and make sure we're not doing more than we need to here.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet @nickwallen , sorry I didn't mean to confuse things. It seems to me that @nickwallen is willing to do some work on this per his 'vision' and that might be easier if this landed in a known state rather than thrashing the pr. But I don't have skin in this review so to speak. Just trying to help.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet @nickwallen I agree with the proposed plan for this PR's refactorings. That being said, I have some comments after having more or less caught up with the thread.
Generally speaking, it's not uncommon to provide an interface that encompasses all the desired methods for your data access layer. For instance:
- https://martinfowler.com/eaaCatalog/tableDataGateway.html
- https://martinfowler.com/eaaCatalog/dataMapper.html
- https://www.tutorialspoint.com/design_pattern/data_access_object_pattern.htm
I initially blinked at having the broad assortment of individual (CRUD) interfaces along with the aggregate ones, but I think that's fine. Not strictly necessary, but at least the intention is clear.
I think some of the trouble I/we're seeing with these interfaces and the inheritance is that it seems like there's some code smell around the abstractions we're using wrt the meta alerts and index. "Index" and "meta alert" don't seem to be on the same plane of abstraction. Index is a data store whereas meta alert is a domain model object. I think it's probably time (as a follow on DISCUSS thread) to map out how we want our access patterns restructured to better accommodate the request for composition that @nickwallen pointed out along with addressing the model and DAO abstraction mismatch.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I should probably hit you up too before we confirm that's what the plan is, since you've run stuff up a couple times and been pretty involved in the Solr work. Does the above plan for reaching an end state to the PR work for you?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> @justinleet Solve the code reuse problem for Solr and ES.
I am also noticing another positive side effect here. Getting rid of these interface hierarchies (like having the `MetaAlertService` use an `MetaAlertUpdateDao`) is that the service layer becomes where code lives that can be reused across ES and Solr. And that makes a ton of logical sense to me.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
Just finished testing this in full dev. Everything basically looks good. I'll take another spin through the source code as well. This is a good test script @justinleet.
Here are my findings:
> Find two alerts
I get 2 alerts, however the index shows as null
```
{
"facetCounts": null,
"results": [
{
"id": "10ececfc-8c1a-4039-9a4e-2027caa92491",
"index": null,
"score": 0.0,
"source": {
"guid": "10ececfc-8c1a-4039-9a4e-2027caa92491"
}
},
{
"id": "d1b8491b-24b3-4181-a206-fe05bf6d6218",
"index": null,
"score": 0.0,
"source": {
"guid": "d1b8491b-24b3-4181-a206-fe05bf6d6218"
}
}
],
"total": 2992
}
```
>Change the meta alert status to inactive
Just confirming expected behavior. When I set the meataalert to inactive and search for it, the child alerts still show the metaalerts array. Searching individual alerts does not include this array, as expected by the manual test instructions.
**Note**
In case it's helpful to anyone else, I did the following to run the 11 queries with all child guids and filter on the metaalerts array. I didn't want to copy paste and reconstruct every command:
```
for guid in 5c582812-7b7f-489e-bf69-f2235fd44044 3229525c-d31b-4083-b335-0a9f0645093d 0ce6f10a-f960-4da5-920f-a8bdc5350ebc 9174cbdc-5cea-42d0-a6c3-d656735744ae af610b0c-b978-44aa-adfd-efbc4b1dcc0d e0c40f3f-76ac-489e-af6b-831fb67eaa0f 2cab8b73-4bef-4b3c-9edd-4246d1c10bf3 be9037f0-4bf7-441b-8baa-4006a079e1f2 d0dc7bb6-011a-4004-8aa7-65a8ded0be78 5443fcae-6ded-48f3-b3de-b19f54b81f11 3a0dfcde-2408-4585-b90c-a5e0797667e0;
do
curl -s -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"'$guid'",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne' | python -m json.tool | grep -A 2 metaalerts
done;
```
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen I did a super POC refactoring I think is closer to what you're looking for. It only restructures the code more the way you're talking about (for Solr only, didn't have time to push everything to ES also), and doesn't resolve the code reuse problem. Check out https://github.com/justinleet/metron/tree/METRON-1421-interface-refactor
Right now the way that's set up is that it pushes the implementations down to a class level and composes them directly, which is a push closer to the way the base SolrDao does things.
It does not get rid of IndexDao. I think that's valuable to keep as a unified, single-entry point interface. The main implementations (SolrDao and SolrMetaAlertDao) defer primarily to the interface. I'm inclined to not break that for this PR (the current master does this exact thing, but duplicates the methods in IndexDao and the related DAOs, which I think is worse than just having IndexDao extend explicitly). We could have those vary independently, but I'm inclined to consider that outside the scope of this PR. MetaAlertDao is similarly structured by extending the MetaAlert* interfaces + IndexDao, ensuring that SolrMetaAlertDao meets all the requirements for the DAOs.
To make that branch viable, in my mind we need a couple things
1. Mirror the changes in the ES side
2. Solve the code reuse problem for Solr and ES.
3. Clean up tests for both
4. Probably other cleanup I'm not thinking of right now.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen Refactoring should be done, barring any additional changes. The only abstract class ends up being the UpdateDao (unless we want to be consistent), because mutation is really the main shared code. Otherwise I just chose to leave them.
I'm letting Travis take care of the running the existing tests for me (although I've run the obviously relevant tests locally).
I'll run through and see what other tests should be added tomorrow (and let me know if you come across any during review).
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Hopefully taking care of the test failures. Added appropriate teardowns on the solrcomponent usage on a couple tests. Also added some .gitignore on a couple log directories because I kept accidentally adding garbage because tests were spinning up.
---
[GitHub] metron pull request #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet closed the pull request at:
https://github.com/apache/metron/pull/970
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen My main concern re: pluggability is for a lot of this stuff, it's going to be very pretty specific to the fact that (right now) we're using Lucene stores (and it might be worth renaming some of the base classes to things like `AbstractLuceneMetaAlertDao` to make that more clear). The entire denormalization implementation is directly because of the limitations / strengths of Lucene stores. A store that implements more familiar joins would be totally different (and probably much, much simpler) than the ES and Solr impls.
I agree that refactoring is extremely nice to improve testability, but I'm not sure what the benefit of making things more extensively pluggable (beyond splitting the DAO up into the Search / Update / subsets + your example of calculating metascores is good). Are you more interested into breaking things out into more classes than that, just breaking apart functions more so they're more easily tested, some combination of both?
I guess the main question is: "Is breaking the meta alert dao into the various sub functions (Search, Update, etc.) + pulling out the calculate logic + at least a refactoring pass for testability a good first step in moving this forward?" I want to make sure there's at least a clear next step before doing a lot of adjusting, even if the exact final state might shift a bit.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Pulled in https://github.com/justinleet/metron/pull/18 from @merrimanr to address the reported bugs when metaalerts become empty. Thanks for digging into the bug you found, it's super helpful!
The short story for anyone here, is that the latest commit prevents the removal of all child alerts from a metalert (which caused problems for Solr in particular). Given that having an empty collection of alerts has no useful meaning (and there's no score and such), it's just disallowed entirely if a remove request will leave a metaalert empty.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll have to dig into that more. The stuff at the MultiIndexDao level wasn't touched, so I really don't know why the sensor type doesn't seem to be making it down there. It's possible something above that has a bug to track down.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
+1 from me. Thanks @justinleet, this was a beast.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
So, @nickwallen , do you propose implementation of Decorator or another such pattern? Is this problem inherent in the ES part as well? Should the whole thing be changed? If so in this PR or after this PR?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
It is definitely looking better. I am liking the progress. But I still think having overly broad interfaces that do everything is causing problems; primarily `IndexDao` and also `MetaAlertDao`.
Neither of those need to exist. For example...
* The `MetaAlertService` does not need a `MetaAlertDao`.
* If the `MetaAlertService` needs to update meta-alerts then it should directly use a `MetaAlertUpdateDao`.
* If the `MetaAlertService` needs to search for meta-alerts then it should directly use a `MetaAlertSearchDao`.
I can submit a PR to your branch, if you like.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I'll take a look and see where things are falling apart and make sure to get whatever test case sorted out. Sounds like the branch is in a state where we just spin up full dev, get Solr setup, and change the configs? Anything else needed?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I spun this up in full dev and can't quite get it to work. In the first test I am trying to create and modify a metaalert directly through Swagger. After switching the search engine to Solr and indexing data into Solr collections I run a search in Swagger to get a couple guids. Once I have those I try to create a metaalert through the http://node1:8082/swagger-ui.html#!/meta-alert-controller/createUsingPOST endpoint. Here is the request:
```
{
"alerts": [
{
"guid": "0ce62f0e-12cd-4dd7-994b-cd1f7220d9ec",
"index": "bro",
"sensorType": "bro"
},
{
"guid": "f425aa12-5a36-4461-917c-a61f47d9781e",
"index": "bro",
"sensorType": "bro"
}
],
"groups": [
"group"
]
}
```
Then I get this error response:
```
{
"responseCode": 500,
"message": "Unable to create meta alert",
"fullMessage": "IOException: class org.apache.metron.solr.dao.SolrDao: Error from server at http://10.0.2.15:8983/solr/metaalert: ERROR: [doc=8cf24075-520b-440d-8ae3-f63481a1660c] multiple values encountered for non multiValued field _childDocuments_: [{adapter.threatinteladapter.end.ts=1522100275770, bro_timestamp=1522100274.971272, ip_dst_port=8080, enrichmentsplitterbolt.splitter.end.ts=1522100275760, enrichmentsplitterbolt.splitter.begin.ts=1522100275760, adapter.hostfromjsonlistadapter.end.ts=1522100275763, adapter.geoadapter.begin.ts=1522100275763, uid=CUrRne3iLIxXavQtci, trans_depth=77, protocol=http, original_string=HTTP | id.orig_p:50451 method:GET request_body_len:0 id.resp_p:8080 uri:/api/v1/clusters/metron_cluster/components/?ServiceComponentInfo/component_name=APP_TIMELINE_SERVER|ServiceComponentInfo/category=MASTER&fields=ServiceComponentInfo/service_name,host_components/HostRoles/display_name,host_components/HostRoles/host_name,host_components/HostRoles/state,hos
t_components/HostRoles/maintenance_state,host_components/HostRoles/stale_configs,host_components/HostRoles/ha_state,host_components/HostRoles/desired_admin_state,,host_components/metrics/jvm/memHeapUsedM,host_components/metrics/jvm/HeapMemoryMax,host_components/metrics/jvm/HeapMemoryUsed,host_components/metrics/jvm/memHeapCommittedM,host_components/metrics/mapred/jobtracker/trackers_decommissioned,host_components/metrics/cpu/cpu_wio,host_components/metrics/rpc/client/RpcQueueTime_avg_time,host_components/metrics/dfs/FSNamesystem/*,host_components/metrics/dfs/namenode/Version,host_components/metrics/dfs/namenode/LiveNodes,host_components/metrics/dfs/namenode/DeadNodes,host_components/metrics/dfs/namenode/DecomNodes,host_components/metrics/dfs/namenode/TotalFiles,host_components/metrics/dfs/namenode/UpgradeFinalized,host_components/metrics/dfs/namenode/Safemode,host_components/metrics/runtime/StartTime,host_components/metrics/hbase/master/IsActiveMaster,host_components/metrics/hbase/m
aster/MasterStartTime,host_components/metrics/hbase/master/MasterActiveTime,host_components/metrics/hbase/master/AverageLoad,host_components/metrics/master/AssignmentManger/ritCount,metrics/api/v1/cluster/summary,metrics/api/v1/topology/summary,metrics/api/v1/nimbus/summary,host_components/metrics/yarn/Queue,host_components/metrics/yarn/ClusterMetrics/NumActiveNMs,host_components/metrics/yarn/ClusterMetrics/NumLostNMs,host_components/metrics/yarn/ClusterMetrics/NumUnhealthyNMs,host_components/metrics/yarn/ClusterMetrics/NumRebootedNMs,host_components/metrics/yarn/ClusterMetrics/NumDecommissionedNMs&minimal_response=true&_=1484168590350 tags:[] uid:CUrRne3iLIxXavQtci referrer:http://node1:8080/ trans_depth:77 host:node1 id.orig_h:192.168.66.1 response_body_len:0 user_agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36 ts:1522100274.971272 id.resp_h:192.168.66.121, ip_dst_addr=192.168.66.121, threatinteljoinbol
t.joiner.ts=1522100275773, host=node1, enrichmentjoinbolt.joiner.ts=1522100275765, adapter.hostfromjsonlistadapter.begin.ts=1522100275763, threatintelsplitterbolt.splitter.begin.ts=1522100275767, ip_src_addr=192.168.66.1, user_agent=Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36, timestamp=1522100274971, method=GET, request_body_len=0, uri=/api/v1/clusters/metron_cluster/components/?ServiceComponentInfo/component_name=APP_TIMELINE_SERVER|ServiceComponentInfo/category=MASTER&fields=ServiceComponentInfo/service_name,host_components/HostRoles/display_name,host_components/HostRoles/host_name,host_components/HostRoles/state,host_components/HostRoles/maintenance_state,host_components/HostRoles/stale_configs,host_components/HostRoles/ha_state,host_components/HostRoles/desired_admin_state,,host_components/metrics/jvm/memHeapUsedM,host_components/metrics/jvm/HeapMemoryMax,host_components/metrics/jvm/HeapMemoryUsed,host
_components/metrics/jvm/memHeapCommittedM,host_components/metrics/mapred/jobtracker/trackers_decommissioned,host_components/metrics/cpu/cpu_wio,host_components/metrics/rpc/client/RpcQueueTime_avg_time,host_components/metrics/dfs/FSNamesystem/*,host_components/metrics/dfs/namenode/Version,host_components/metrics/dfs/namenode/LiveNodes,host_components/metrics/dfs/namenode/DeadNodes,host_components/metrics/dfs/namenode/DecomNodes,host_components/metrics/dfs/namenode/TotalFiles,host_components/metrics/dfs/namenode/UpgradeFinalized,host_components/metrics/dfs/namenode/Safemode,host_components/metrics/runtime/StartTime,host_components/metrics/hbase/master/IsActiveMaster,host_components/metrics/hbase/master/MasterStartTime,host_components/metrics/hbase/master/MasterActiveTime,host_components/metrics/hbase/master/AverageLoad,host_components/metrics/master/AssignmentManger/ritCount,metrics/api/v1/cluster/summary,metrics/api/v1/topology/summary,metrics/api/v1/nimbus/summary,host_components/me
trics/yarn/Queue,host_components/metrics/yarn/ClusterMetrics/NumActiveNMs,host_components/metrics/yarn/ClusterMetrics/NumLostNMs,host_components/metrics/yarn/ClusterMetrics/NumUnhealthyNMs,host_components/metrics/yarn/ClusterMetrics/NumRebootedNMs,host_components/metrics/yarn/ClusterMetrics/NumDecommissionedNMs&minimal_response=true&_=1484168590350, metaalerts=[8cf24075-520b-440d-8ae3-f63481a1660c], source.type=bro, adapter.geoadapter.end.ts=1522100275763, referrer=http://node1:8080/, threatintelsplitterbolt.splitter.end.ts=1522100275767, adapter.threatinteladapter.begin.ts=1522100275770, ip_src_port=50451, guid=0ce62f0e-12cd-4dd7-994b-cd1f7220d9ec, response_body_len=0}, {adapter.threatinteladapter.end.ts=1522100275769, bro_timestamp=1522100274.955487, status_code=200, ip_dst_port=80, enrichmentsplitterbolt.splitter.end.ts=1522100275758, enrichments.geo.ip_dst_addr.latitude=55.7386, enrichments.geo.ip_dst_addr.location_point_0_coordinate=55.7386, enrichmentsplitterbolt.splitter.begi
n.ts=1522100275758, adapter.hostfromjsonlistadapter.end.ts=1522100275760, enrichments.geo.ip_dst_addr.country=RU, adapter.geoadapter.begin.ts=1522100275761, uid=CA0G2ASkF1efFirs7, resp_mime_types=[image/png], trans_depth=3, protocol=http, original_string=HTTP | id.orig_p:49210 status_code:200 method:GET request_body_len:0 id.resp_p:80 uri:/img/button_pay.png tags:[] uid:CA0G2ASkF1efFirs7 referrer:http://7oqnsnzwwnm6zb7y.gigapaysun.com/11iQmfg resp_mime_types:[\"image\\/png\"] trans_depth:3 host:7oqnsnzwwnm6zb7y.gigapaysun.com status_msg:OK id.orig_h:192.168.138.158 response_body_len:727 user_agent:Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0) ts:1522100274.955487 id.resp_h:95.163.121.204 resp_fuids:[\"F7c5Lp3iMksOUQHIbl\"], ip_dst_addr=95.163.121.204, threatinteljoinbolt.joiner.ts=1522100275772, host=7oqnsnzwwnm6zb7y.gigapaysun.com, enrichmentjoinbolt.joiner.ts=1522100275
763, adapter.hostfromjsonlistadapter.begin.ts=1522100275760, threatintelsplitterbolt.splitter.begin.ts=1522100275766, enrichments.geo.ip_dst_addr.longitude=37.6068, ip_src_addr=192.168.138.158, user_agent=Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0), resp_fuids=[F7c5Lp3iMksOUQHIbl], timestamp=1522100274955, method=GET, request_body_len=0, uri=/img/button_pay.png, metaalerts=[8cf24075-520b-440d-8ae3-f63481a1660c], source.type=bro, adapter.geoadapter.end.ts=1522100275761, referrer=http://7oqnsnzwwnm6zb7y.gigapaysun.com/11iQmfg, threatintelsplitterbolt.splitter.end.ts=1522100275766, adapter.threatinteladapter.begin.ts=1522100275769, ip_src_port=49210, enrichments.geo.ip_dst_addr.location_point_1_coordinate=37.6068, enrichments.geo.ip_dst_addr.location_point=55.7386,37.6068, status_msg=OK, guid=f425aa12-5a36-4461-917c-a61f47d9781e, response_body_len=727}]\norg.apache.solr.cl
ient.solrj.impl.HttpSolrClient$RemoteSolrException: Error from server at http://10.0.2.15:8983/solr/metaalert: ERROR: [doc=8cf24075-520b-440d-8ae3-f63481a1660c] multiple values encountered for non multiValued field _childDocuments_: [{adapter.threatinteladapter.end.ts=1522100275770, bro_timestamp=1522100274.971272, ip_dst_port=8080, enrichmentsplitterbolt.splitter.end.ts=1522100275760, enrichmentsplitterbolt.splitter.begin.ts=1522100275760, adapter.hostfromjsonlistadapter.end.ts=1522100275763, adapter.geoadapter.begin.ts=1522100275763, uid=CUrRne3iLIxXavQtci, trans_depth=77, protocol=http, original_string=HTTP | id.orig_p:50451 method:GET request_body_len:0 id.resp_p:8080 uri:/api/v1/clusters/metron_cluster/components/?ServiceComponentInfo/component_name=APP_TIMELINE_SERVER|ServiceComponentInfo/category=MASTER&fields=ServiceComponentInfo/service_name,host_components/HostRoles/display_name,host_components/HostRoles/host_name,host_components/HostRoles/state,host_components/HostRoles/m
aintenance_state,host_components/HostRoles/stale_configs,host_components/HostRoles/ha_state,host_components/HostRoles/desired_admin_state,,host_components/metrics/jvm/memHeapUsedM,host_components/metrics/jvm/HeapMemoryMax,host_components/metrics/jvm/HeapMemoryUsed,host_components/metrics/jvm/memHeapCommittedM,host_components/metrics/mapred/jobtracker/trackers_decommissioned,host_components/metrics/cpu/cpu_wio,host_components/metrics/rpc/client/RpcQueueTime_avg_time,host_components/metrics/dfs/FSNamesystem/*,host_components/metrics/dfs/namenode/Version,host_components/metrics/dfs/namenode/LiveNodes,host_components/metrics/dfs/namenode/DeadNodes,host_components/metrics/dfs/namenode/DecomNodes,host_components/metrics/dfs/namenode/TotalFiles,host_components/metrics/dfs/namenode/UpgradeFinalized,host_components/metrics/dfs/namenode/Safemode,host_components/metrics/runtime/StartTime,host_components/metrics/hbase/master/IsActiveMaster,host_components/metrics/hbase/master/MasterStartTime,ho
st_components/metrics/hbase/master/MasterActiveTime,host_components/metrics/hbase/master/AverageLoad,host_components/metrics/master/AssignmentManger/ritCount,metrics/api/v1/cluster/summary,metrics/api/v1/topology/summary,metrics/api/v1/nimbus/summary,host_components/metrics/yarn/Queue,host_components/metrics/yarn/ClusterMetrics/NumActiveNMs,host_components/metrics/yarn/ClusterMetrics/NumLostNMs,host_components/metrics/yarn/ClusterMetrics/NumUnhealthyNMs,host_components/metrics/yarn/ClusterMetrics/NumRebootedNMs,host_components/metrics/yarn/ClusterMetrics/NumDecommissionedNMs&minimal_response=true&_=1484168590350 tags:[] uid:CUrRne3iLIxXavQtci referrer:http://node1:8080/ trans_depth:77 host:node1 id.orig_h:192.168.66.1 response_body_len:0 user_agent:Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36 ts:1522100274.971272 id.resp_h:192.168.66.121, ip_dst_addr=192.168.66.121, threatinteljoinbolt.joiner.ts=152210027577
3, host=node1, enrichmentjoinbolt.joiner.ts=1522100275765, adapter.hostfromjsonlistadapter.begin.ts=1522100275763, threatintelsplitterbolt.splitter.begin.ts=1522100275767, ip_src_addr=192.168.66.1, user_agent=Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36, timestamp=1522100274971, method=GET, request_body_len=0, uri=/api/v1/clusters/metron_cluster/components/?ServiceComponentInfo/component_name=APP_TIMELINE_SERVER|ServiceComponentInfo/category=MASTER&fields=ServiceComponentInfo/service_name,host_components/HostRoles/display_name,host_components/HostRoles/host_name,host_components/HostRoles/state,host_components/HostRoles/maintenance_state,host_components/HostRoles/stale_configs,host_components/HostRoles/ha_state,host_components/HostRoles/desired_admin_state,,host_components/metrics/jvm/memHeapUsedM,host_components/metrics/jvm/HeapMemoryMax,host_components/metrics/jvm/HeapMemoryUsed,host_components/metrics/jvm/
memHeapCommittedM,host_components/metrics/mapred/jobtracker/trackers_decommissioned,host_components/metrics/cpu/cpu_wio,host_components/metrics/rpc/client/RpcQueueTime_avg_time,host_components/metrics/dfs/FSNamesystem/*,host_components/metrics/dfs/namenode/Version,host_components/metrics/dfs/namenode/LiveNodes,host_components/metrics/dfs/namenode/DeadNodes,host_components/metrics/dfs/namenode/DecomNodes,host_components/metrics/dfs/namenode/TotalFiles,host_components/metrics/dfs/namenode/UpgradeFinalized,host_components/metrics/dfs/namenode/Safemode,host_components/metrics/runtime/StartTime,host_components/metrics/hbase/master/IsActiveMaster,host_components/metrics/hbase/master/MasterStartTime,host_components/metrics/hbase/master/MasterActiveTime,host_components/metrics/hbase/master/AverageLoad,host_components/metrics/master/AssignmentManger/ritCount,metrics/api/v1/cluster/summary,metrics/api/v1/topology/summary,metrics/api/v1/nimbus/summary,host_components/metrics/yarn/Queue,host_co
mponents/metrics/yarn/ClusterMetrics/NumActiveNMs,host_components/metrics/yarn/ClusterMetrics/NumLostNMs,host_components/metrics/yarn/ClusterMetrics/NumUnhealthyNMs,host_components/metrics/yarn/ClusterMetrics/NumRebootedNMs,host_components/metrics/yarn/ClusterMetrics/NumDecommissionedNMs&minimal_response=true&_=1484168590350, metaalerts=[8cf24075-520b-440d-8ae3-f63481a1660c], source.type=bro, adapter.geoadapter.end.ts=1522100275763, referrer=http://node1:8080/, threatintelsplitterbolt.splitter.end.ts=1522100275767, adapter.threatinteladapter.begin.ts=1522100275770, ip_src_port=50451, guid=0ce62f0e-12cd-4dd7-994b-cd1f7220d9ec, response_body_len=0}, {adapter.threatinteladapter.end.ts=1522100275769, bro_timestamp=1522100274.955487, status_code=200, ip_dst_port=80, enrichmentsplitterbolt.splitter.end.ts=1522100275758, enrichments.geo.ip_dst_addr.latitude=55.7386, enrichments.geo.ip_dst_addr.location_point_0_coordinate=55.7386, enrichmentsplitterbolt.splitter.begin.ts=1522100275758, adap
ter.hostfromjsonlistadapter.end.ts=1522100275760, enrichments.geo.ip_dst_addr.country=RU, adapter.geoadapter.begin.ts=1522100275761, uid=CA0G2ASkF1efFirs7, resp_mime_types=[image/png], trans_depth=3, protocol=http, original_string=HTTP | id.orig_p:49210 status_code:200 method:GET request_body_len:0 id.resp_p:80 uri:/img/button_pay.png tags:[] uid:CA0G2ASkF1efFirs7 referrer:http://7oqnsnzwwnm6zb7y.gigapaysun.com/11iQmfg resp_mime_types:[\"image\\/png\"] trans_depth:3 host:7oqnsnzwwnm6zb7y.gigapaysun.com status_msg:OK id.orig_h:192.168.138.158 response_body_len:727 user_agent:Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0) ts:1522100274.955487 id.resp_h:95.163.121.204 resp_fuids:[\"F7c5Lp3iMksOUQHIbl\"], ip_dst_addr=95.163.121.204, threatinteljoinbolt.joiner.ts=1522100275772, host=7oqnsnzwwnm6zb7y.gigapaysun.com, enrichmentjoinbolt.joiner.ts=1522100275763, adapter.hostfromjso
nlistadapter.begin.ts=1522100275760, threatintelsplitterbolt.splitter.begin.ts=1522100275766, enrichments.geo.ip_dst_addr.longitude=37.6068, ip_src_addr=192.168.138.158, user_agent=Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0), resp_fuids=[F7c5Lp3iMksOUQHIbl], timestamp=1522100274955, method=GET, request_body_len=0, uri=/img/button_pay.png, metaalerts=[8cf24075-520b-440d-8ae3-f63481a1660c], source.type=bro, adapter.geoadapter.end.ts=1522100275761, referrer=http://7oqnsnzwwnm6zb7y.gigapaysun.com/11iQmfg, threatintelsplitterbolt.splitter.end.ts=1522100275766, adapter.threatinteladapter.begin.ts=1522100275769, ip_src_port=49210, enrichments.geo.ip_dst_addr.location_point_1_coordinate=37.6068, enrichments.geo.ip_dst_addr.location_point=55.7386,37.6068, status_msg=OK, guid=f425aa12-5a36-4461-917c-a61f47d9781e, response_body_len=727}]\n\tat org.apache.solr.client.solrj.impl.Htt
pSolrClient.executeMethod(HttpSolrClient.java:612)\n\tat org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:279)\n\tat org.apache.solr.client.solrj.impl.HttpSolrClient.request(HttpSolrClient.java:268)\n\tat org.apache.solr.client.solrj.impl.LBHttpSolrClient.doRequest(LBHttpSolrClient.java:447)\n\tat org.apache.solr.client.solrj.impl.LBHttpSolrClient.request(LBHttpSolrClient.java:388)\n\tat org.apache.solr.client.solrj.impl.CloudSolrClient.sendRequest(CloudSolrClient.java:1383)\n\tat org.apache.solr.client.solrj.impl.CloudSolrClient.requestWithRetryOnStaleState(CloudSolrClient.java:1134)\n\tat org.apache.solr.client.solrj.impl.CloudSolrClient.request(CloudSolrClient.java:1073)\n\tat org.apache.solr.client.solrj.SolrRequest.process(SolrRequest.java:160)\n\tat org.apache.solr.client.solrj.SolrClient.add(SolrClient.java:106)\n\tat org.apache.solr.client.solrj.SolrClient.add(SolrClient.java:71)\n\tat org.apache.metron.solr.dao.SolrUpdateDao.batchUpdate(SolrUpdat
eDao.java:87)\n\tat org.apache.metron.solr.dao.SolrDao.batchUpdate(SolrDao.java:118)\n\tat org.apache.metron.indexing.dao.MultiIndexDao.lambda$batchUpdate$3(MultiIndexDao.java:79)\n\tat java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)\n\tat java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374)\n\tat java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)\n\tat java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)\n\tat java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:747)\n\tat java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:721)\n\tat java.util.stream.AbstractTask.compute(AbstractTask.java:316)\n\tat java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:731)\n\tat java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)\n\tat java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)\n\tat java.util.concurrent.ForkJoinPool.runWorker(ForkJo
inPool.java:1692)\n\tat java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)\n\nclass org.apache.metron.indexing.dao.HBaseDao: Guid and sensor type must not be null: guid = f425aa12-5a36-4461-917c-a61f47d9781e, sensorType = null\njava.lang.IllegalStateException: Guid and sensor type must not be null: guid = f425aa12-5a36-4461-917c-a61f47d9781e, sensorType = null\n\tat org.apache.metron.indexing.dao.HBaseDao$Key.toBytes(HBaseDao.java:98)\n\tat org.apache.metron.indexing.dao.HBaseDao$Key.toBytes(HBaseDao.java:110)\n\tat org.apache.metron.indexing.dao.HBaseDao.buildPut(HBaseDao.java:252)\n\tat org.apache.metron.indexing.dao.HBaseDao.batchUpdate(HBaseDao.java:237)\n\tat org.apache.metron.indexing.dao.MultiIndexDao.lambda$batchUpdate$3(MultiIndexDao.java:79)\n\tat java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)\n\tat java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374)\n\tat java.util.stream.AbstractPipeline.copy
Into(AbstractPipeline.java:481)\n\tat java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)\n\tat java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:747)\n\tat java.util.stream.ReduceOps$ReduceTask.doLeaf(ReduceOps.java:721)\n\tat java.util.stream.AbstractTask.compute(AbstractTask.java:316)\n\tat java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:731)\n\tat java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)\n\tat java.util.concurrent.ForkJoinTask.doInvoke(ForkJoinTask.java:401)\n\tat java.util.concurrent.ForkJoinTask.invoke(ForkJoinTask.java:734)\n\tat java.util.stream.ReduceOps$ReduceOp.evaluateParallel(ReduceOps.java:714)\n\tat java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:233)\n\tat java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:499)\n\tat org.apache.metron.indexing.dao.MultiIndexDao.batchUpdate(MultiIndexDao.java:84)\n\tat org.apache.metron.indexing.dao.AbstractMetaAlertDao.indexDaoU
pdate(AbstractMetaAlertDao.java:273)\n\tat org.apache.metron.solr.dao.SolrMetaAlertDao.createMetaAlert(SolrMetaAlertDao.java:278)\n\tat org.apache.metron.rest.service.impl.MetaAlertServiceImpl.create(MetaAlertServiceImpl.java:55)\n\tat org.apache.metron.rest.controller.MetaAlertController.create(MetaAlertController.java:67)\n\tat sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)\n\tat sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)\n\tat sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)\n\tat java.lang.reflect.Method.invoke(Method.java:498)\n\tat org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:221)\n\tat org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:136)\n\tat org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:114)\
n\tat org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:827)\n\tat org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:738)\n\tat org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:85)\n\tat org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:963)\n\tat org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:897)\n\tat org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:970)\n\tat org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:872)\n\tat javax.servlet.http.HttpServlet.service(HttpServlet.java:648)\n\tat org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:846)\n\tat javax.servlet.http.HttpServlet.service(HttpSe
rvlet.java:729)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:230)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:317)\n\tat org.springframework.security.web.access.intercept.FilterSecurityInterceptor.invoke(FilterSecurityInterceptor.java:127)\n\tat org.springframework.security.web.access.intercept.FilterSecurityInterceptor.doFilter(FilterSecurityInterceptor.java:91)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.secur
ity.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:115)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:137)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:111)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:169)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.savedrequest.RequestCacheAware
Filter.doFilter(RequestCacheAwareFilter.java:63)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.authentication.www.BasicAuthenticationFilter.doFilterInternal(BasicAuthenticationFilter.java:215)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:121)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:66)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFil
terChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.context.SecurityContextPersistenceFilter.doFilter(SecurityContextPersistenceFilter.java:105)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:56)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:331)\n\tat org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:214)\n\tat org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:177)\n\tat org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:346)\n\tat org.springframework.web.filter.DelegatingFi
lterProxy.doFilter(DelegatingFilterProxy.java:262)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:89)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.co
re.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:77)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:197)\n\tat org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)\n\tat org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:192)\n\tat org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:165)\n\tat org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:198)\n\tat org.apache.catal
ina.core.StandardContextValve.invoke(StandardContextValve.java:108)\n\tat org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:472)\n\tat org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:140)\n\tat org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:79)\n\tat org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:87)\n\tat org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:349)\n\tat org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:784)\n\tat org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66)\n\tat org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:802)\n\tat org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1410)\n\tat org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)\n\tat java.util.concurrent.ThreadPoolExecutor.runWorker(Thread
PoolExecutor.java:1142)\n\tat java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)\n\tat org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)\n\tat java.lang.Thread.run(Thread.java:745)\n"
}
```
I also tried testing this from the Alerts UI. After grouping a couple alerts together and trying to create a metaalert, this request is sent to REST:
```
{
"alerts": [
{
"guid": "f823d536-09c9-4eed-a50a-0a786a30c401",
"sensorType": null,
"index": null
},
{
"guid": "3090f2a1-f8b4-44c3-9bd4-5e23584f94ca",
"sensorType": null,
"index": null
},
{
"guid": "94050674-4b4c-425a-90b6-f9e48cb92206",
"sensorType": null,
"index": null
}
],
"groups": [
"ip_dst_addr"
]
}
```
Notice the null value for the sensorType and index. Not sure if this was some kind of regression that happened before this PR or not. After fixing those fields and sending the request again through Swagger, I get the same 500 error above.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr I fixed the metaalert template; I'd forgotten to update the original placeholder one in addition to the test one. I haven't had a chance to run it up and see if it fixes what you saw, but it explains the _childDocuments_ problem. The message itself is a bit of a red herring: it's a special field, so the lack of definition at all caused problem.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Thanks @justinleet for consuming my rant. :)
I agree with most of what you're saying. I'm perfectly happy iterating improvements on this. And totally willing to help. One disagreement...
> For the specific calculating metascores case, and probably some of the other stuff, it only seems useful to break that sort of thing into it's own class if we intend on making it pluggable.
I disagree here. Making things pluggable, even if we now currently only have one type of plug, is still beneficial. It is going to help make it more testable and shrink the square footage of that class.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
I have run the following set of manual tests against the REST UI.
Do the following to setup Solr after ssh'ing into full dev
```
sudo su -
export METRON_HOME=/usr/metron/0.4.3/
cd ${METRON_HOME}/bin/
./start_solr.sh
./install_solr.sh
./create_collection.sh bro
./create_collection.sh yah
./create_collection.sh snort
./create_collection.sh error
./create_collection.sh metaalert
```
In Ambari:
Indexing -> Random Access Search Engine -> Solr
# Meta Alerts Test
## Adding alerts and adding a preexisting alert
### Find two alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"fields": [
"guid"
],
"from": 0,
"indices": [
"snort"
],
"query": "ip_dst_addr:192.168.66.121",
"size": 2
}' 'http://node1:8082/api/v1/search/search'
```
Results in two guids:
{"total":256,"results":[{"id":"e4e35dff-b160-4c21-bb48-251ff873d7c6","source":{"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6"},"score":0.0,"index":null},{"id":"5dde3195-4076-4dea-89b6-c65c88b1357e","source":{"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e"},"score":0.0,"index":null}],"facetCounts":null}
```
e4e35dff-b160-4c21-bb48-251ff873d7c6
5dde3195-4076-4dea-89b6-c65c88b1357e
```
### Create a metaalert with only one of the alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort",
"sensorType": "snort"
}
],
"groups": [
"test"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
Make sure to get the resulting guid from the response.
```
15354a7e-0a17-4b1d-b64e-25bc465b27a9
```
### Retrieve the meta alert and ensure it contains the provided alert
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alert and ensure the 'metaalerts' field contains the new GUID of the new metaalert
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Add the same alert to the meta alert
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort",
"sensorType": "snort"
}
],
"metaAlertGuid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9"
}' 'http://node1:8082/api/v1/metaalert/add/alert'
```
It should return "false" as no alerts have been added. The meta alert should be retrieved again to validate.
### Run the add alert again but with the second alert
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort_index_2017.11.15.17",
"sensorType": "snort"
},
{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"index": "snort_index_2017.11.15.17",
"sensorType": "snort"
}
],
"metaAlertGuid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9"
}' 'http://node1:8082/api/v1/metaalert/add/alert'
```
It should return true, because the second alert will be added. The meta alert should be retrieved again to validate.
### Retrieve the child alerts
Ensure they have the 'metaalerts' field populated with their parent.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
## Removing alerts and removing an already removed alert
### Find two alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"fields": [
"guid"
],
"from": 0,
"indices": [
"snort"
],
"query": "ip_dst_addr:192.168.66.121",
"size": 2
}' 'http://node1:8082/api/v1/search/search'
```
Results in two guids:
```
e4e35dff-b160-4c21-bb48-251ff873d7c6
5dde3195-4076-4dea-89b6-c65c88b1357e
```
### Create a metaalert with the alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"index": "snort",
"sensorType": "snort"
}
],
"groups": [
"test"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
Make sure to get the resulting guid from the response.
```
15354a7e-0a17-4b1d-b64e-25bc465b27a9
```
### Retrieve the meta alert and ensure it contains the provided alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Remove one of the alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort",
"sensorType": "snort"
}
],
"metaAlertGuid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9"
}' 'http://node1:8082/api/v1/metaalert/remove/alert'
```
### Retrieve the meta alert again, and ensure it only contains the second alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert_index",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Rerun the delete
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "e4e35dff-b160-4c21-bb48-251ff873d7c6",
"index": "snort",
"sensorType": "snort"
}
],
"metaAlertGuid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9"
}' 'http://node1:8082/api/v1/metaalert/remove/alert'
```
### Retrieve the meta alert again, and ensure it only contains the second alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert_index",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alerts
Ensure only the second alert has the 'metaalerts' field populated with the parent met alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
## Changing Metaalert status
### Find two alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"fields": [
"guid"
],
"from": 0,
"indices": [
"snort"
],
"query": "ip_dst_addr:192.168.66.121",
"size": 2
}' 'http://node1:8082/api/v1/search/search'
```
Results in two guids:
```
e4e35dff-b160-4c21-bb48-251ff873d7c6
5dde3195-4076-4dea-89b6-c65c88b1357e
```
### Create a metaalert with the alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "8b8314d4-277b-44dc-a75b-04b0cdcedb40",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"4ac26cf7-ab93-4940-9a0e-8e7f4d67736d",
"index": "snort",
"sensorType": "snort"
}
],
"groups": [
"test"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
Make sure to get the resulting guid from the response.
```
15354a7e-0a17-4b1d-b64e-25bc465b27a9
```
### Retrieve the meta alert and ensure it contains the provided alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Change the meta alert status to active
This makes sure nothing happens when we set active status to the same active status.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' 'http://node1:8082/api/v1/metaalert/update/status/15354a7e-0a17-4b1d-b64e-25bc465b27a9/active'
```
It should return false, as no status has changed.
### Retrieve the metaalert and ensure it is still active
Look for the 'status' field.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alerts
Ensure both alerts have the 'metaalerts' field populated with the parent meta alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Change the meta alert status to inactive
Look for the 'status' field. Running this once will set it to 'inactive'. Subsequent runs have no effect ('inactive' -> 'inactive' does nothing).
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' 'http://node1:8082/api/v1/metaalert/update/status/15354a7e-0a17-4b1d-b64e-25bc465b27a9/inactive'
```
It should return true, because the status has changed.
### Retrieve the metaalert and ensure it is inactive
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alerts
Ensure neither alert has the 'metaalerts' field populated with the parent meta alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Change status back to active
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' 'http://node1:8082/api/v1/metaalert/update/status/15354a7e-0a17-4b1d-b64e-25bc465b27a9/active'
```
It should return true, as the status has changed.
### Retrieve the metaalert and ensure it is active again
Look for the 'status' field.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "15354a7e-0a17-4b1d-b64e-25bc465b27a9",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alerts
Ensure both alerts have the 'metaalerts' field populated with the parent meta alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"e4e35dff-b160-4c21-bb48-251ff873d7c6",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"5dde3195-4076-4dea-89b6-c65c88b1357e",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
```
## Create meta alert with more than 10 alerts
### Find more than 10 alerts alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"fields": [
"guid"
],
"from": 0,
"indices": [
"snort"
],
"query": "ip_dst_addr:192.168.66.121",
"size": 11
}' 'http://node1:8082/api/v1/search/search'
```
Note the alerts that come back
```
02468e00-3183-413d-85f0-959f77279f10
e9ef65d4-f896-42bf-b4a1-c30785e9124b
c4c114f1-f7c2-48ef-87a3-3a843baf6e4c
d6cd9342-5638-4e5a-8ab6-1cc0a982455f
882ce84d-770e-432a-8999-cc3f4a9cc08e
20fc092e-e48a-4468-a581-fee1903da66a
ac611b4f-0271-4cb2-b8d4-a64d2221929b
80002446-2cb8-4998-85d3-653a79fc0f6f
9ccf7a96-2340-4dac-9d5d-e983f4a35c38
6decf7bd-f5fc-4e03-9560-819d64048cff
7b099a2c-0740-4d02-a567-bcb8e00b8e31
```
### Create a metaalert with the alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"alerts": [
{
"guid": "02468e00-3183-413d-85f0-959f77279f10",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"e9ef65d4-f896-42bf-b4a1-c30785e9124b",
"index": "snort",
"sensorType": "snort"
},
{
"guid": "c4c114f1-f7c2-48ef-87a3-3a843baf6e4c",
"index": "snort",
"sensorType": "snort"
},
{
"guid": "d6cd9342-5638-4e5a-8ab6-1cc0a982455f",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"882ce84d-770e-432a-8999-cc3f4a9cc08e",
"index": "snort",
"sensorType": "snort"
},
{
"guid": "20fc092e-e48a-4468-a581-fee1903da66a",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"ac611b4f-0271-4cb2-b8d4-a64d2221929b",
"index": "snort",
"sensorType": "snort"
},
{
"guid": "80002446-2cb8-4998-85d3-653a79fc0f6f",
"index": "snort",
"sensorType": "snort"
},
{
"guid": "9ccf7a96-2340-4dac-9d5d-e983f4a35c38",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"6decf7bd-f5fc-4e03-9560-819d64048cff",
"index": "snort",
"sensorType": "snort"
},
{
"guid":"7b099a2c-0740-4d02-a567-bcb8e00b8e31",
"index": "snort",
"sensorType": "snort"
}
],
"groups": [
"test"
]
}' 'http://node1:8082/api/v1/metaalert/create'
```
Make sure to get the resulting guid from the response.
```
4c772ffd-8f1d-4528-885e-a528fd085673
```
### Retrieve the meta alert and ensure it contains the provided alerts
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "4c772ffd-8f1d-4528-885e-a528fd085673",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
### Retrieve the child alerts
Ensure all alerts have the 'metaalerts' field populated with the parent meta alert.
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid":"02468e00-3183-413d-85f0-959f77279f10",
"sensorType": "snort"
}' 'http://node1:8082/api/v1/search/findOne'
... // 10 more times
```
## Patch alert and status
Create a metaalerts and get the GUID for the following steps.
### Attempt to update status field
```
curl -u user:password -X PATCH --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "4c772ffd-8f1d-4528-885e-a528fd085673",
"index": "metaalert",
"patch": [
{
"op": "replace"
, "path": "/status"
, "value": "failure"
}
],
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/update/patch'
```
Should return
```
{
"responseCode": 500,
"message": "Meta alert patches are not allowed for /alert or /status paths. Please use the add/remove alert or update status functions instead.",
"fullMessage": "IllegalArgumentException: Meta alert patches are not allowed for /alert or /status paths. Please use the add/remove alert or update status functions instead."
}
```
### Attempt to update the alert list
```
curl -u user:password -X PATCH --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "4c772ffd-8f1d-4528-885e-a528fd085673",
"index": "metaalert",
"patch": [
{
"op": "replace"
, "path": "/alert"
, "value": [{
"alertOne":"test"
},
{
"alertTwo":"test"
}
]
}
],
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/update/patch'
```
Should return
```
{
"responseCode": 500,
"message": "Meta alert patches are not allowed for /alert or /status paths. Please use the add/remove alert or update status functions instead.",
"fullMessage": "IllegalArgumentException: Meta alert patches are not allowed for /alert or /status paths. Please use the add/remove alert or update status functions instead."
}
```
## Patch neither alert and status
Create a metaalerts and get the GUID for the following steps.
### Patch in new field
```
curl -u user:password -X PATCH --header 'Content-Type: application/json' --header 'Accept: */*' -d '{
"guid": "4c772ffd-8f1d-4528-885e-a528fd085673",
"index": "metaalert",
"patch": [
{
"op": "add"
, "path": "/name"
, "value": "My new meta alert name"
}
],
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/update/patch'
```
### Retrieve the meta alert and ensure it contains the new 'name' field
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"guid": "4c772ffd-8f1d-4528-885e-a528fd085673",
"index": "metaalert",
"sensorType": "metaalert"
}' 'http://node1:8082/api/v1/search/findOne'
```
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc I think that's a good insight. The original intention of metaalerts was to basically wrap and augment the functionality of the index alerts. I think a portion of that problem we've hit is that wrapping that is both specific in implementation and general in purpose and it blurs that line between data store and domain model object.
I definitely agree that a discuss thread on our access patterns is a good outcome of this. Doing an second implementation is always revealing for things like this, and taking that insight and improving things for the future would be good.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
Do you think we have enough unit tests on this functionality how it is?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I'm still getting an error in full dev. When I execute this create metaalert function in REST:
```
{
"alerts": [
{
"guid": "45534065-a8bb-4c01-8c83-1812e661f76e",
"index": "bro",
"sensorType": "bro"
},
{
"guid": "654008ff-d389-4721-86f9-076a4315b7df",
"index": "bro",
"sensorType": "bro"
}
],
"groups": [
"string"
]
}
```
I get this error:
```
IOException: class org.apache.metron.indexing.dao.HBaseDao: Guid and sensor type must not be null: guid = 45534065-a8bb-4c01-8c83-1812e661f76e, sensorType = null
java.lang.IllegalStateException: Guid and sensor type must not be null: guid = 45534065-a8bb-4c01-8c83-1812e661f76e, sensorType = null
org.apache.metron.indexing.dao.HBaseDao$Key.toBytes(HBaseDao.java:98)
org.apache.metron.indexing.dao.HBaseDao$Key.toBytes(HBaseDao.java:110)
org.apache.metron.indexing.dao.HBaseDao.buildPut(HBaseDao.java:252)
org.apache.metron.indexing.dao.HBaseDao.batchUpdate(HBaseDao.java:237)
org.apache.metron.indexing.dao.MultiIndexDao.lambda$batchUpdate$3(MultiIndexDao.java:79)
java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193)
java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374)
java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481)
java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471)
```
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Last commit is the one I tested on. I just forgot to push it up.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> I'd like to know what specific problems we're going to solve that merit that level of change in this PR.
The fundamental problem here is a lack of good unit tests on these search DAOs. That is why I am interested in refactoring this.
What I don't want to repeat is #832 or [the recent issue found in Full Dev on this PR](https://github.com/apache/metron/pull/970#issuecomment-379753506). Are these clues that we don't have sufficient testing?
We really need quality tests here in particular, because this is a complex and difficult to implement feature. Because it is so difficult, we need to have **especially** good automated testing around this.
I completely appreciate not wanting to expand scope too much. But when adding a big chunk of net-new functionality, you inherently have to refactor to keep things maintainable. To what degree refactoring is needed, is a judgement call of course.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by cestella <gi...@git.apache.org>.
Github user cestella commented on the issue:
https://github.com/apache/metron/pull/970
I'm giving this a +1 after going through the test script, great job and thanks for the effort.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
This is bringing me flashbacks from when I did #832. I found the Index DAOs really hard to work with and grok back then. I had to refactor them slightly to get decent unit tests for the one thing I was trying to fix. I feel like we are running into the same problem.
I apologize if this feedback is too abstract and not specific enough. Maybe I'm way off base because I haven't spent enough time in this portion of the code. Maybe some of this is pre-existing, but whatever the case is, I'd really like to try and avoid repeating another #832.
(1) Use composition over inhertence
I think trying to share code between the two implementations (ES and Solr) via inheritance with an abstract base class is going to get us in trouble. I would do exactly the kind of refactoring you did, but instead via composition.
The two implementations are now fairly tied at the hip. Code is shared between the two in the base class without well-defined abstractions defining how they interact. Changes in the base class to evolve Solr is likely to break ES or vice versa.
A good example of this is in `removeAlertsFromMetaAlert` lines 128 - 131. It seems we had to add some logic in here due to a qwerk in Solr. We are only going to find more qwerks over time. The abstraction is already breaking a bit here and will continue to do so.
(2) These DAOs are big.
I don't see that many unit tests around this; mostly integration tests. And I think that is partly because we could break this logic down into multiple, smaller chunks to make testing it easier. And that also makes it easier for newbies to grok.
For example, there is logic in there for calculating meta scores. That should be broken out into its own class whose sole responsibility is to do that. Then we can easily write unit tests around it. Right now, that logic is all tied up in the abstract base class which prevents us from easily unit testing it.
There is other high-level logic like that, that we could easily test in unit tests, if we refactored this and broke this down into smaller chunks.
* Do not add/remove alerts from/to a metaalert that is not active.
* Only add alert to a meta alert, if it does not already exist
* Only remove alert it it is actually in the metaalert
* If making meta alert active, add the meta alert guid to every attached alert.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@merrimanr The previous commits should also (hopefully) resolve the issues with `source.type` vs `source:type`. Given that there were issues with the tests and test schemas themselves I haven't added any new tests, but if we see it in full-dev still, we'll need to make sure we either add tests or fix any remaining issues in the current tests, depending on what the exact details are.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
I dug into this further. I think fundamentally there are a few issues causing problems.
1. The extensive inheritance amongst all of these interfaces is problematic. This introduces hard dependencies and is difficult to make sense of. This picture does not look clean to me; IMHO.
<img src="https://user-images.githubusercontent.com/2475409/38499370-537d4f12-3bd5-11e8-8e1b-8171630e8adf.jpeg" width=200>
For example, why is an `UpdateDao` also a `RetrieveLatestDao`? These need to be completely separate. There should be no inheritance between the two. There should be a `SolrUpdateDao -> UpdateDao` and a `SolrRetrieveLatestDao -> RetrieveLatestDao`.
If someone needs to do updates, they should use the `UpdateDao`. If someone needs to get the latest documents, they should the `RetrieveLatestDao`.
The same logic can be applied to the other interfaces.
1. I believe all of this inheritance, along with all of the default methods, exist because we are using inheritance for code reuse, instead of composition. This prevents it from being more modular, simple, and testable.
For example, the `SolrMetaAlertDao` should **use** a `RetrieveLatestDao`, it should **not be** a `RetrieveLatestDao`.
Another example, if the `SolrUpdateDao` needs to also search, then it should be passed an instance of a `SearchDao` to do that. It does not itself need to be a `SearchDao`.
1. There are also unnecessary interfaces that cause confusion.
For example, there are compositional uses of the interface `IndexDAO`. But what is an `IndexDAO`? Based on the inheritance hierarchy an `IndexDAO` is an `UpdateDao`, a `SearchDao`, a `RetrieveLatestDao` all at the same time.
If something needs to do updates, then pass in an `UpdateDao`. If something needs to search, then pass in a `SearchDao`. If something needs to do BOTH, then pass in both a `SearchDao` and an `UpdateDao`.
The only real method in `IndexDao` seems exactly the same as a `ColumnMetadataDao`. So if something needs that functionality, it should be given a `ColumnMetadataDao` not an `IndexDao`.
The `IndexDao` should either be given some real purpose or die a quick and painless death
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Merged in master, and in particular METRON-1540. Minor updates occurred to get everything aligned and the columns count was incremented by one for a couple test cases (for the schemas' metaalert field)
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen
Right now those interfaces are basically a marker interface that requires everything that's needed for a complete DAO set. Prior to some of the Solr refactorings, it existed as a single DAO to provide a complete set of functionality to be implemented in order to allow an implementation to be complete (for some definition of complete) and available to be swapped out via configuration. Even with the refactoring into multiple interfaces, the IndexDao and MetaAlertDao still exist to provide some definition of complete.
Getting rid of IndexDao in particular likely has potentially broader implications because right now that interface ties together the various layers. Making those changes in a complete and consistent manner is larger than MetaAlertService, because the SearchServiceImpl and others take an IndexDao (which could be a regular SolrDao or SolrMetaAlertDao, etc.) in order to allow regular searches to have meta alert adjustments, e.g. massage the queries to make sure metaalerts get searched and returned.
Another problem is there's refactoring to existing infrastructure and patterns around the creation and composition of IndexDaos. In addition, configuration changes will need to be made (and thought out) without that interface existing. I'm not sure how large that refactoring is, but I think changing and testing that is nontrivial enough that it should be a separate PR.
In particular, I'm worried about increasing the scope of an already large and complicated PR and I'm disinclined to pull that task into this PR without compelling reasons. If removing those interfaces is a necessity for moving forward here, I'd like to see that as a PR against the parent feature branch that we pull into this branch when it's done. I'm happy to help out using this branch (or a derivative thereof) as a pilot / testing ground for that, but again I think removing the basic DAO interface itself is outside the scope of this.
I'd like to know what specific problems we're going to solve that merit that level of change in this PR. IMO, those problems would need to be pretty compelling (e.g. we find that reducing the duplication between ES and Solr necessitates it) to increase the scope that way unless the changes end up being substantially easier and less impactful than I'd expect.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen
Assuming we go through with the partial refactoring, the existence of those interfaces does not prevent us from testing the individual DAOs. A lot of the highly specific changes at the implementation level of MetaAlertDao are things that more or less need to be integration tested (e.g. making sure the magic Solr incantations do what's expected). The other stuff can be unit tested regardless of what implementation class it's in.
Re: #832 Absolutely agree we should have as many unit tests as possible to avoid things like this.
The tests are substantially better than they were prior to this PR. I'm happy to take another pass to try to catch out any cases we're missing. The initial pass took place during a refactoring and I wouldn't be surprised if there's more we can do. The problem is that I need to have a stable code base that I'm testing against. That's the main reason I'm trying to get this narrowed down and resolve the overall code structure. I'm not trying to say that we're done adding tests or that there's not room for improvement. I'm trying to get to a point where I'm not trying to build the tests we'd both like to see on a pile of sand. For me, killing that dedupe is a prerequisite because it could change how those unit tests happen in terms of avoiding duplication and ensuring that the variants on the implementations are more easily understood and tested. Right now it's hard to build expectations against the code, because we're changing basic structure and how things are built. Even if
the test cases themselves cover the same thing, a lot of stuff like setup changes so there's nontrivial effort involved in keeping the tests aligned.
IMO (and feel free, or even encouraged, to argue with me on this), killing the interfaces doesn't buy us enough testability to merit adding that refactoring to this PR. I do think rethinking the DAO structure is a definitely a worthwhile task that could improve a lot of things at once, but it should be a separate effort.
I think a reasonable level of testing for this PR is the integration testing at the level of broad interface DAOs + the already substantial increase in testing + maybe another pass once the implementation has stabilized. Let me know if your expectations are different, because it sounds like both of our expectations are influencing how much refactoring we want, so we need to be on the same page there.
Re: the issue on this PR itself, that appears to be an issue with using the correct source.type field for Solr. I'm working on a confirming and providing a separate fix for that. Part of the problem appears to be the unit tests themselves are incorrect. The other problem is that our test schemas incorrectly use ':' instead of '.' and I carried this through. So the problem isn't that the tests are incomplete, but that it's compounding a bug in our tests themselves. In fact, I called out that I thought it was weird we were using ':' in the PR description.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
Given the added fix by @merrimanr, this lgtm. +1
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on the issue:
https://github.com/apache/metron/pull/970
@justinleet sounds reasonable to me
---
[GitHub] metron pull request #970: METRON-1421: Create a SolrMetaAlertDao
Posted by mmiklavc <gi...@git.apache.org>.
Github user mmiklavc commented on a diff in the pull request:
https://github.com/apache/metron/pull/970#discussion_r181186288
--- Diff: metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrSearchDao.java ---
@@ -238,19 +204,19 @@ protected SearchResponse buildSearchResponse(
return searchResponse;
}
- protected SearchResult getSearchResult(SolrDocument solrDocument, Optional<List<String>> fields) {
- SearchResult searchResult = new SearchResult();
- searchResult.setId((String) solrDocument.getFieldValue(Constants.GUID));
- Map<String, Object> source;
- if (fields.isPresent()) {
- source = new HashMap<>();
- fields.get().forEach(field -> source.put(field, solrDocument.getFieldValue(field)));
- } else {
- source = solrDocument.getFieldValueMap();
- }
- searchResult.setSource(source);
- return searchResult;
- }
+// protected SearchResult getSearchResult(SolrDocument solrDocument, Optional<List<String>> fields) {
--- End diff --
Deleted 1 commented line, added 13 more? Haha.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/970
I spun this up in full dev and ran through all the tests in the test plan above. Everything in this plan worked. I also added a step in each test to perform a search on alert fields and verified the metaalert was returned when appropriate (status is active and contained an alert with the matching field). The request is similar to:
```
curl -u user:password -X POST --header 'Content-Type: application/json' --header 'Accept: application/json' -d '{
"fields": [
"guid","ip_dst_addr"
],
"from": 0,
"indices": [
"metaalert"
],
"query": "ip_dst_addr:192.168.66.121",
"size": 10
}' 'http://node1:8082/api/v1/search/search'
```
I did find a few cases that did not work. For the first one, I performed these steps:
- Created a metaalert with a single alert
- Removed the alert from the metaalert
- Added the alert back to the metaalert
After this when I did a findOne on the metaalert, I get a 404. The metaalert still shows up in searches however.
For the next one I performed these steps:
- Created a metaalert from an alert
- Created another metaalert from the same alert
- Removed the alert from the first metaalert
When I try to remove the alert from the second metaalert, I get false and the alert is still in the second metaalert. The second metaalert is also still contained in the metaalerts field of the alert.
These seem like pretty unusual edge cases but I thought I should report them anyways since it could be indicative of a deeper issue.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by nickwallen <gi...@git.apache.org>.
Github user nickwallen commented on the issue:
https://github.com/apache/metron/pull/970
> @justinleet Are you more interested into breaking things out into more classes than that, just breaking apart functions more so they're more easily tested, some combination of both?
> @justinleet Is breaking the meta alert dao into the various sub functions (Search, Update, etc.) + pulling out the calculate logic + at least a refactoring pass for testability a good first step in moving this forward?"
Yes, I like it; that works. I will be happy if we get better unit test coverage on all of this.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc I'm also inclined to work on that fix against master, since it effects current ES metaalerts. Seem reasonable?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen I made a pass at the refactoring. Most things end up in the UpdateDao (which is probabyl to be expected, given that mutation is the main sticking point). Even though it largely ends up in one place, it should still help convey what's going on better.
In addition that DAO now has a set of unit tests with it, let me know what you think.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@mmiklavc
Re: The index issue appears to be preexisting. https://github.com/apache/metron/blob/feature/METRON-1416-upgrade-solr/metron-platform/metron-solr/src/main/java/org/apache/metron/solr/dao/SolrSearchDao.java#L231 . If you're okay with it, I think we make a another ticket against the parent ticket and resolve it separately.
Re: the child alert's metaalerts field. That field doesn't actually do anything and is an artifact of the denormalization process. What happens there is that we just update the status of the metaalert, without updating denormalized alert's metaalert links (which are unused anyway and exist because it's more work and messy to update them on something we've already decided isn't active).
Having said that, this points to another issue further on. Check out https://github.com/apache/metron/pull/970/files#diff-b7359d01c3ffbed48b7fdaa2d32169e7R246. The process of updating from inactive to active is slightly incomplete.
Say we have these three steps:
- Metaalert is updated to inactive
- (Former) child alert is updated.
- Metaalert is made active again.
The update will be missing from the metaalert. We need to update the metaalert with the current state of any alerts (which we conveniently have because we needed to update them all anyway!). This is a problem with both ES and Solr (which shouldn't be surprising since that link is to the abstract DAO. It also needs an associated test case.
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
@nickwallen For a bit of context, I tried primarily to keep things essentially how they were in the ES versions (including the integration tests and unit tests). I definitely appreciate you looking at this, the feedback is definitely helpful, especially since like I said, this is a first cut, that gets things implemented in a very similar manner to the current implementation. Iterating on it is definitely worthwhile, but I think you're right, the feedback is a little too abstract to work with as-is, so let's try to work out a more concrete approach. Here's my initial thoughts, which are also mostly too abstract, but we can try to get more detailed from here.
Re (1):
We could probably do something fairly similar to what we have the other DAO's. Specifically, break things up into SolrMetaAlertUpdateDao + ElasticsearchMetaAlertDao, SolrMetaAlertSearchDao + SolrMetaAlertSearchDao, etc. Then have the SolrMetaAlertDao just wrap the appropriate calls as needed. I think this will help make it more digestible (which is definitely a worthy cause), but I don't think it helps make the implementations more independent. I'd still expect them to be fairly large and share a lot of code in some abstraction (whether that's abstract classes or not). The reason they'd share code is that, unlike the SolrSearchDao vs ElasticsearchSearchDao is that the metaalerts are (mostly) avoiding digging into Solr and ES internals which is definitely not the case for the other DAOs.
Essentially there's a tradeoff between changes in the base classes breaking one or both (but if that happens, I'd argue that our testing is incomplete and we should be adding more tests like you mention) or having them vary independently and having issues that are fixed on one side not fixed in the other (which has it's own set of testing challenges). I'm curious what you think is a good approach to breaking things up beyond that that avoids a lot of code duplication.
For the implementation specific quirks, I completely agree, the abstraction probably isn't as well defined as it could be. it might be possible to refactor some things to make it more well defined, but at some point we're other going to be duplicating code or patching abstraction leaks at the DAO level. Because I don't think Solr and ES (and any other implementation we choose) are fundamentally able to be papered over in a perfect abstraction. I definitely agree it would be nice to make them less tied at the hip, the problem is that a lot of this stuff is already broken up at the abstraction points we have (particularly at Document level, rather than implementation specific level). Right now, the implementation details are obscured from the calling REST service, and its the responsibility of the MetaAlertDao to handle the implementation details.
Re (2):
For the specific calculating metascores case, and probably some of the other stuff, it only seems useful to break that sort of thing into it's own class if we intend on making it pluggable. After all, that is a function that is pretty tied into how metaalerts are laid out and I'm not really sure what the benefit of splitting individual operations out like that is (unless we potentially needed to make them available across composed DAOs). The other thing is the actual calculation of the scores themselves are maintained by a different class. It's only the actual management of the metaalert representation handled by the Dao. I'm
To do more unit testing, there's definitely some easy targets in there. I'd kept the unit tests that existed, but looking back at them, there's definitely more things and helper functions that could be better tested even without splitting things into more classes.
Is it worthwhile to do the refactoring into the *MetaAlertUpdateDao and *UpdateSearchDao et al., along with adding some of the unit tests, and seeing where we are at that point?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by ottobackwards <gi...@git.apache.org>.
Github user ottobackwards commented on the issue:
https://github.com/apache/metron/pull/970
As we are already on a feature branch, can we not iterate and refactor this? Does it all have to go into *this* pr?
If this is landed, what is keeping @nickwallen from throwing a PR at the feature branch?
---
[GitHub] metron issue #970: METRON-1421: Create a SolrMetaAlertDao
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/970
Merged in a PR from @merrimanr that fixed a couple bugs, in particular around committing (which would happen in tests but no on full dev)
---