Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explorer - Fix Retaining of filters on switching between Trace and Span view #1486

Closed
wants to merge 7 commits into from
Closed

Explorer - Fix Retaining of filters on switching between Trace and Span view #1486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a58d5ba
chore: added filter chip service to exports
jaywalker21 Mar 15, 2022
9b3714e
fix(parse_url_filter): updated logic to first try exact match before …
jaywalker21 Mar 15, 2022
94aded6
feat(filter-bar): updated filter bar to be controlled component with …
jaywalker21 Mar 15, 2022
984220d
feat(explorer): added tab switch filter handling logic to explorer
jaywalker21 Mar 15, 2022
1a508e0
Merge branch 'latest_fork' into fix/explorer-tab-switch
jaywalker21 Mar 15, 2022
926a342
chore: removed linting errors from explorer component
jaywalker21 Mar 15, 2022
b91643d
Merge branch 'main' into fix/explorer-tab-switch
jaywalker21 Mar 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions projects/components/src/filtering/filter-bar/filter-bar.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
} from '@angular/core';
import { IconType } from '@hypertrace/assets-library';
import { TypedSimpleChanges } from '@hypertrace/common';
import { isEqual } from 'lodash-es';
import { isEmpty, isEqual } from 'lodash-es';
import { BehaviorSubject, Observable, Subscription } from 'rxjs';
import { distinctUntilChanged, switchMap } from 'rxjs/operators';
import { IconSize } from '../../icon/icon-size';
Expand Down Expand Up @@ -108,6 +108,18 @@ export class FilterBarComponent implements OnChanges, OnInit, OnDestroy {
) {}

public ngOnChanges(changes: TypedSimpleChanges<this>): void {
if (changes.filters) {
if (changes.attributes) {
this.onFiltersChanged(this.filters || [], false, this.syncWithUrl);
this.attributeSubject$.next(this.attributes || []);

return;
}

// The local state should be in sync with the state passed by parent
this.internalFiltersSubject$.next(changes.filters.currentValue || []);
}

if (changes.attributes) {
this.attributeSubject$.next(this.attributes || []);
this.syncWithUrl ? this.readFromUrlFilters() : this.onFiltersChanged(this.filters || [], false);
Expand All @@ -131,27 +143,33 @@ export class FilterBarComponent implements OnChanges, OnInit, OnDestroy {
}

private onFiltersChanged(filters: Filter[], emit: boolean = true, writeIfSyncEnabled: boolean = true): void {
this.internalFiltersSubject$.next([...filters]);
const newFilters: Filter[] = [...filters];
this.internalFiltersSubject$.next(newFilters);
this.changeDetector.markForCheck();

if (writeIfSyncEnabled && this.syncWithUrl && !!this.attributes) {
this.writeToUrlFilter();
this.writeToUrlFilter(newFilters);
}

emit && this.filtersChange.emit(this.internalFiltersSubject$.value);
emit && this.filtersChange.emit(newFilters);
}

private readFromUrlFilters(): void {
const filters = this.filterUrlService.getUrlFilters(this.attributes || []);
this.onFiltersChanged(filters, true, false);
}

private writeToUrlFilter(): void {
this.filterUrlService.setUrlFilters(this.internalFiltersSubject$.value);
private writeToUrlFilter(filters: Filter[]): void {
this.filterUrlService.setUrlFilters(filters);
}

public onInputApply(filter: Filter): void {
this.onFiltersChanged(this.filterBarService.addFilter(this.internalFiltersSubject$.value, filter));
this.onFiltersChanged(
this.filterBarService.addFilter(
isEmpty(this.internalFiltersSubject$.value) ? [] : this.internalFiltersSubject$.value,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value should never be undefined, so we're just checking if empty and if so assigning empty? did some logic change that forced this? Either this isn't required or we've broken the assigned type.

filter
)
);
this.resetFocus();
}

Expand All @@ -175,10 +193,10 @@ export class FilterBarComponent implements OnChanges, OnInit, OnDestroy {
}

private updateFilter(oldFilter: Filter, newFilter: Filter): void {
this.onFiltersChanged(this.filterBarService.updateFilter(this.internalFiltersSubject$.value, oldFilter, newFilter));
this.onFiltersChanged(this.filterBarService.updateFilter(this.filters || [], oldFilter, newFilter));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we use the subject's value and when do we use this.filters? We seem to use different ways in different places which makes it very confusion to understand as a reader.

}

private deleteFilter(filter: Filter): void {
this.onFiltersChanged(this.filterBarService.deleteFilter(this.internalFiltersSubject$.value, filter));
this.onFiltersChanged(this.filterBarService.deleteFilter(this.filters || [], filter));
}
}
15 changes: 12 additions & 3 deletions projects/components/src/filtering/filter/parser/parsed-filter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,19 @@ export const tryParseStringForAttribute = (
nameFields: KeysWithType<FilterAttribute, string>[] = ['displayName']
): FilterAttributeExpression | undefined => {
const [stringContainingFullAttribute] = text.trim().split(MAP_LHS_DELIMITER, 1);
// The string to the left of any delimeter must start with the attribute otherwise no match
const matchingNameField = nameFields.find(nameField =>
stringContainingFullAttribute.toLowerCase().startsWith(attributeToTest[nameField].toLowerCase())

// Check if there is an exact match for the string left of the delimeter and attribute
let matchingNameField = nameFields.find(
nameField => stringContainingFullAttribute.toLowerCase() === attributeToTest[nameField].toLowerCase()
);

// If there is no exact match, the string to the left of any delimeter must start with the attribute otherwise no match
if (!matchingNameField) {
matchingNameField = nameFields.find(nameField =>
stringContainingFullAttribute.toLowerCase().startsWith(attributeToTest[nameField].toLowerCase())
);
}

if (!matchingNameField) {
return undefined;
}
Expand Down
1 change: 1 addition & 0 deletions projects/components/src/public-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,3 +360,4 @@ export { TooltipDirective } from './tooltip/tooltip.directive';

// Filter Url Service
export * from './filtering/filter/filter-url.service';
export * from './filtering/filter-bar/filter-chip/filter-chip.service';
73 changes: 69 additions & 4 deletions projects/observability/src/pages/explorer/explorer.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import {
TimeDuration,
TimeDurationService
} from '@hypertrace/common';
import { Filter, ToggleItem } from '@hypertrace/components';
import { Filter, FilterAttribute, FilterChipService, IncompleteFilter, ToggleItem } from '@hypertrace/components';
import { isEmpty, isNil } from 'lodash-es';
import { concat, EMPTY, Observable, Subject } from 'rxjs';
import { BehaviorSubject, concat, EMPTY, Observable } from 'rxjs';
import { map, take } from 'rxjs/operators';
import { CartesianSeriesVisualizationType } from '../../shared/components/cartesian/chart';
import {
Expand All @@ -20,7 +20,7 @@ import {
} from '../../shared/components/explore-query-editor/explore-visualization-builder';
import { IntervalValue } from '../../shared/components/interval-select/interval-select.component';
import { AttributeExpression } from '../../shared/graphql/model/attribute/attribute-expression';
import { AttributeMetadata } from '../../shared/graphql/model/metadata/attribute-metadata';
import { AttributeMetadata, toFilterAttributeType } from '../../shared/graphql/model/metadata/attribute-metadata';
import { MetricAggregationType } from '../../shared/graphql/model/metrics/metric-aggregation';
import { GraphQlGroupBy } from '../../shared/graphql/model/schema/groupby/graphql-group-by';
import { ObservabilityTraceType } from '../../shared/graphql/model/schema/observability-traces';
Expand Down Expand Up @@ -52,6 +52,7 @@ import {
class="explorer-filter-bar"
[attributes]="this.attributes$ | async"
[syncWithUrl]="true"
[filters]="this.filters"
(filtersChange)="this.onFiltersUpdated($event)"
></ht-filter-bar>
<div class="explorer-content">
Expand Down Expand Up @@ -145,13 +146,16 @@ export class ExplorerComponent {
public visualizationExpanded$: Observable<boolean>;
public resultsExpanded$: Observable<boolean>;

private readonly contextChangeSubject: Subject<ExplorerGeneratedDashboardContext> = new Subject();
private readonly contextChangeSubject: BehaviorSubject<ExplorerGeneratedDashboardContext> = new BehaviorSubject(
ObservabilityTraceType.Api as ExplorerGeneratedDashboardContext
);

public constructor(
private readonly metadataService: MetadataService,
private readonly navigationService: NavigationService,
private readonly timeDurationService: TimeDurationService,
private readonly preferenceService: PreferenceService,
private readonly filterChipService: FilterChipService,
@Inject(EXPLORER_DASHBOARD_BUILDER_FACTORY) explorerDashboardBuilderFactory: ExplorerDashboardBuilderFactory,
activatedRoute: ActivatedRoute
) {
Expand Down Expand Up @@ -194,11 +198,51 @@ export class ExplorerComponent {
}
}

private convertToFilterAttributes(attrArray: AttributeMetadata[]): FilterAttribute[] {
return attrArray.map(({ name, displayName, units, type, onlySupportsAggregation, onlySupportsGrouping }) => {
const applicableType = toFilterAttributeType(type);

return {
name: name,
displayName: displayName,
units: units,
type: applicableType,
onlySupportsAggregation: onlySupportsAggregation,
onlySupportsGrouping: onlySupportsGrouping
};
});
}

public onContextUpdated(contextWrapper: ExplorerContextScope): void {
this.attributes$ = this.metadataService.getFilterAttributes(contextWrapper.dashboardContext);
const listener = this.attributes$.subscribe(attributes => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try to move this block outside of this method in an independent structure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use a subscription here. Make the filters an observable, and subscribe via async pipe in the template.

const lastTab = this.contextChangeSubject.getValue();
const newFilters = this.filters.map(eachFilter => {
// If the given filter has a different name for the selected tab, update the filter value
if (eachFilter.field in contextMapObject[lastTab]) {
const newFilter = this.filterChipService.autocompleteFilters(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to do this without the chip service, that's an impl detail from the filter bar. Can we use the filter builder service (I hate how complex we made this!) instead just read the details out of the old one and write them into the new one. May need to lookup the new attribute, too - but we shouldn't have to create it, it's already one of the ones in the array coming in.

this.convertToFilterAttributes(attributes),
eachFilter.userString
);

if (!isEmpty(newFilter) && this.isValidFilter(newFilter[0])) {
return newFilter[0];
}
}

return eachFilter;
});

this.filters = newFilters;
});
listener.unsubscribe();
this.contextChangeSubject.next(contextWrapper.dashboardContext);
}

private isValidFilter(incompleteFilter: IncompleteFilter): incompleteFilter is Filter {
return incompleteFilter.operator !== undefined && incompleteFilter.value !== undefined;
}

public onVisualizationExpandedChange(expanded: boolean): void {
this.preferenceService.set(ExplorerComponent.VISUALIZATION_EXPANDED_PREFERENCE, expanded);
}
Expand Down Expand Up @@ -327,6 +371,12 @@ interface ExplorerContextScope {
scopeQueryParam: ScopeQueryParam;
}

type contextMap = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PascalCase for type names (also, we generally use interfaces if constructing a brand new type that's not projected from another.

Naming here is also an issue. I think context is an overloaded term, and even knowing the goal of this PR it took me reading the object using this to understand what this was. Maybe something like

type AttributeTranslationDictionary = { [key: string]: string; }

const spanToApiTraceAttributeTranslationDictionary = {...};
const apiTraceToSpanAttributeTranslationDictionary = {...};

[key in ExplorerGeneratedDashboardContext]: {
[key: string]: string;
};
};

export const enum ScopeQueryParam {
EndpointTraces = 'endpoint-traces',
Spans = 'spans'
Expand All @@ -339,3 +389,18 @@ const enum ExplorerQueryParam {
GroupLimit = 'limit',
Series = 'series'
}

const contextMapObject: contextMap = {
API_TRACE: {
protocol: 'protocolName',
requestMethod: 'spanRequestMethod',
requestUrl: 'spanRequestUrl',
tags: 'spanTags'
},
SPAN: {
protocolName: 'protocol',
spanRequestMethod: 'requestMethod',
spanRequestUrl: 'requestUrl',
spanTags: 'tags'
}
};