Skip to content

refactor: use new group limit in explorer #698

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

Merged
merged 5 commits into from
Mar 16, 2021
+80 −121
Merged

refactor: use new group limit in explorer #698

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f3a02f8
refactor: use new group limit in explorer
aaron-steinfeld Mar 16, 2021
d0a6f5a
test: fix up explorer query handler test
aaron-steinfeld Mar 16, 2021
5d7595b
Merge branch 'main' into explorer-group-limit
aaron-steinfeld Mar 16, 2021
7ead1ee
test: more cleanup
aaron-steinfeld Mar 16, 2021
6d1dcf3
Merge branch 'main' into explorer-group-limit
aaron-steinfeld Mar 16, 2021
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
8 changes: 4 additions & 4 deletions projects/observability/src/pages/explorer/explorer.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ describe('Explorer component', () => {
expect.objectContaining({
requestType: EXPLORE_GQL_REQUEST,
context: ObservabilityTraceType.Api,
limit: 500,
limit: 1000,
interval: new TimeDuration(15, TimeUnit.Second)
}),
expect.objectContaining({})
Expand Down Expand Up @@ -186,7 +186,7 @@ describe('Explorer component', () => {
requestType: EXPLORE_GQL_REQUEST,
context: ObservabilityTraceType.Api,
filters: [new GraphQlFieldFilter('first', GraphQlOperatorType.Equals, 'foo')],
limit: 500,
limit: 1000,
interval: new TimeDuration(15, TimeUnit.Second)
}),
expect.objectContaining({})
Expand Down Expand Up @@ -222,7 +222,7 @@ describe('Explorer component', () => {
expect.objectContaining({
requestType: EXPLORE_GQL_REQUEST,
context: SPAN_SCOPE,
limit: 500,
limit: 1000,
interval: new TimeDuration(15, TimeUnit.Second)
}),
expect.objectContaining({})
Expand Down Expand Up @@ -274,7 +274,7 @@ describe('Explorer component', () => {
expect.objectContaining({
requestType: EXPLORE_GQL_REQUEST,
context: SPAN_SCOPE,
limit: 500,
limit: 1000,
interval: new TimeDuration(15, TimeUnit.Second),
filters: [new GraphQlFieldFilter('first', GraphQlOperatorType.Equals, 'foo')]
}),
Expand Down
8 changes: 4 additions & 4 deletions ...ability/src/shared/components/explore-query-editor/explore-query-editor.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,9 @@ describe('Explore query editor', () => {
expect(onRequestChange).toHaveBeenLastCalledWith(
expect.objectContaining({
series: [defaultSeries],
groupByLimit: 5, // Default group by limit
groupBy: {
keys: ['first groupable']
keys: ['first groupable'],
limit: 5 // Default group by limit
}
})
);
Expand Down Expand Up @@ -213,9 +213,9 @@ describe('Explore query editor', () => {
expect(onQueryChange).toHaveBeenLastCalledWith(
expect.objectContaining({
series: [defaultSeries],
groupByLimit: 6,
groupBy: {
keys: ['first groupable']
keys: ['first groupable'],
limit: 6
}
})
);
Expand Down
15 changes: 9 additions & 6 deletions ...bservability/src/shared/components/explore-query-editor/explore-query-editor.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,19 +44,20 @@ import {
></ht-explore-query-group-by-editor>

<ht-explore-query-limit-editor
*ngIf="currentVisualization.groupBy"
class="limit"
[limit]="currentVisualization.groupByLimit"
(limitChange)="this.setLimit($event)"
[limit]="currentVisualization.groupBy?.limit"
(limitChange)="this.updateGroupByLimit(currentVisualization.groupBy!, $event)"
[includeRest]="currentVisualization.groupBy?.includeRest"
(includeRestChange)="this.updateGroupByIncludeRest(currentVisualization.groupBy!, $event)"
[disabled]="!currentVisualization.groupBy"
>
</ht-explore-query-limit-editor>
</div>
</div>
`
})
export class ExploreQueryEditorComponent implements OnChanges, OnInit {
private static readonly DEFAULT_GROUP_LIMIT: number = 5;
@Input()
public filters?: Filter[];

Expand Down Expand Up @@ -94,16 +95,18 @@ export class ExploreQueryEditorComponent implements OnChanges, OnInit {
if (key === undefined) {
this.visualizationBuilder.groupBy();
} else {
this.visualizationBuilder.groupBy(groupBy ? { ...groupBy, keys: [key] } : { keys: [key] });
this.visualizationBuilder.groupBy(
groupBy ? { ...groupBy, keys: [key] } : { keys: [key], limit: ExploreQueryEditorComponent.DEFAULT_GROUP_LIMIT }
);
}
}

public updateGroupByIncludeRest(groupBy: GraphQlGroupBy, includeRest: boolean): void {
this.visualizationBuilder.groupBy({ ...groupBy, includeRest: includeRest });
}

public setLimit(limit: number): void {
this.visualizationBuilder.groupByLimit(limit);
public updateGroupByLimit(groupBy: GraphQlGroupBy, limit: number): void {
this.visualizationBuilder.groupBy({ ...groupBy, limit: limit });
}

public setInterval(interval: IntervalValue): void {
Expand Down
17 changes: 5 additions & 12 deletions ...vability/src/shared/components/explore-query-editor/explore-visualization-builder.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,30 +83,23 @@ describe('Explore visualization builder', () => {
spectator.service
.setSeries([buildSeries('test1')])
.groupBy({
keys: ['testGroupBy']
keys: ['testGroupBy'],
limit: 15
})
.groupByLimit(15)
.setSeries([buildSeries('test2')]);

expectObservable(recordedRequests).toBe('(abcde)', {
expectObservable(recordedRequests).toBe('(abcd)', {
a: expectedQuery(),
b: expectedQuery({
series: [matchSeriesWithName('test1')]
}),
c: expectedQuery({
series: [matchSeriesWithName('test1')],
groupBy: { keys: ['testGroupBy'] },
groupByLimit: 5
groupBy: { keys: ['testGroupBy'], limit: 15 }
}),
d: expectedQuery({
series: [matchSeriesWithName('test1')],
groupBy: { keys: ['testGroupBy'] },
groupByLimit: 15
}),
e: expectedQuery({
series: [matchSeriesWithName('test2')],
groupBy: { keys: ['testGroupBy'] },
groupByLimit: 15
groupBy: { keys: ['testGroupBy'], limit: 15 }
})
});
});
Expand Down
24 changes: 6 additions & 18 deletions ...observability/src/shared/components/explore-query-editor/explore-visualization-builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ import { CartesianSeriesVisualizationType } from '../cartesian/chart';

@Injectable()
export class ExploreVisualizationBuilder implements OnDestroy {
private static readonly DEFAULT_GROUP_LIMIT: number = 5;
private static readonly DEFAULT_UNGROUPED_LIMIT: number = 500;
private static readonly DEFAULT_LIMIT: number = 1000;

public readonly visualizationRequest$: Observable<ExploreVisualizationRequest>;
private readonly destroyed$: Subject<void> = new Subject();
Expand Down Expand Up @@ -80,20 +79,8 @@ export class ExploreVisualizationBuilder implements OnDestroy {
}

public groupBy(groupBy?: GraphQlGroupBy): this {
const groupByLimit =
this.currentState().groupByLimit !== undefined
? this.currentState().groupByLimit
: ExploreVisualizationBuilder.DEFAULT_GROUP_LIMIT;

return this.updateState({
groupBy: groupBy,
groupByLimit: groupBy && groupByLimit
});
}

public groupByLimit(limit: number): this {
return this.updateState({
groupByLimit: limit
groupBy: groupBy
});
}

Expand Down Expand Up @@ -128,11 +115,11 @@ export class ExploreVisualizationBuilder implements OnDestroy {
private buildRequest(state: ExploreRequestState): ExploreVisualizationRequest {
return {
context: state.context,
resultLimit: state.resultLimit,
series: [...state.series],
filters: state.filters && [...state.filters],
interval: this.resolveInterval(state.interval),
groupBy: state.groupBy && { ...state.groupBy },
groupByLimit: state.groupByLimit,
exploreQuery$: this.mapStateToExploreQuery(state),
resultsQuery$: this.mapStateToResultsQuery(state)
};
Expand All @@ -146,7 +133,7 @@ export class ExploreVisualizationBuilder implements OnDestroy {
interval: this.resolveInterval(state.interval),
filters: state.filters && this.graphQlFilterBuilderService.buildGraphQlFilters(state.filters),
groupBy: state.groupBy,
limit: state.groupByLimit === undefined ? ExploreVisualizationBuilder.DEFAULT_UNGROUPED_LIMIT : state.groupByLimit
limit: state.resultLimit
});
}

Expand Down Expand Up @@ -215,6 +202,7 @@ export class ExploreVisualizationBuilder implements OnDestroy {
return {
context: context,
interval: 'AUTO',
resultLimit: ExploreVisualizationBuilder.DEFAULT_LIMIT,
series: [this.buildDefaultSeries(context)]
};
}
Expand Down Expand Up @@ -242,8 +230,8 @@ export interface ExploreRequestState {
interval?: TimeDuration | 'AUTO';
filters?: Filter[];
groupBy?: GraphQlGroupBy;
groupByLimit?: number;
useGroupName?: boolean;
resultLimit: number;
}

export type ExploreRequestContext = TraceType | 'SPAN' | 'DOMAIN_EVENT';
Expand Down
1 change: 0 additions & 1 deletion ...rc/shared/components/explore-query-editor/limit/explore-query-limit-editor.component.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@

.limit-label {
@include body-1-medium($gray-9);
@include disableable;
height: 32px;
line-height: 32px;
margin-bottom: 12px;
Expand Down
19 changes: 0 additions & 19 deletions ...shared/components/explore-query-editor/limit/explore-query-limit-editor.component.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,25 +26,6 @@ describe('Explore Query Limit Editor component', () => {
expect(spectator.query(InputComponent)!.value).toBe(12);
});

test('sets limit as disabled and display to Auto when disabled', () => {
const spectator = hostBuilder(
`
<ht-explore-query-limit-editor [limit]="limit" [disabled]="disabled">
</ht-explore-query-limit-editor>`,
{
hostProps: {
limit: 12,
disabled: true
}
}
);

expect(spectator.query(InputComponent)!.value).toBe('Auto');
expect(spectator.query(InputComponent)!.type).toBe('text');
expect(spectator.query(InputComponent)!.disabled).toBe(true);
expect(spectator.query('.limit-label')).toHaveClass('disabled');
});

test('updates when the provided limit changes', () => {
const spectator = hostBuilder(
`
Expand Down
27 changes: 5 additions & 22 deletions .../src/shared/components/explore-query-editor/limit/explore-query-limit-editor.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,21 @@ import { InputAppearance } from '@hypertrace/components';
template: `
<div class="limit-container">
<div class="limit-number-input-container">
<span class="limit-label" [ngClass]="{ disabled: this.disabled }"> Max Results </span>
<span class="limit-label"> Max Groups </span>
<div class="limit-input-wrapper">
<ht-input
[type]="this.getInputType()"
type="number"
class="limit-input"
appearance="${InputAppearance.Border}"
[disabled]="this.disabled"
[value]="this.getLimitValue()"
[value]="this.limit"
(valueChange)="this.limitChange.emit($event)"
>
</ht-input>
</div>
</div>
<div class="limit-include-rest-container" *ngIf="!this.disabled">
<div class="limit-include-rest-container">
<span class="limit-include-rest-label"> Show Other: </span>
<ht-checkbox
[disabled]="this.disabled"
[checked]="this.includeRest"
(checkedChange)="this.includeRestChange.emit($event)"
>
</ht-checkbox>
<ht-checkbox [checked]="this.includeRest" (checkedChange)="this.includeRestChange.emit($event)"> </ht-checkbox>
</div>
</div>
`
Expand All @@ -40,20 +34,9 @@ export class ExploreQueryLimitEditorComponent {
@Input()
public includeRest: boolean = false;

@Input()
public disabled: boolean = false;

@Output()
public readonly limitChange: EventEmitter<number> = new EventEmitter();

@Output()
public readonly includeRestChange: EventEmitter<boolean> = new EventEmitter();

public getInputType(): string {
return this.disabled ? 'text' : 'number';
}

public getLimitValue(): string | number {
return this.disabled ? 'Auto' : this.limit;
}
}
2 changes: 1 addition & 1 deletion ...vability/src/shared/dashboard/data/graphql/explore/explore-cartesian-data-source.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export abstract class ExploreCartesianDataSourceModel extends GraphQlDataSourceM
requestType: EXPLORE_GQL_REQUEST,
selections: requestState.series.map(series => series.specification),
context: requestState.context,
limit: requestState.groupByLimit ?? 100,
limit: requestState.resultLimit,
Copy link
Contributor

@anandtiwary anandtiwary Mar 16, 2021

Choose a reason for hiding this comment

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

On the consumer side, do we need to change the existing limit in modelJsons (may be) since this will now limit the number of results?

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Mar 16, 2021

Choose a reason for hiding this comment

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

None exist in the UI as of today (besides what you've got in flight), but in general yes - new data models that extend this will have to use the two types of limit appropriately.

timeRange: this.getTimeRangeOrThrow(),
interval: requestState.interval as TimeDuration,
filters: filters,
Expand Down
24 changes: 6 additions & 18 deletions ...data/graphql/explorer-visualization/explorer-visualization-cartesian-data-source.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { TimeDuration } from '@hypertrace/common';
import { GraphQlFilter } from '@hypertrace/distributed-tracing';
import { Model } from '@hypertrace/hyperdash';
import { NEVER, Observable } from 'rxjs';
Expand All @@ -19,18 +18,16 @@ import { ExploreCartesianDataSourceModel, ExplorerData } from '../explore/explor
export class ExplorerVisualizationCartesianDataSourceModel extends ExploreCartesianDataSourceModel {
public request?: ExploreVisualizationRequest;

protected fetchResults(interval: TimeDuration | 'AUTO'): Observable<CartesianResult<ExplorerData>> {
const requestState = this.buildRequestState(interval);

if (this.request === undefined || requestState === undefined) {
protected fetchResults(): Observable<CartesianResult<ExplorerData>> {
if (this.request === undefined) {
return NEVER;
}

return this.request.exploreQuery$.pipe(
switchMap(exploreRequest =>
this.query<ExploreGraphQlQueryHandlerService>(inheritedFilters =>
this.appendFilters(exploreRequest, this.getFilters(inheritedFilters))
).pipe(mergeMap(response => this.mapResponseData(requestState, response)))
).pipe(mergeMap(response => this.mapResponseData(this.request!, response)))
)
);
}
Expand All @@ -46,17 +43,8 @@ export class ExplorerVisualizationCartesianDataSourceModel extends ExploreCartes
};
}

protected buildRequestState(interval: TimeDuration | 'AUTO'): ExploreRequestState | undefined {
if (this.request?.series.length === 0 || this.request?.context === undefined) {
return undefined;
}

return {
series: this.request.series,
context: this.request.context,
interval: interval,
groupBy: this.request.groupBy,
groupByLimit: this.request.groupByLimit
};
protected buildRequestState(): ExploreRequestState | undefined {
// Unused since fetchResults is overriden, but abstract so requires a def
Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Mar 16, 2021

Choose a reason for hiding this comment

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

This one's a bit weird. This is an abstract method used in the parent's fetchResults which we override in this class. It's not really relevant to this implementation since fetchResults uses an observable of state rather than building its own.

This was actually breaking the explorer, (although no one noticed) since the previous implementation was mapping the response data based on the initial state, not the current one.

Copy link
Contributor

@anandtiwary anandtiwary Mar 16, 2021

Choose a reason for hiding this comment

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

Yeah. AFAIR, the ExploreVisualizationRequest did not have all the latest request data. I think we emit on exploreQuery$: Observable<TimeUnaware<GraphQlExploreRequest>>; with latest interval data (possibly more). When i tested it last, using ExploreVisualizationRequest directly did break few use cases for me. So for correctness, I kept it the same way for this data source (but fixed the dependency in the parent data source).

Copy link
Contributor

@anandtiwary anandtiwary Mar 16, 2021

Choose a reason for hiding this comment

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

If this is working, then you don't need to override fetchResults. Just return this.request inside buildRequestState

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Mar 16, 2021

Choose a reason for hiding this comment

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

As a user interacts with the explorer, all of the fields on ExploreVisualizationRequest may change, and the new queries are emitted on that observable - so we should never really be reading from ExploreVisualizationRequest directly (it's basically like the angular router snapshot vs observable state, but with more confusing naming), but we also can't build state from it either - we should always use the latest from the observable. One way to see the current issue is going to the explorer, selecting a group by key and removing the interval. Instead of getting a segmented aggregation, nothing renders (because we process the result thinking it still has an interval).

Copy link
Contributor

@anandtiwary anandtiwary Mar 16, 2021

Choose a reason for hiding this comment

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

ah okay. It will still be confusing to a new developer. Do you think we should change the design a little bit (as a tech debt)?

Copy link
Contributor Author

@aaron-steinfeld aaron-steinfeld Mar 16, 2021

Choose a reason for hiding this comment

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

yeah, for sure.

return undefined;
}
}
11 changes: 10 additions & 1 deletion ...bility/src/shared/dashboard/data/graphql/table/explore/explore-table-data-source.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
ModelModelPropertyTypeInstance,
ModelProperty,
ModelPropertyType,
NUMBER_PROPERTY,
STRING_PROPERTY
} from '@hypertrace/hyperdash';
import { ExploreSpecification } from './../../../../../graphql/model/schema/specifications/explore-specification';
Expand Down Expand Up @@ -57,6 +58,13 @@ export class ExploreTableDataSourceModel extends TableDataSourceModel {
})
public groupByIncludeRest: boolean = true;

@ModelProperty({
key: 'group-limit',
required: false,
type: NUMBER_PROPERTY.type
})
public groupLimit: number = 100;

public getScope(): string {
return this.context;
}
Expand All @@ -82,7 +90,8 @@ export class ExploreTableDataSourceModel extends TableDataSourceModel {
timeRange: this.getTimeRangeOrThrow(),
groupBy: {
keys: this.groupBy,
includeRest: this.groupByIncludeRest
includeRest: this.groupByIncludeRest,
limit: this.groupLimit
}
};
}
Expand Down
Loading