You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@metron.apache.org by sardell <gi...@git.apache.org> on 2018/05/11 13:46:43 UTC
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
GitHub user sardell opened a pull request:
https://github.com/apache/metron/pull/1010
METRON-1548: Remove hardcoded source:type from Alerts UI
## Contributor Comments
Instead of 'source:type' being hard coded as a string in the Alerts UI, we now use a global-config service to retrieve whether a user's setup is using "source:type" or "source.type" syntax (e.g. elastic search or solr). The app default's to using "source:type" if no configuration is specified. If a user wants, they can update their configuration to use source.type by adding `"sourceType": "source.type"` to their configuration.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [ ] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sardell/metron METRON-1548
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1010.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 #1010
----
commit 1f6edee9a8a9a2b83072408a5089f1b95b35bb4b
Author: Shane Ardell <sa...@...>
Date: 2018-05-07T11:19:42Z
remove hard-coded source:type and replace with env variable
commit c37d23988a6466256512c7417d855fc15af0b065
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T12:20:51Z
get config from API instead of a environment file
commit be59b8e7e8f5abfd1ab3edf21658695e4cd4e46e
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:32:56Z
remove unused facet and group constants
commit ec68878b96d8831e03299d19c8daf7cef7f3104b
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:34:41Z
use global config to set source type in update service
commit e8935077f779cbffcc35400014cf24d6bf0e2de5
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:37:23Z
remove sourceType var from environment file
----
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by sardell <gi...@git.apache.org>.
Github user sardell commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r189261776
--- Diff: metron-interface/metron-alerts/src/app/alerts/alerts-list/table-view/table-view.component.ts ---
@@ -77,12 +80,24 @@ export class TableViewComponent implements OnChanges {
searchService: SearchService,
metronDialogBox: MetronDialogBox,
updateService: UpdateService,
- metaAlertService: MetaAlertService) {
+ metaAlertService: MetaAlertService,
+ globalConfigService: GlobalConfigService) {
this.router = router;
this.searchService = searchService;
this.metronDialogBox = metronDialogBox;
this.updateService = updateService;
this.metaAlertService = metaAlertService;
+ this.globalConfigService = globalConfigService;
+ }
+
+ ngOnInit() {
+ this.globalConfigService.get().subscribe((config: {}) => {
+ this.globalConfig = config;
+ if (this.globalConfig['source.type.field'] === 'source.type' && !this.alertsColumnsToDisplay['source.type']) {
--- End diff --
@merrimanr Good point. What if we check if a config property of ['source.type.field'] exists and change it to whatever value it has. If no config exists, default to `source:type` (to catch existing users without a config property set)?
```
if (this.globalConfig['source.type.field']) {
this.alertsColumnsToDisplay.splice(2, 0, new ColumnMetadata(config['source.type.field'], 'string'));
}
else {
this.alertsColumnsToDisplay.splice(2, 0, new ColumnMetadata(config['source:type'], 'string'));
}
```
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r189060226
--- Diff: metron-interface/metron-alerts/src/app/alerts/alerts-list/table-view/table-view.component.ts ---
@@ -77,12 +80,24 @@ export class TableViewComponent implements OnChanges {
searchService: SearchService,
metronDialogBox: MetronDialogBox,
updateService: UpdateService,
- metaAlertService: MetaAlertService) {
+ metaAlertService: MetaAlertService,
+ globalConfigService: GlobalConfigService) {
this.router = router;
this.searchService = searchService;
this.metronDialogBox = metronDialogBox;
this.updateService = updateService;
this.metaAlertService = metaAlertService;
+ this.globalConfigService = globalConfigService;
+ }
+
+ ngOnInit() {
+ this.globalConfigService.get().subscribe((config: {}) => {
+ this.globalConfig = config;
+ if (this.globalConfig['source.type.field'] === 'source.type' && !this.alertsColumnsToDisplay['source.type']) {
--- End diff --
I don't think this logic is correct. What happens if you have "source.type" selected as a table column? I went in a updated by local storage directly and ended up with 2 "source.type" columns.
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1010
+1 pending travis
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
I spun this up, changed the config, restarted the UI and had a couple comments.
1. Every time you log in, it keeps adding new source.type fields in the table. Unselecting these in the options doesn't work properly (only the top one toggles no matter which I click on). Possibly doubles every time?
2. The filters on the left still say "source:type", even after the config is changed.
3. The group by still says "source:type", even after the config is changed. This grouping still works fine (which I would expect if it's still grouping on source:type).
I would expect both the filter and group by to act on the configured `source.type.field` by default. Otherwise, the user has to know what's going on and change it manually.
I'd also like to add a bit in the documentation that updating the `source.type.field` requires a UI restart. This is pretty typical of the UI, but most global config things get picked up automagically (e.g. topology changes and such), so a quick blurb would be nice.
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by sardell <gi...@git.apache.org>.
Github user sardell closed the pull request at:
https://github.com/apache/metron/pull/1010
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
I think the general ES migration to source.type is outside the scope of this PR. That would have to be part of a larger migration anyway since you'd have to reindex data on an existing metron cluster to do that. I'm sure we'll want to think about it at some point, though.
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
@merrimanr After thinking about this a bit, is that actually a serious problem? This setting is likely to only be done on initial install (e.g. when selecting Solr or ES). Switching midlife is unlikely to be a problem for the vast majority of users. Most users should never see the problem of it being switched because they'd never see `source:type` instead of `source.type` or vice versa, because you'd have to reindex all your old data into the new format anyway. We'd need to improve documentation in the Solr feature branch to ensure that users initially set it up appropriately.
Another way of going about this might be to have the app itself get config updates on the fly similar to how our topologies do, given that this particular one is from ZK. At that point, you could do the mapping from old source type to new source type and update the storage appropriately. I'm sure there's way more to it than just doing that though (when do you update UI for users, what are the potential snags in updating that when users are querying, etc.).
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r189065371
--- Diff: metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts ---
@@ -174,6 +177,13 @@ export class AlertsListComponent implements OnInit, OnDestroy {
}
ngOnInit() {
+ this.globalConfigService.get().subscribe((config: {}) => {
+ this.globalConfig = config;
+ if (this.globalConfig['source.type.field'] === 'source.type' && !this.alertsColumns['source.type']) {
--- End diff --
Same as https://github.com/apache/metron/pull/1010/files#r189060226
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
Also, UI for the subsytem should be fine. It's purely informational. If you wanted to be more explicit, you could put "Alerts UI" or something similar, but I think just "UI" is fine.
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1010
Most of this worked, I could only find one issue. We store a user's selected columns in local storage right now. These columns are selected in the Configure Table view and the list of available columns comes from a REST call that returns ES (or Solr) metadata. The available column list will always be correct since that information is coming straight from the index templates or schemas but the column name in local storage will become incorrect since it's still the old source type.
There is an attempt to just transform the name in the selected column list but I think this is flawed (I added an inline comment on that). I am not sure what the best way to handle this is. I feel like if we find ourselves hardcoding either "source:type" or "source.type" we're doing something wrong.
One idea is to limit the source.type.field property to be either "source:type" or "source.type". This would at least allow us to perform those hardcoded checks to see if we need to substitute the new value. Otherwise we'll need to evolve our local storage model by using boolean fields like "includeSourceType" instead of the actual source.type.field value. This would make upgrading harder though.
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on the issue:
https://github.com/apache/metron/pull/1010
@justinleet I agree with you that it's not a serious problem. I think it can be solved more easily with documentation and some manual steps from the user. There is some logic we need to fix, specifically around substituting the source.type.field value for "source:type" (I added comments for those). Other than that, I'm good with documenting how to switch to Solr in the Solr feature branch.
There is also the case of upgrading ES and using "source.type" since that name is allowed in later versions. Is that something we need to think about and document?
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by sardell <gi...@git.apache.org>.
Github user sardell commented on the issue:
https://github.com/apache/metron/pull/1010
@justinleet I've changed the property to be `source.type.field` and added the property to the documentation at https://github.com/apache/metron/tree/master/metron-platform/metron-common#global-configuration. I was slightly confused about the subset field, but I took an educated guess and entered `UI` as the value. Let me know if this is not correct and I'll update it.
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/metron/pull/1010
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r188086075
--- Diff: metron-interface/metron-alerts/src/app/service/global-config.service.ts ---
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import {Injectable, Inject} from '@angular/core';
+import {Http, Headers, RequestOptions, Response, ResponseOptions} from '@angular/http';
+import {Observable} from 'rxjs/Observable';
+import {HttpUtil} from '../utils/httpUtil';
+
+@Injectable()
+export class GlobalConfigService {
+ url = 'api/v1/global/config';
+ defaultHeaders = {'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest'};
+
+ private globalConfig = {};
+
+ constructor(private http: Http) {
--- End diff --
I think this constructor was carried over from metron-config and the property assignments are not needed. I think it should just be empty.
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by merrimanr <gi...@git.apache.org>.
Github user merrimanr commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r189066511
--- Diff: metron-interface/metron-alerts/src/app/service/elasticsearch-localstorage-impl.ts ---
@@ -47,6 +51,18 @@ export class ElasticSearchLocalstorageImpl extends DataSource {
new ColumnMetadata('alert_status', 'string')
];
+ getSourceTypeFromLocalstorage() {
+ let sourceType: string;
+ if (localStorage['metron-alerts-saved-search']) {
+ let columnHeaders = JSON.parse(localStorage['metron-alerts-saved-search']);
+ sourceType = columnHeaders[0]['tableColumns'].filter(colName => colName.name === 'source.type');
+ if (sourceType && !this.defaultColumnMetadata['source.type']) {
+ this.defaultColumnMetadata = this.defaultColumnMetadata.filter(colName => colName.name !== 'source:type');
--- End diff --
Same as https://github.com/apache/metron/pull/1010/files#r189060226
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
After the fix for the table header bug, I wasn't able to repeat it.
Also, my PR was merged into here to have for the REST API to pull the "source.type.field" from the global config to ensure numbers 2 and 3 are taken care of. I'm going to refrain from voting on this PR given that contribution.
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by sardell <gi...@git.apache.org>.
Github user sardell commented on a diff in the pull request:
https://github.com/apache/metron/pull/1010#discussion_r188243982
--- Diff: metron-interface/metron-alerts/src/app/service/global-config.service.ts ---
@@ -0,0 +1,88 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+import {Injectable, Inject} from '@angular/core';
+import {Http, Headers, RequestOptions, Response, ResponseOptions} from '@angular/http';
+import {Observable} from 'rxjs/Observable';
+import {HttpUtil} from '../utils/httpUtil';
+
+@Injectable()
+export class GlobalConfigService {
+ url = 'api/v1/global/config';
+ defaultHeaders = {'Content-Type': 'application/json', 'X-Requested-With': 'XMLHttpRequest'};
+
+ private globalConfig = {};
+
+ constructor(private http: Http) {
--- End diff --
Thanks for pointing this out, @merrimanr. Most of the file was carried over from metron-config, and after taking another look I agree that the constructor should be empty. I'm also going to remove the post and delete methods, as they are not used in metron-alerts currently.
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
Thanks for the contribution, getting this cleaned up is really helpful!
Could you update https://github.com/apache/metron/tree/master/metron-platform/metron-common#global-configuration with the new config property?
I'd also like to see the property be something like `source.type.field` to be a bit more explanatory and consistent with the other non-Object type fields.
---
[GitHub] metron pull request #1010: METRON-1548: Remove hardcoded source:type from Al...
Posted by sardell <gi...@git.apache.org>.
GitHub user sardell reopened a pull request:
https://github.com/apache/metron/pull/1010
METRON-1548: Remove hardcoded source:type from Alerts UI
## Contributor Comments
Instead of 'source:type' being hard coded as a string in the Alerts UI, we now use a global-config service to retrieve whether a user's setup is using "source:type" or "source.type" syntax (e.g. elastic search or solr). The app default's to using "source:type" if no configuration is specified. If a user wants, they can update their configuration to use source.type by adding `"sourceType": "source.type"` to their configuration.
## Pull Request Checklist
Thank you for submitting a contribution to Apache Metron.
Please refer to our [Development Guidelines](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=61332235) for the complete guide to follow for contributions.
Please refer also to our [Build Verification Guidelines](https://cwiki.apache.org/confluence/display/METRON/Verifying+Builds?show-miniview) for complete smoke testing guides.
In order to streamline the review of the contribution we ask you follow these guidelines and ask you to double check the following:
### For all changes:
- [x] Is there a JIRA ticket associated with this PR? If not one needs to be created at [Metron Jira](https://issues.apache.org/jira/browse/METRON/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel).
- [x] Does your PR title start with METRON-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [x] Has your PR been rebased against the latest commit within the target branch (typically master)?
### For code changes:
- [ ] Have you included steps to reproduce the behavior or problem that is being changed or addressed?
- [ ] Have you included steps or a guide to how the change may be verified and tested manually?
- [ ] Have you ensured that the full suite of tests and checks have been executed in the root metron folder via:
```
mvn -q clean integration-test install && dev-utilities/build-utils/verify_licenses.sh
```
- [ ] Have you written or updated unit tests and or integration tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] Have you verified the basic functionality of the build by building and running locally with Vagrant full-dev environment or the equivalent?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered by building and verifying the site-book? If not then run the following commands and the verify changes via `site-book/target/site/index.html`:
```
cd site-book
mvn site
```
#### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
It is also recommended that [travis-ci](https://travis-ci.org) is set up for your personal repository such that your branches are built there before submitting a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sardell/metron METRON-1548
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/metron/pull/1010.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 #1010
----
commit 1f6edee9a8a9a2b83072408a5089f1b95b35bb4b
Author: Shane Ardell <sa...@...>
Date: 2018-05-07T11:19:42Z
remove hard-coded source:type and replace with env variable
commit c37d23988a6466256512c7417d855fc15af0b065
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T12:20:51Z
get config from API instead of a environment file
commit be59b8e7e8f5abfd1ab3edf21658695e4cd4e46e
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:32:56Z
remove unused facet and group constants
commit ec68878b96d8831e03299d19c8daf7cef7f3104b
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:34:41Z
use global config to set source type in update service
commit ea01e5d6eaf826b6314eb6472eff8017c203f79f
Author: Shane Ardell <sa...@...>
Date: 2018-05-11T13:37:23Z
remove sourceType var from environment file
commit 7e6147a43e0794e537412576fb5d4316f7747e7d
Author: Shane Ardell <sa...@...>
Date: 2018-05-15T09:45:41Z
cleanup unused property assignments and methods from global config service
commit 65c50126d94ff7a69289429559a2043c3577a4eb
Author: Shane Ardell <sa...@...>
Date: 2018-05-15T09:46:35Z
update sourceType property to source.type.field
commit 89be803bc46af2d2958672e3b7a2162e9083dc0d
Author: Shane Ardell <sa...@...>
Date: 2018-05-15T10:45:47Z
update documentation to include source.type.field config property
----
---
[GitHub] metron issue #1010: METRON-1548: Remove hardcoded source:type from Alerts UI
Posted by justinleet <gi...@git.apache.org>.
Github user justinleet commented on the issue:
https://github.com/apache/metron/pull/1010
@sardell Looks like Travis failed for unrelated reasons (which I'm curious about on it's own). Could you just close and reopen the PR to kick Travis off again?
---