METRON-1337: List of facets should not be hardcoded #853
Conversation
|
Does ambari own the application.yaml currently? |
|
Each environment will have it's own application.yaml. Full dev has one, our testing environment has one, Ambari ships one etc. There is also a base application.yaml that has the defaults (default facets are included here). Does that make sense? |
|
What I get to was, the setting should be exposed in ambari if ambari manages the config. |
|
Ambari does not manage this config. It is only included in the base application.yml as a default setting. I don't feel like this setting should be in Ambari for a couple reasons: changing it requires a restart and the list of facets cannot be user specific. |
|
@merrimanr Can you deconflict this? |
# Conflicts: # metron-interface/metron-rest/src/test/java/org/apache/metron/rest/controller/SearchControllerIntegrationTest.java
|
Done |
|
To respond to the questions in the description (and maybe kick off conversation, especially if anyone disagrees) + add my own thoughts. In no particular order. @merrimanr Let me know if I'm misunderstanding anything as well, I'm digging into this for the first time
|
|
Here are my thoughts on your responses.
|
|
Just ducking in here, @merrimanr is this ready for review? Specifically, the responses to @justinleet 's questions have all been factored into the current code for this PR, right? If yes, @justinleet would you agree or is there more work to be done to react to your comments? |
|
We're good on 1-4. For point 5. I'd still like to see a note in the upgrading, even if it gets removed when the more flexible store is added. I don't know when we'll get around to using a different store, and if it ends up not being this release, we'll need to add this anyway. I think point 6 still needs to be addressed. |
|
I will address 6 shortly. For 5, should we explore a more flexible store in this PR? Or at least validate that an RDBMS is the right choice? I think this is something we should tackle now as it will likely affect future work as well. Does it make sense to start a discuss thread on this topic? |
|
I don't see anything that should be problematic in ES 5.6.2, but can you confirm @merrimanr? |
|
Bump @merrimanr |
|
The latest commit switches the persistence for storing user settings to HBase rather than a RDBMS as discussed on the dev list. Instead of fields being stored in RDBMS columns, the user settings object is now serialized with Jackson and stored in HBase as a byte[]. This required several changes including:
I also refactored some areas to make things clearer and easier to understand:
There are a couple of design issues to consider when reviewing this. The implementation in the REST app is specific to this use case. I considered trying to make it more generic for future HBase use cases but decided to keep it simple for now. Instead of trying to predict what those use cases look like and choose a pattern that works, I decided to leave that to whoever implements a new use case in the future. I also considered making this generic to all of Metron but again, decided to keep it simple. Should these settings be limited to just REST/UIs? Any thoughts on this? This has been tested in full dev and all tests are passing. In full dev, navigate to the UserController and use the various endpoints to save and retrieve settings for the currently logged in user. You should also be able to log in as the admin user and see all user settings and delete an individual user's settings. I also tested evolving the user settings model by adding new fields and it worked without issue. Users with existing settings just return null for new fields. |
|
First, nice work. Since we will have some unknown number of rest clients, that may want to save 'configuration' associated with a user, and not just Alerts UI, should it not be factored more generically? getUserSettingsForType() And stored as user, type, settings byte[] or some such? We may also want to store the version so that it is user, type, settingsFmtVersion, settings. We might also want to consider storing as Protobufs, which are more supporting changes. |
|
Of course re-reading your comment, I see you specifically didn't account for these, but I think there are some basic tweaks to allow for this that don't presuppose far flung 20% use cases. |
|
I think it's a fair question @ottobackwards. Anything that might affect how the HBase table is laid out should be worked out now or we're back to altering tables during upgrades. Currently the row key is the user name, the column family is hardcoded (although configurable), and the column qualifier is hardcoded. Would you add the type to the row key or store different client settings in different columns? We could add versioning but that is not trivial and will increase the scope of this PR. I can take it on here if we decide we want it but this PR is already large. Follow on maybe? Either way works for me. If we decide a follow on is better I would make an effort to do it right away before we do a release. Jackson is the standard in Metron for serialization/deserialization. I think we should have a very compelling reason to introduce something new. Happy to discuss. |
|
I am not an expert in hbase, I cannot say how i'd implement it. I think the use case is: For any given User ( at least ) we need the ability to store currently known and future unknown configurations as bytes, with configuration type identifier lookup. This should be done such that new configuration types can be added by 'putting' config bytes with a new identifier. I think that gets us a long way. |
mmiklavc
left a comment
mmiklavc
left a comment
There was a problem hiding this comment.
Thanks for the contributions @merrimanr. On the subject of abstractions, what is your recommendation for how we would upgrade when new use cases are added? I ask because the primary driver for us choosing HBase was to avoid drama around upgrades, but by not baking in the ability to specify multiple config types as @ottobackwards said up front I fear we will still have that problem. Would you be ok with adding this - I think it would be minimal additional work, but save us quite a bit of extra effort later.
| self.__params.hbase_principal_name, | ||
| execute_user=self.__params.hbase_user) | ||
|
|
||
| cmd = "echo \"grant '{0}', 'RW', '{1}'\" | hbase shell -n" |
There was a problem hiding this comment.
Do we check that the REST API has the hbase client also installed on the node? This would be needed for this to work for > 1 nodes.
There was a problem hiding this comment.
Good catch. Done.
| public Optional<List<String>> getFacetFields() { | ||
| return facetFields == null || facetFields.size() == 0 ? Optional.empty() : Optional.of(facetFields); | ||
| public List<String> getFacetFields() { | ||
| return facetFields; |
There was a problem hiding this comment.
If we're going to change the interface, what about
return facetFields == null ? new ArrayList() : facetFields;
so it's not null.
There was a problem hiding this comment.
Because null and an empty array mean 2 different things. See point 6 here: #853 (comment).
| import static org.apache.metron.rest.repository.UserSettingsRepository.USER_SETTINGS_HBASE_TABLE; | ||
|
|
||
| @Configuration | ||
| @Profile("!" + TEST_PROFILE) |
There was a problem hiding this comment.
We don't want to use this Configuration if tests are running. There is a separate TestConfig that setups up mock, inmemory components, etc.
|
I took @ottobackwards's suggestion and added a "type" parameter that can be used to independently manage user settings for different types of clients. I moved the users settings for the Alerts UI (the original intention of this PR) back to the AlertController. I also went ahead and moved the low level client code to the metron-hbase module so that this user settings abstraction can be used anywhere in Metron. For examples of how to setup and use the client see HBaseConfig and AlertServiceImpl in the metron-rest module. Testing in full dev is slightly different now. Instead of the endpoints being in the UserController they should now be tested in the AlertController. The Alerts UI should also be tested. Navigate to the Alerts UI and you should see the default facets on the left. Update the "facetFields" user setting with Swagger and the new list should appear in the Alerts UI after refreshing. For example, if you set facetFields = ip_src_addr, only that facet field should appear in the Alerts UI. |
| */ | ||
| @Service | ||
| public class AlertServiceImpl implements AlertService { | ||
|
|
There was a problem hiding this comment.
can we make the type metron_alert? alert seems a little generic
There was a problem hiding this comment.
Sure I can do that. Since these settings are specific to the Alerts UI, should we include "ui" too? Should the REST service also follow a similar pattern?
There was a problem hiding this comment.
That seems reasonable
There was a problem hiding this comment.
Name is changed in the latest commit. Let me know what you think.
There was a problem hiding this comment.
I think that is great. At some point, if we are going to do this a few times we should have a standard naming convention ( separators at least or something). But that isn't a thing for this PR
|
This looks great, one small comment from review |
|
@merrimanr this looks much better. I'm +1 via inspection pending any further feedback from @ottobackwards. |
|
Any other feedback @ottobackwards or is this ready to go? |
|
Sorry, I'll try to get back to this today |
|
+1 by inspection |
5 participants
Contributor Comments
This PR makes the list of facet fields in the Alerts UI configurable. There is now a "search.facet.fields" setting in https://github.com/apache/metron/blob/master/metron-interface/metron-rest/src/main/resources/application.yml that is a comma-separate list of fields to be used as facets. Originally a comment was made that the "host" field wasn't commonly used so I removed that from the default list.
I can think of two options for exposing this configuration:
I chose to include the first option in this PR to get the conversation going. Is one of these preferable? The AlertProfile approach allows this setting to be changed at runtime and each user has their own list of facet fields. But it is not versioned like it would if it were in Ambari. Do we prefer one over the other? Do we want both with Ambari being the default when an AlertProfile doesn't exist for a user? Are there other options I'm not thinking of?
This works similar to how default search indices work: it is managed in the REST layer and can be overriden by including facet fields in a search request. However it seemed useful to allow a way to explicitly NOT include facets in a query so it works slightly different than indices. A missing facetFields property in the request will use the defaults while an empty array will disable facets. A missing indices property or an empty array will use the default indices. Is this the correct behavior?
This has been tested in full dev and the UI e2e tests pass when run in isolation. There is currently an effort to stabilize the e2e tests as a follow on to #803 so I did not try to solve that here.
I will add some documentation around configuring the facet field list and facetFields behavior in a search request once we come to a consensus on the solution.
Another issue I would like to point out. When I added facetFields to the AlertProfile object it required a database update because that new field needed to be added to the database. This would become an issue if someone were upgrading from a previous version. Is this acceptable if we document it for future upgrades? Is a relational database the right solution or should we consider a more flexible storage option?
Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our Development Guidelines for the complete guide to follow for contributions.
Please refer also to our Build Verification Guidelines for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
For all changes:
For code changes:
Have you included steps to reproduce the behavior or problem that is being changed or addressed?
Have you included steps or a guide to how the change may be verified and tested manually?
Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
Have you written or updated unit tests and or integration tests to verify your changes?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
For documentation related changes:
Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via
site-book/target/site/index.html:Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that travis-ci is set up for your personal repository such that your branches are built there before submitting a pull request.