[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327094011
##
File path: metron-interface/metron-alerts/src/app/utils/httpUtil.ts
##
@@ -33,9 +33,12 @@ export class HttpUtil {
return body || {};
}
+ /**
+ * @depricated Turning all errors to 404 and hiding actual errors from the
consumers
Review comment:
Fixed. Thanks
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327021709
##
File path: metron-interface/metron-alerts/src/app/utils/httpUtil.ts
##
@@ -33,9 +33,12 @@ export class HttpUtil {
return body || {};
}
+ /**
+ * @depricated Turning all errors to 404 and hiding actual errors from the
consumers
+ * could limit how we can recover or react to errors.
+ * Use sessionExpiration instead and/or introduce new composable
error handlers.
+ */
public static handleError(res: HttpErrorResponse): Observable {
-// In a real world app, we might use a remote logging infrastructure
-// We'd also dig deeper into the error to get a better message
Review comment:
If we once accepted to have comments like this in the code using // FIXME is
also acceptable I think. :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327021709
##
File path: metron-interface/metron-alerts/src/app/utils/httpUtil.ts
##
@@ -33,9 +33,12 @@ export class HttpUtil {
return body || {};
}
+ /**
+ * @depricated Turning all errors to 404 and hiding actual errors from the
consumers
+ * could limit how we can recover or react to errors.
+ * Use sessionExpiration instead and/or introduce new composable
error handlers.
+ */
public static handleError(res: HttpErrorResponse): Observable {
-// In a real world app, we might use a remote logging infrastructure
-// We'd also dig deeper into the error to get a better message
Review comment:
If we once accepted to have comments like this in the code using // FIXME is
also acceptable I think. :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327004638
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -311,16 +344,12 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
prepareColumnData(configuredColumns: ColumnMetadata[], defaultColumns:
ColumnMetadata[]) {
this.alertsColumns = (configuredColumns && configuredColumns.length > 0) ?
configuredColumns : defaultColumns;
-this.queryBuilder.setFields(this.getColumnNamesForQuery());
this.calcColumnsToDisplay();
}
prepareData(tableMetaData: TableMetadata, defaultColumns: ColumnMetadata[]) {
-this.tableMetaData = tableMetaData;
-this.refreshInterval = this.tableMetaData.refreshInterval;
-
-this.updateConfigRowsSettings();
Review comment:
Logic moved to auto-polling.service
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327004408
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -373,11 +402,6 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
}
}
- restoreRefreshState() {
-this.isRefreshPaused = this.lastIsRefreshPausedValue;
-this.tryStartPolling();
- }
Review comment:
Logic moved to auto-polling.service
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327004808
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -282,15 +324,6 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
this.search();
}
- onPausePlay() {
-this.isRefreshPaused = !this.isRefreshPaused;
-if (this.isRefreshPaused) {
- this.tryStopPolling();
-} else {
- this.search(false);
-}
- }
-
Review comment:
Logic moved to auto-polling.service and component.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327001281
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -226,8 +259,6 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
onClear() {
this.timeStampFilterPresent = false;
this.queryBuilder.clearSearch();
-if (this.hideQueryBuilder) { this.manualQuery.nativeElement.value = '*'; }
Review comment:
Source of manual query string is no longer the native dom element. I moved
it to query builder to eliminate the two posibble source of query. Logic
belongs to query creations is inside query builder to encapsulate and not make
alerts-list more bloated.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327021709
##
File path: metron-interface/metron-alerts/src/app/utils/httpUtil.ts
##
@@ -33,9 +33,12 @@ export class HttpUtil {
return body || {};
}
+ /**
+ * @depricated Turning all errors to 404 and hiding actual errors from the
consumers
+ * could limit how we can recover or react to errors.
+ * Use sessionExpiration instead and/or introduce new composable
error handlers.
+ */
public static handleError(res: HttpErrorResponse): Observable {
-// In a real world app, we might use a remote logging infrastructure
-// We'd also dig deeper into the error to get a better message
Review comment:
If we once accepted to have comments like this in the code using // FIXME is
also acceptable. :)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests URL: https://github.com/apache/metron/pull/1514#discussion_r326992592 ## File path: metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.html ## @@ -84,14 +92,20 @@ - + - - Data is in a stale state! Click - to update your view based on your current filter and time-range configuration! - Review comment: The data state related warning messages was refactored to functions. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327005089
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -460,69 +472,30 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
}
showConfigureTable() {
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
this.router.navigateByUrl('/alerts-list(dialog:configure-table)');
}
showDetails(alert: Alert) {
this.selectedAlerts = [];
this.selectedAlerts = [alert];
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
let sourceType = alert.source[this.globalConfig['source.type.field']];
let url = '/alerts-list(dialog:details/' + sourceType + '/' +
alert.source.guid + '/' + alert.index + ')';
this.router.navigateByUrl(url);
}
- saveRefreshState() {
-this.lastIsRefreshPausedValue = this.isRefreshPaused;
-this.tryStopPolling();
- }
-
- pause() {
-this.isRefreshPaused = true;
-this.tryStopPolling();
- }
-
- resume() {
-this.isRefreshPaused = false;
-this.tryStartPolling();
- }
-
showSavedSearches() {
Review comment:
Moved to auto-polling.service/component.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326999623
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -170,7 +185,7 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
}
getAlertColumnNames(resetPaginationForSearch: boolean) {
-observableForkJoin(
+forkJoin(
Review comment:
In some places in the code we just renamed standard rxjs operators. Don't
think too much explanation needed here. I changed the code to use the standard
names.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327009938
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/auto-polling/auto-polling.service.ts
##
@@ -0,0 +1,184 @@
+/**
+ * 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 } from '@angular/core';
+import { Subscription, Subject, Observable, interval, onErrorResumeNext } from
'rxjs';
+import { SearchService } from 'app/service/search.service';
+import { QueryBuilder } from '../query-builder';
+import { SearchResponse } from 'app/model/search-response';
+import { switchMap, filter, takeWhile, tap } from 'rxjs/operators';
+import { POLLING_DEFAULT_STATE } from 'app/utils/constants';
+import { RestError } from 'app/model/rest-error';
+import { DialogType } from 'app/shared/metron-dialog/metron-dialog.component';
+import { DialogService } from 'app/service/dialog.service';
+
+interface AutoPollingStateModel {
+ isActive: boolean,
+ refreshInterval: number,
+}
+
+@Injectable()
+export class AutoPollingService {
Review comment:
Auto polling logic was previously distributed across alerts-list,
query-builder, and search.service.
I was refactored them out and encapsulated in a dedicated service.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326994513
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.spec.ts
##
@@ -28,53 +28,136 @@ import { SaveSearchService } from
'app/service/save-search.service';
import { MetaAlertService } from 'app/service/meta-alert.service';
import { GlobalConfigService } from 'app/service/global-config.service';
import { DialogService } from 'app/service/dialog.service';
-import { SearchRequest } from 'app/model/search-request';
-import { Observable, of, Subject } from 'rxjs';
-import { Filter } from 'app/model/filter';
-import { QueryBuilder } from './query-builder';
import { TIMESTAMP_FIELD_NAME } from 'app/utils/constants';
-import { SearchResponse } from 'app/model/search-response';
import { By } from '@angular/platform-browser';
+import { Observable, Subject, of, throwError } from 'rxjs';
+import { Filter } from 'app/model/filter';
+import { QueryBuilder, FilteringMode } from './query-builder';
+import { SearchResponse } from 'app/model/search-response';
+import { AutoPollingService } from './auto-polling/auto-polling.service';
+import { Router } from '@angular/router';
+import { Alert } from 'app/model/alert';
+import { AlertSource } from 'app/model/alert-source';
+import { SearchRequest } from 'app/model/search-request';
+import { query } from '@angular/core/src/render3';
+import { RestError } from 'app/model/rest-error';
+import { DialogType } from 'app/shared/metron-dialog/metron-dialog.component';
+
+@Component({
+ selector: 'app-auto-polling',
+ template: '',
+})
+class MockAutoPollingComponent {}
+
+@Component({
+ selector: 'app-configure-rows',
+ template: '',
+})
+class MockConfigureRowsComponent {
+ @Input() refreshInterval = 0;
+ @Input() srcElement = {};
+ @Input() pageSize = 0;
+}
+
+@Component({
+ selector: 'app-modal-loading-indicator',
+ template: '',
+})
+class MockModalLoadingIndicatorComponent {
+ @Input() show = false;
+}
+
+@Component({
+ selector: 'app-time-range',
+ template: '',
+})
+class MockTimeRangeComponent {
+ @Input() disabled = false;
+ @Input() selectedTimeRange = {};
+}
+
+@Directive({
+ selector: '[appAceEditor]',
+})
+class MockAceEditorDirective {
+ @Input() text = '';
+}
+
+@Component({
+ selector: 'app-alert-filters',
+ template: '',
+})
+class MockAlertFilterComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-group-by',
+ template: '',
+})
+class MockGroupByComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-table-view',
+ template: '',
+})
+class MockTableViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+}
+
+@Component({
+ selector: 'app-tree-view',
+ template: '',
+})
+class MockTreeViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+ @Input() globalConfig = {};
+ @Input() query = '';
+ @Input() groups = [];
+}
+
Review comment:
With mocking out all the visual sub components instead of importing the
actual ones the test become faster.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327008834
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -549,24 +522,52 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
this.cdRef.detectChanges();
}
- toggleQueryBuilder() {
+ getStaleDataWarning() {
+if (this.autoPollingSvc.getIsPollingActive()) {
+ return ` Data is in a
stale state!
+Click to update your
view based
+on your current filter and time-range configuration!`;
+} else {
+ return ` Data is in a
stale state!
+Automatic refresh is turned on. Your filter and/or time-range changes
will apply automatically on next refresh.`;
+}
+ }
+
+ getPollingCongestionWarning() {
+return ` Refresh interval
is shorter than the response time.
+ Please increase the refresh interval in the menu above,
+ or try to simplify your query filter.`;
+ }
+
+ private updatePollingInterval(refreshInterval: number): void {
+this.autoPollingSvc.setInterval(refreshInterval);
+ }
+
+ private restoreAutoPollingState() {
+if (this.autoPollingSvc.getIsPollingActive()) {
+ this.autoPollingSvc.setSuppression(false);
+}
+ }
+
+ isQueryBuilderModeManual() {
+return this.queryBuilder.getFilteringMode() === FilteringMode.MANUAL;
+ }
+
+ toggleQueryBuilderMode() {
+// FIXME setting timerange on toggle feels like a hack
this.setSelectedTimeRange([this.selectedTimeRange]);
-if (!this.hideQueryBuilder) {
- this.hideQueryBuilder = true;
- this.manualQuery.nativeElement.value = this.queryBuilder.query;
+if (this.queryBuilder.getFilteringMode() === FilteringMode.BUILDER) {
+ this.queryBuilder.setFilteringMode(FilteringMode.MANUAL);
} else {
- this.hideQueryBuilder = false;
+ this.queryBuilder.setFilteringMode(FilteringMode.BUILDER);
+ // FIXME: this could lead to a large blocking load depending on the
response time
this.queryBuilder.clearSearch();
this.search();
}
}
queryForTreeView() {
-if (!this.hideQueryBuilder) {
- return this.queryBuilder.generateSelect();
-} else {
- return this.manualQuery.nativeElement.value;
-}
Review comment:
This is a good example when we having two separated source of the same
value. Merged into one. The logic encapsulated in query builder and the
difference between manual and built query in no longer exposed.
(Query builder is never hidden or turned off only the filter bar changes
depending on this flag.)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326996126
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.spec.ts
##
@@ -28,53 +28,136 @@ import { SaveSearchService } from
'app/service/save-search.service';
import { MetaAlertService } from 'app/service/meta-alert.service';
import { GlobalConfigService } from 'app/service/global-config.service';
import { DialogService } from 'app/service/dialog.service';
-import { SearchRequest } from 'app/model/search-request';
-import { Observable, of, Subject } from 'rxjs';
-import { Filter } from 'app/model/filter';
-import { QueryBuilder } from './query-builder';
import { TIMESTAMP_FIELD_NAME } from 'app/utils/constants';
-import { SearchResponse } from 'app/model/search-response';
import { By } from '@angular/platform-browser';
+import { Observable, Subject, of, throwError } from 'rxjs';
+import { Filter } from 'app/model/filter';
+import { QueryBuilder, FilteringMode } from './query-builder';
+import { SearchResponse } from 'app/model/search-response';
+import { AutoPollingService } from './auto-polling/auto-polling.service';
+import { Router } from '@angular/router';
+import { Alert } from 'app/model/alert';
+import { AlertSource } from 'app/model/alert-source';
+import { SearchRequest } from 'app/model/search-request';
+import { query } from '@angular/core/src/render3';
+import { RestError } from 'app/model/rest-error';
+import { DialogType } from 'app/shared/metron-dialog/metron-dialog.component';
+
+@Component({
+ selector: 'app-auto-polling',
+ template: '',
+})
+class MockAutoPollingComponent {}
+
+@Component({
+ selector: 'app-configure-rows',
+ template: '',
+})
+class MockConfigureRowsComponent {
+ @Input() refreshInterval = 0;
+ @Input() srcElement = {};
+ @Input() pageSize = 0;
+}
+
+@Component({
+ selector: 'app-modal-loading-indicator',
+ template: '',
+})
+class MockModalLoadingIndicatorComponent {
+ @Input() show = false;
+}
+
+@Component({
+ selector: 'app-time-range',
+ template: '',
+})
+class MockTimeRangeComponent {
+ @Input() disabled = false;
+ @Input() selectedTimeRange = {};
+}
+
+@Directive({
+ selector: '[appAceEditor]',
+})
+class MockAceEditorDirective {
+ @Input() text = '';
+}
+
+@Component({
+ selector: 'app-alert-filters',
+ template: '',
+})
+class MockAlertFilterComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-group-by',
+ template: '',
+})
+class MockGroupByComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-table-view',
+ template: '',
+})
+class MockTableViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+}
+
+@Component({
+ selector: 'app-tree-view',
+ template: '',
+})
+class MockTreeViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+ @Input() globalConfig = {};
+ @Input() query = '';
+ @Input() groups = [];
+}
+
describe('AlertsListComponent', () => {
let component: AlertsListComponent;
let fixture: ComponentFixture;
- let searchServiceStub = {
-search() { return of({
- total: 0,
- groupedBy: '',
- results: [],
- facetCounts: [],
- groups: []
-}) },
-pollSearch() { return of({}) }
- }
- let queryBuilderStub = {
-addOrUpdateFilter() { return {} },
-clearSearch() { return {} },
-generateSelect() { return '*' },
-isTimeStampFieldPresent() { return {} },
-filters: [{}],
-searchRequest: {
- from: 0
-}
- }
let queryBuilder: QueryBuilder;
let searchService: SearchService;
beforeEach(async(() => {
+
+const searchResponseFake = new SearchResponse();
+searchResponseFake.facetCounts = {};
+
TestBed.configureTestingModule({
- schemas: [ NO_ERRORS_SCHEMA ],
Review comment:
NO_ERROR_SCHEMA is no longer needed bc I defined mocks for all sub component.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327018486
##
File path:
metron-interface/metron-alerts/src/app/alerts/configure-rows/show-hide/show-hide-alert-entries.component.spec.ts
##
@@ -104,29 +104,26 @@ describe('ShowHideAlertEntriesComponent', () => {
expect(component.onVisibilityChanged).toHaveBeenCalledWith('DISMISS',
false);
});
- it('should trigger changed event on any toggle changes', () => {
+ it('should trigger changed event on any toggle changes and propagate state',
() => {
+const serviceSpy = TestBed.get(ShowHideService);
spyOn(component.changed, 'emit');
fixture.detectChanges();
-
fixture.debugElement.query(By.css('[data-qe-id="hideDismissedAlertsToggle"]
input')).nativeElement.click();
-fixture.detectChanges();
-
-expect((component.changed.emit as Spy).calls.argsFor(0)[0]).toEqual(new
ShowHideChanged('DISMISS', true));
-
-fixture.debugElement.query(By.css('[data-qe-id="hideResolvedAlertsToggle"]
input')).nativeElement.click();
-fixture.detectChanges();
-
-expect((component.changed.emit as Spy).calls.argsFor(1)[0]).toEqual(new
ShowHideChanged('RESOLVE', true));
Review comment:
The content of the event is changed to what you see in line 118 and 126.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327005220
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -460,69 +472,30 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
}
showConfigureTable() {
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
this.router.navigateByUrl('/alerts-list(dialog:configure-table)');
}
showDetails(alert: Alert) {
this.selectedAlerts = [];
this.selectedAlerts = [alert];
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
let sourceType = alert.source[this.globalConfig['source.type.field']];
let url = '/alerts-list(dialog:details/' + sourceType + '/' +
alert.source.guid + '/' + alert.index + ')';
this.router.navigateByUrl(url);
}
- saveRefreshState() {
-this.lastIsRefreshPausedValue = this.isRefreshPaused;
-this.tryStopPolling();
- }
-
- pause() {
-this.isRefreshPaused = true;
-this.tryStopPolling();
- }
-
- resume() {
-this.isRefreshPaused = false;
-this.tryStartPolling();
- }
-
showSavedSearches() {
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
this.router.navigateByUrl('/alerts-list(dialog:saved-searches)');
}
showSaveSearch() {
-this.saveRefreshState();
+this.autoPollingSvc.setSuppression(true);
this.saveSearchService.setCurrentQueryBuilderAndTableColumns(this.queryBuilder,
this.alertsColumns);
this.router.navigateByUrl('/alerts-list(dialog:save-search)');
}
- tryStartPolling(manualSearch?: SearchRequest) {
-if (!this.isRefreshPaused && !manualSearch) {
- this.tryStopPolling();
- this.refreshTimer =
this.searchService.pollSearch(this.queryBuilder.searchRequest).subscribe(results
=> {
-this.setData(results);
- });
-} else if (!this.isRefreshPaused && manualSearch) {
- this.tryStopPolling();
- this.refreshTimer =
this.searchService.pollSearch(manualSearch).subscribe(results => {
-this.setData(results);
- });
-}
- }
-
- tryStopPolling() {
-if (this.refreshTimer && !this.refreshTimer.closed) {
- this.refreshTimer.unsubscribe();
-}
- }
-
- updateConfigRowsSettings() {
-this.searchService.interval = this.refreshInterval;
- }
-
Review comment:
Logic moved to auto-polling.service
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326998004
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -99,17 +108,23 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
private metaAlertsService: MetaAlertService,
private globalConfigService: GlobalConfigService,
private dialogService: DialogService,
+ private cdRef: ChangeDetectorRef,
public queryBuilder: QueryBuilder,
- private cdRef: ChangeDetectorRef) {
+ public autoPollingSvc: AutoPollingService) {
router.events.subscribe(event => {
if (event instanceof NavigationStart && event.url === '/alerts-list') {
this.selectedAlerts = [];
-this.restoreRefreshState();
+this.restoreAutoPollingState();
}
});
+
+autoPollingSvc.data.subscribe((result: SearchResponse) => {
+ this.setData(result);
+ this.staleDataState = false;
+})
Review comment:
I was separating auto polling from normal search to distinguish timer and
user triggered queries The output of auto polling is now an observable.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327007289
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -549,24 +522,52 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
this.cdRef.detectChanges();
}
- toggleQueryBuilder() {
+ getStaleDataWarning() {
+if (this.autoPollingSvc.getIsPollingActive()) {
+ return ` Data is in a
stale state!
+Click to update your
view based
+on your current filter and time-range configuration!`;
+} else {
+ return ` Data is in a
stale state!
+Automatic refresh is turned on. Your filter and/or time-range changes
will apply automatically on next refresh.`;
+}
+ }
+
+ getPollingCongestionWarning() {
+return ` Refresh interval
is shorter than the response time.
+ Please increase the refresh interval in the menu above,
+ or try to simplify your query filter.`;
+ }
+
+ private updatePollingInterval(refreshInterval: number): void {
+this.autoPollingSvc.setInterval(refreshInterval);
+ }
+
+ private restoreAutoPollingState() {
+if (this.autoPollingSvc.getIsPollingActive()) {
+ this.autoPollingSvc.setSuppression(false);
+}
+ }
+
+ isQueryBuilderModeManual() {
+return this.queryBuilder.getFilteringMode() === FilteringMode.MANUAL;
+ }
+
+ toggleQueryBuilderMode() {
+// FIXME setting timerange on toggle feels like a hack
this.setSelectedTimeRange([this.selectedTimeRange]);
-if (!this.hideQueryBuilder) {
- this.hideQueryBuilder = true;
- this.manualQuery.nativeElement.value = this.queryBuilder.query;
+if (this.queryBuilder.getFilteringMode() === FilteringMode.BUILDER) {
+ this.queryBuilder.setFilteringMode(FilteringMode.MANUAL);
} else {
- this.hideQueryBuilder = false;
+ this.queryBuilder.setFilteringMode(FilteringMode.BUILDER);
+ // FIXME: this could lead to a large blocking load depending on the
response time
Review comment:
A separated jira was made out of this.
There are lot's of opinions about using FIXME. In here I intentionally kept
them bc alerts-list can't be cleared out/refactored in one iteration and
reminders about already identified issues can be handy later when we touching
this part of the code again.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327019294
##
File path: metron-interface/metron-alerts/src/app/model/table-metadata.ts
##
@@ -15,23 +15,17 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-import {PageSize, RefreshInterval} from
'../alerts/configure-rows/configure-rows-enums';
-import {ColumnMetadata} from './column-metadata';
+import { PageSize } from '../alerts/configure-rows/configure-rows-enums';
+import { ColumnMetadata } from './column-metadata';
export class TableMetadata {
size = PageSize.TWENTY_FIVE;
- refreshInterval = RefreshInterval.TEN_MIN;
- hideResolvedAlerts = true;
- hideDismissedAlerts = true;
Review comment:
hideResolvedAlerts = true;
hideDismissedAlerts = true;
were dead code.
refreshInterval was moved to auto-polling.service
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326996827
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -62,10 +63,7 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
alerts: Alert[] = [];
searchResponse: SearchResponse = new SearchResponse();
colNumberTimerId: number;
- refreshInterval = RefreshInterval.TEN_MIN;
- refreshTimer: Subscription;
- isRefreshPaused = POLLING_DEFAULT_STATE;
- lastIsRefreshPausedValue = false;
Review comment:
Auto polling related this moved to auto-polling.component or
auto-polling.service to keep this functionality encapsulated and shrink the
size of alerts-list.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327004201
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -386,31 +410,19 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
this.setSearchRequestSize();
-if (this.hideQueryBuilder) {
- const newSearch = new SearchRequest();
- newSearch.query = this.manualQuery.nativeElement.value;
- newSearch.size = this.pagination.size;
- newSearch.from = 0;
-
- this.searchService.search(newSearch).subscribe(results => {
-this.setData(results);
-this.staleDataState = false;
- }, error => {
-this.setData(new SearchResponse());
-
this.dialogService.launchDialog(ElasticsearchUtils.extractESErrorMessage(error),
DialogType.Error);
- });
-
- this.tryStartPolling(newSearch);
-} else {
-
this.searchService.search(this.queryBuilder.searchRequest).subscribe(results =>
{
+this.pendingSearch =
this.searchService.search(this.queryBuilder.searchRequest).subscribe(
+ results => {
this.setData(results);
+this.pendingSearch = null;
this.staleDataState = false;
}, error => {
this.setData(new SearchResponse());
-
this.dialogService.launchDialog(ElasticsearchUtils.extractESErrorMessage(error),
DialogType.Error);
+this.pendingSearch = null;
+this.dialogService.launchDialog('Server were unable to apply query
string.', DialogType.Error);
});
- this.tryStartPolling();
+if (this.autoPollingSvc.getIsPollingActive()) {
+ this.autoPollingSvc.dropNextAndContinue();
Review comment:
This was a large block of code duplication.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests URL: https://github.com/apache/metron/pull/1514#discussion_r327009244 ## File path: metron-interface/metron-alerts/src/app/alerts/alerts-list/auto-polling/auto-polling.component.html ## @@ -0,0 +1,17 @@ + + + + + Review comment: Auto polling UI controls refactored to a separated component. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326997272
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -75,7 +73,18 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
@ViewChild('table') table: ElementRef;
@ViewChild('dataViewComponent') dataViewComponent: TableViewComponent;
@ViewChild(AlertSearchDirective) alertSearchDirective: AlertSearchDirective;
- @ViewChild('manualQuery') manualQuery: ElementRef;
+
+ private manualQueryFieldChangeSubs: Subscription;
+ private manualQueryInputEl: ElementRef;
+ @ViewChild('manualQuery') set manualQuery(el: ElementRef) {
+if (el && !this.manualQueryInputEl) {
+ this.manualQueryInputEl = el;
+ this.manualQueryFieldChangeSubs =
this.addManualQueryFieldChangeStream(el.nativeElement);
+}
+ };
+ get manualQuery(): ElementRef {
+return this.manualQueryInputEl;
+ }
Review comment:
This structure is used to use to deal with a viewChild in an ngIf.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326995509
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.spec.ts
##
@@ -28,53 +28,136 @@ import { SaveSearchService } from
'app/service/save-search.service';
import { MetaAlertService } from 'app/service/meta-alert.service';
import { GlobalConfigService } from 'app/service/global-config.service';
import { DialogService } from 'app/service/dialog.service';
-import { SearchRequest } from 'app/model/search-request';
-import { Observable, of, Subject } from 'rxjs';
-import { Filter } from 'app/model/filter';
-import { QueryBuilder } from './query-builder';
import { TIMESTAMP_FIELD_NAME } from 'app/utils/constants';
-import { SearchResponse } from 'app/model/search-response';
import { By } from '@angular/platform-browser';
+import { Observable, Subject, of, throwError } from 'rxjs';
+import { Filter } from 'app/model/filter';
+import { QueryBuilder, FilteringMode } from './query-builder';
+import { SearchResponse } from 'app/model/search-response';
+import { AutoPollingService } from './auto-polling/auto-polling.service';
+import { Router } from '@angular/router';
+import { Alert } from 'app/model/alert';
+import { AlertSource } from 'app/model/alert-source';
+import { SearchRequest } from 'app/model/search-request';
+import { query } from '@angular/core/src/render3';
+import { RestError } from 'app/model/rest-error';
+import { DialogType } from 'app/shared/metron-dialog/metron-dialog.component';
+
+@Component({
+ selector: 'app-auto-polling',
+ template: '',
+})
+class MockAutoPollingComponent {}
+
+@Component({
+ selector: 'app-configure-rows',
+ template: '',
+})
+class MockConfigureRowsComponent {
+ @Input() refreshInterval = 0;
+ @Input() srcElement = {};
+ @Input() pageSize = 0;
+}
+
+@Component({
+ selector: 'app-modal-loading-indicator',
+ template: '',
+})
+class MockModalLoadingIndicatorComponent {
+ @Input() show = false;
+}
+
+@Component({
+ selector: 'app-time-range',
+ template: '',
+})
+class MockTimeRangeComponent {
+ @Input() disabled = false;
+ @Input() selectedTimeRange = {};
+}
+
+@Directive({
+ selector: '[appAceEditor]',
+})
+class MockAceEditorDirective {
+ @Input() text = '';
+}
+
+@Component({
+ selector: 'app-alert-filters',
+ template: '',
+})
+class MockAlertFilterComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-group-by',
+ template: '',
+})
+class MockGroupByComponent {
+ @Input() facets = [];
+}
+
+@Component({
+ selector: 'app-table-view',
+ template: '',
+})
+class MockTableViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+}
+
+@Component({
+ selector: 'app-tree-view',
+ template: '',
+})
+class MockTreeViewComponent {
+ @Input() alerts = [];
+ @Input() pagination = {};
+ @Input() alertsColumnsToDisplay = [];
+ @Input() selectedAlerts = [];
+ @Input() globalConfig = {};
+ @Input() query = '';
+ @Input() groups = [];
+}
+
describe('AlertsListComponent', () => {
let component: AlertsListComponent;
let fixture: ComponentFixture;
- let searchServiceStub = {
-search() { return of({
- total: 0,
- groupedBy: '',
- results: [],
- facetCounts: [],
- groups: []
-}) },
-pollSearch() { return of({}) }
- }
- let queryBuilderStub = {
-addOrUpdateFilter() { return {} },
-clearSearch() { return {} },
-generateSelect() { return '*' },
-isTimeStampFieldPresent() { return {} },
-filters: [{}],
-searchRequest: {
- from: 0
-}
- }
Review comment:
I removed this kind of a third type of mocking to when we use variables to
define mocks. In other places in our code base we used to use mock classes and
inline mocks in our import declaration.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327019948
##
File path: metron-interface/metron-alerts/src/app/service/search.service.ts
##
@@ -79,21 +76,12 @@ export class SearchService {
catchError(HttpUtil.handleError));
}
- public pollSearch(searchRequest: SearchRequest): Observable {
-return this.ngZone.runOutsideAngular(() => {
- return this.ngZone.run(() => {
-return observableInterval(this.interval * 1000).pipe(switchMap(() => {
- return this.search(searchRequest);
-}));
- });
-});
- }
-
Review comment:
polling interval logic was refactored to auto-polling.service.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327020559
##
File path:
metron-interface/metron-alerts/src/app/shared/modal-loading-indicator/modal-loading-indicator.component.scss
##
@@ -15,7 +15,26 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-"use strict";
-exports.environment = {
-production: true
-};
Review comment:
Git get confused on this.
environment.prod.js was deleted as an unused file
scss was introduced for the modal loader.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326993769
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.scss
##
@@ -197,27 +197,6 @@ $searchbox-height: 42px;
cursor: pointer;
}
-.pause-play {
- height: 38px;
- padding: 0px;
- border: 1px solid #0F6F9E;
- width: 38px;
- line-height: 39px;
- border-radius: 12px;
- margin-left: 15px;
- background: $mine-shaft-2;
- cursor: pointer;
-
- i {
-font-size: 17px;
-color: $piction-blue;
- }
-
- .fa-play {
-padding-left: 3px;
- }
-}
-
Review comment:
alerts-list.component is extremely big. I introduced an
auto-polling.component to encapsulate visual aspect of auto polling and make
alerts-list little bit smaller.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327019632
##
File path:
metron-interface/metron-alerts/src/app/service/elasticsearch-localstorage-impl.ts
##
@@ -20,6 +16,8 @@ import {catchError, map, onErrorResumeNext} from
'rxjs/operators';
* limitations under the License.
*/
import {Observable} from 'rxjs';
+import {throwError as observableThrowError} from 'rxjs';
+import {catchError, map, onErrorResumeNext} from 'rxjs/operators';
Review comment:
Moved imports after the license header.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327017385
##
File path:
metron-interface/metron-alerts/src/app/alerts/configure-rows/configure-rows.component.ts
##
@@ -16,53 +16,29 @@
* limitations under the License.
*/
import { Component, Input, HostListener, ElementRef, Output, EventEmitter }
from '@angular/core';
-import { TableMetadata } from '../../model/table-metadata';
-import { ConfigureTableService } from '../../service/configure-table.service';
+export interface ConfigureRowsModel {
+ values: {
+pageSize: number;
+refreshInterval: number;
+ },
+ triggerQuery: boolean
+}
@Component({
selector: 'app-configure-rows',
templateUrl: './configure-rows.component.html',
styleUrls: ['./configure-rows.component.scss']
})
export class ConfigureRowsComponent {
-
showView = false;
- tableMetadata = new TableMetadata();
@Input() srcElement: HTMLElement;
- @Output() sizeChange = new EventEmitter();
- @Output() intervalChange = new EventEmitter();
- @Output() configRowsChange = new EventEmitter();
-
- constructor(private elementRef: ElementRef,
- private configureTableService: ConfigureTableService) {}
-
- @Input()
- get size() {
-return this.tableMetadata.size;
- }
+ @Input() pageSize: number;
+ @Input() refreshInterval: number;
- set size(val) {
-this.tableMetadata.size = val;
- }
+ @Output() configRowsChange = new EventEmitter();
- @Input()
- get interval() {
-return this.tableMetadata.refreshInterval;
- }
-
- set interval(val) {
-this.tableMetadata.refreshInterval = val;
- }
-
- @Input()
- get tableMetaData() {
-return this.tableMetadata;
- }
-
- set tableMetaData(val) {
Review comment:
tableMetaData is no directly used by this component so I refactored it out.
Changes here was needed to separate config change events we like to trigger
query from others.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327021175
##
File path: metron-interface/metron-alerts/src/app/utils/constants.ts
##
@@ -36,7 +36,7 @@ export const CUSTOMM_DATE_RANGE_LABEL = 'Date Range';
export const TREE_SUB_GROUP_SIZE = 5;
export const INDEXES = environment.indices ? environment.indices.split(',') :
[];
-export const POLLING_DEFAULT_STATE = !environment.defaultPollingState;
Review comment:
The logic here was a bit confused. It was fixed and aligned everywhere in
the code.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327003814
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -271,9 +298,24 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
this.staleDataState = true;
}
- onConfigRowsChange() {
-this.searchService.interval = this.refreshInterval;
-this.search();
Review comment:
onConfigRowChange was triggering a manual query. Auto polling interval
setting is part of row config. It make no sense to trigger a manual query when
user change a settings belong to auto polling. I was separated this.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r327003056
##
File path:
metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.ts
##
@@ -258,11 +289,7 @@ export class AlertsListComponent implements OnInit,
OnDestroy {
alert => (alert.source.metron_alert && alert.source.metron_alert.length
> 0)
);
-if (selectedAlerts.length > 0) {
- this.pause();
-} else {
- this.resume();
-}
+this.autoPollingSvc.setSuppression(!!selectedAlerts.length);
Review comment:
The concept of suppression was added to auto polling to prevent starting and
sopping it when ever another feature interfere whit it. With this we no longer
resetting the timer, persistent state only turning a flag on and skipping
polling cycles til it's needed. (the list of features suppressing auto polling
is in the pr desc)
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests URL: https://github.com/apache/metron/pull/1514#discussion_r326992280 ## File path: metron-interface/metron-alerts/src/app/alerts/alerts-list/alerts-list.component.html ## @@ -12,33 +12,37 @@ the specific language governing permissions and limitations under the License. Review comment: Only minor refactoring happened this file to make it more clear. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests URL: https://github.com/apache/metron/pull/1514#discussion_r326991804 ## File path: metron-interface/metron-alerts/cypress/integration/search/auto-polling.feature.spec.js ## @@ -0,0 +1,98 @@ +/// Review comment: Cypress test: high level UAT for auto polling. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326991079
##
File path: metron-interface/metron-alerts/cypress/fixtures/search-1.1.json
##
@@ -0,0 +1,102 @@
+{
Review comment:
Cypress test: mock data.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [metron] tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts UI: Indicating loading and preventing parallel requests
tiborm commented on a change in pull request #1514: METRON-2190: [UI] Alerts
UI: Indicating loading and preventing parallel requests
URL: https://github.com/apache/metron/pull/1514#discussion_r326991248
##
File path: metron-interface/metron-alerts/cypress/fixtures/search-1.2.json
##
@@ -0,0 +1,102 @@
+{
Review comment:
Cypress test: mock data.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
