Skip to content

feat: add global filtering that applies aross all queries #546

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 2 commits into from
Jan 30, 2021
Merged

feat: add global filtering that applies aross all queries #546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
359234e
feat: add global filtering that applies aross all queries
aaron-steinfeld Jan 29, 2021
0395ea2
Merge branch 'main' into global-filtering
aaron-steinfeld Jan 30, 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
1 change: 1 addition & 0 deletions projects/distributed-tracing/src/public-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export * from './shared/graphql/model/schema/filter/field/graphql-field-filter';
export * from './shared/graphql/model/schema/filter/id/graphql-id-filter';

export * from './shared/graphql/model/schema/filter/graphql-filter';
export * from './shared/graphql/model/schema/filter/global-graphql-filter.service';
export {
GraphQlMetricAggregationType,
convertToGraphQlMetricAggregationType
Expand Down
58 changes: 58 additions & 0 deletions ...uted-tracing/src/shared/graphql/model/schema/filter/global-graphql-filter.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { createServiceFactory } from '@ngneat/spectator/jest';
import { SPAN_SCOPE } from '../span';
import { GraphQlFieldFilter } from './field/graphql-field-filter';
import { GlobalGraphQlFilterService } from './global-graphql-filter.service';
import { GraphQlOperatorType } from './graphql-filter';

describe('Global graphql filter serivce', () => {
const firstFilter = new GraphQlFieldFilter('first', GraphQlOperatorType.Equals, 'first');
const secondFilter = new GraphQlFieldFilter('second', GraphQlOperatorType.Equals, 'second');
const thirdFilter = new GraphQlFieldFilter('third', GraphQlOperatorType.Equals, 'third');
const createService = createServiceFactory({
service: GlobalGraphQlFilterService
});
test('provides no filters by default', () => {
const spectator = createService();

expect(spectator.service.getGlobalFilters(SPAN_SCOPE)).toEqual([]);
});

test('provides filters from multiple matching rules', () => {
const spectator = createService();
spectator.service.setGlobalFilterRule(Symbol('first'), {
filtersForScope: scope => (scope === SPAN_SCOPE ? [firstFilter] : [])
});
spectator.service.setGlobalFilterRule(Symbol('second'), {
filtersForScope: scope => (scope === 'fake' ? [secondFilter] : [])
});
spectator.service.setGlobalFilterRule(Symbol('third'), {
filtersForScope: scope => (scope === SPAN_SCOPE ? [thirdFilter] : [])
});
expect(spectator.service.getGlobalFilters(SPAN_SCOPE)).toEqual([firstFilter, thirdFilter]);
});

test('allows removing global filter rules', () => {
const spectator = createService();
const filterRuleKey = Symbol('first');
spectator.service.setGlobalFilterRule(filterRuleKey, {
filtersForScope: scope => (scope === SPAN_SCOPE ? [firstFilter] : [])
});

expect(spectator.service.getGlobalFilters(SPAN_SCOPE)).toEqual([firstFilter]);

spectator.service.clearGlobalFilterRule(filterRuleKey);
expect(spectator.service.getGlobalFilters(SPAN_SCOPE)).toEqual([]);
});

test('merges with local filters', () => {
const spectator = createService();
spectator.service.setGlobalFilterRule(Symbol('first'), {
filtersForScope: scope => (scope === SPAN_SCOPE ? [firstFilter] : [])
});

expect(spectator.service.mergeGlobalFilters(SPAN_SCOPE)).toEqual([firstFilter]);

expect(spectator.service.mergeGlobalFilters('fake', [secondFilter])).toEqual([secondFilter]);
expect(spectator.service.mergeGlobalFilters(SPAN_SCOPE, [secondFilter])).toEqual([secondFilter, firstFilter]);
});
});
27 changes: 27 additions & 0 deletions ...stributed-tracing/src/shared/graphql/model/schema/filter/global-graphql-filter.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { Injectable } from '@angular/core';
import { GraphQlFilter } from './graphql-filter';

@Injectable({ providedIn: 'root' })
export class GlobalGraphQlFilterService {
private readonly filterRules: Map<symbol, FilterRule> = new Map();

public getGlobalFilters(scope: string): GraphQlFilter[] {
return Array.from(this.filterRules.values()).flatMap(rule => rule.filtersForScope(scope));
}

public mergeGlobalFilters(scope: string, localFilters: GraphQlFilter[] = []): GraphQlFilter[] {
return [...localFilters, ...this.getGlobalFilters(scope)];
}

public setGlobalFilterRule(ruleKey: symbol, rule: FilterRule): void {
this.filterRules.set(ruleKey, rule);
}

public clearGlobalFilterRule(ruleKey: symbol): void {
this.filterRules.delete(ruleKey);
}
}

interface FilterRule {
filtersForScope(scope: string): GraphQlFilter[];
}
8 changes: 6 additions & 2 deletions ...-tracing/src/shared/graphql/request/handlers/spans/spans-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { GraphQlHandlerType, GraphQlQueryHandler, GraphQlSelection } from '@hype
import { Observable } from 'rxjs';
import { map } from 'rxjs/operators';
import { MetadataService } from '../../../../services/metadata/metadata.service';
import { GlobalGraphQlFilterService } from '../../../model/schema/filter/global-graphql-filter.service';
import { GraphQlFilter } from '../../../model/schema/filter/graphql-filter';
import { GraphQlSortBySpecification } from '../../../model/schema/sort/graphql-sort-by-specification';
import { Span, spanIdKey, SPAN_SCOPE } from '../../../model/schema/span';
Expand All @@ -19,7 +20,10 @@ export class SpansGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
private readonly argBuilder: GraphQlArgumentBuilder = new GraphQlArgumentBuilder();
private readonly selectionBuilder: GraphQlSelectionBuilder = new GraphQlSelectionBuilder();

public constructor(private readonly metadataService: MetadataService) {}
public constructor(
private readonly metadataService: MetadataService,
private readonly globalGraphQlFilterService: GlobalGraphQlFilterService
) {}

public matchesRequest(request: unknown): request is GraphQlSpansRequest {
return (
Expand All @@ -37,7 +41,7 @@ export class SpansGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
this.argBuilder.forTimeRange(request.timeRange),
...this.argBuilder.forOffset(request.offset),
...this.argBuilder.forOrderBy(request.sort),
...this.argBuilder.forFilters(request.filters)
...this.argBuilder.forFilters(this.globalGraphQlFilterService.mergeGlobalFilters(SPAN_SCOPE, request.filters))
],
children: [
{
Expand Down
18 changes: 14 additions & 4 deletions ...tracing/src/shared/graphql/request/handlers/traces/trace-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@ import { Dictionary, TimeDuration, TimeRangeService, TimeUnit } from '@hypertrac
import { GraphQlHandlerType, GraphQlQueryHandler, GraphQlSelection } from '@hypertrace/graphql-client';
import { isEmpty, isNil } from 'lodash-es';
import { GraphQlFieldFilter } from '../../../model/schema/filter/field/graphql-field-filter';
import { GlobalGraphQlFilterService } from '../../../model/schema/filter/global-graphql-filter.service';
import { GraphQlFilter, GraphQlOperatorType } from '../../../model/schema/filter/graphql-filter';
import { GraphQlIdFilter } from '../../../model/schema/filter/id/graphql-id-filter';
import { Span, spanIdKey } from '../../../model/schema/span';
import { Span, spanIdKey, SPAN_SCOPE } from '../../../model/schema/span';
import { Specification } from '../../../model/schema/specifier/specification';
import { GraphQlTimeRange } from '../../../model/schema/timerange/graphql-time-range';
import { resolveTraceType, Trace, traceIdKey, TraceType, traceTypeKey } from '../../../model/schema/trace';
Expand All @@ -21,7 +22,10 @@ export class TraceGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
private readonly argBuilder: GraphQlArgumentBuilder = new GraphQlArgumentBuilder();
private readonly selectionBuilder: GraphQlSelectionBuilder = new GraphQlSelectionBuilder();

public constructor(private readonly timeRangeService: TimeRangeService) {}
public constructor(
private readonly timeRangeService: TimeRangeService,
private readonly globalGraphQlFilterService: GlobalGraphQlFilterService
) {}

public matchesRequest(request: unknown): request is GraphQlTraceRequest {
return (
Expand All @@ -40,7 +44,11 @@ export class TraceGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
this.argBuilder.forTraceType(resolveTraceType(request.traceType)),
this.argBuilder.forLimit(1),
this.argBuilder.forTimeRange(timeRange),
...this.argBuilder.forFilters([this.buildTraceIdFilter(request)])
...this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(resolveTraceType(request.traceType), [
this.buildTraceIdFilter(request)
])
)
],
children: [
{
Expand All @@ -58,7 +66,9 @@ export class TraceGraphQlQueryHandlerService implements GraphQlQueryHandler<Grap
arguments: [
this.argBuilder.forLimit(request.spanLimit),
this.argBuilder.forTimeRange(timeRange),
...this.argBuilder.forFilters([...this.buildSpansFilter(request)])
...this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(SPAN_SCOPE, this.buildSpansFilter(request))
)
],
children: [
{
Expand Down
10 changes: 8 additions & 2 deletions ...racing/src/shared/graphql/request/handlers/traces/traces-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { GraphQlHandlerType, GraphQlQueryHandler, GraphQlSelection } from '@hype
import { Observable } from 'rxjs';
import { map } from 'rxjs/operators';
import { MetadataService } from '../../../../services/metadata/metadata.service';
import { GlobalGraphQlFilterService } from '../../../model/schema/filter/global-graphql-filter.service';
import { GraphQlFilter } from '../../../model/schema/filter/graphql-filter';
import { GraphQlSortBySpecification } from '../../../model/schema/sort/graphql-sort-by-specification';
import { Specification } from '../../../model/schema/specifier/specification';
Expand All @@ -18,7 +19,10 @@ export class TracesGraphQlQueryHandlerService implements GraphQlQueryHandler<Gra
private readonly selectionBuilder: GraphQlSelectionBuilder = new GraphQlSelectionBuilder();
public readonly type: GraphQlHandlerType.Query = GraphQlHandlerType.Query;

public constructor(private readonly metadataService: MetadataService) {}
public constructor(
private readonly metadataService: MetadataService,
private readonly globalGraphQlFilterService: GlobalGraphQlFilterService
) {}

public matchesRequest(request: unknown): request is GraphQlTracesRequest {
return (
Expand All @@ -37,7 +41,9 @@ export class TracesGraphQlQueryHandlerService implements GraphQlQueryHandler<Gra
this.argBuilder.forTimeRange(request.timeRange),
...this.argBuilder.forOffset(request.offset),
...this.argBuilder.forOrderBy(request.sort),
...this.argBuilder.forFilters(request.filters)
...this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(resolveTraceType(request.traceType), request.filters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting this everywhere doesn't seem too scalable and may be error prone. Can we add it in idk GraphQlQueryEventService may be so that the inherited filters already have the desired filter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't love it, but it seems like the best place we have right now. There's no place with central knowledge of all queries, by design - and filters aren't a concept that applies to every request in the same way. QueryEventService for example pushes filters down to the data sources to apply, which is even more decentralized (and only works in the dashboard code) - the service itself doesn't know the request shapes to provide them.

Open to other ideas, but I think the handlers are the narrowest (and most stable point) that still has the context on how to apply the filters without a larger redesign.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then can we do it using an abstract method which the concrete handler has to provide? I could see people easily missing to apply the right global filters in new handlers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wouldn't work very well as an abstract method. The transformation depends on the shape of the request, the logic of the handler and the shape of the constructed query. We could make a util, but at that point we're basically wrapping this.globalGraphQlFilterService.mergeGlobalFilters with another function, so no real purpose. Also, there's no base class for the handlers (and if there were, we wouldn't want to add an injection dependency to it for something that's only applicable to certain handlers).

Copy link
Contributor

Choose a reason for hiding this comment

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

okay. I was thinking we could just define a function below. While it doesn't do much, it would just force the dev to consider the case of global filter application. Otherwise it is easy to miss. It is still a hack.

export interface GraphQlHandler<TRequest, TResponse> {
  readonly type: GraphQlHandlerType;

  matchesRequest(request: unknown): request is TRequest;
  /**
   * Converts the provided request into a single selection, or a map of selections with arbitrary keys.
   * If a map is provided, the response provided to the converter will use the same keys to disambiguate
   * the response for each selection.
   */
  convertRequest(request: TRequest): GraphQlSelection | Map<unknown, GraphQlSelection>;
  /**
   * Converts the provided response, or map of responses (if a map of selections were provided in the request
   * conversion), to a response object.
   */
  convertResponse(response: unknown | Map<unknown, unknown>, request: TRequest): TResponse | Observable<TResponse>;

  getRequestOptions?(request: TRequest): GraphQlRequestOptions;
}

We do need a better solution for this.

)
],
children: [
{
Expand Down
10 changes: 7 additions & 3 deletions .../shared/graphql/request/handlers/entities/query/entities-graphql-query-builder.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Inject, Injectable } from '@angular/core';
import { Dictionary, forkJoinSafeEmpty } from '@hypertrace/common';
import {
GlobalGraphQlFilterService,
GraphQlFilter,
GraphQlSelectionBuilder,
GraphQlSortBySpecification,
Expand All @@ -27,7 +28,8 @@ export class EntitiesGraphqlQueryBuilderService {

public constructor(
private readonly metadataService: MetadataService,
@Inject(ENTITY_METADATA) private readonly entityMetetadata: EntityMetadataMap
private readonly globalGraphQlFilterService: GlobalGraphQlFilterService,
@Inject(ENTITY_METADATA) private readonly entityMetadata: EntityMetadataMap
) {}

public buildRequestArguments(request: GraphQlEntitiesRequest): GraphQlArgument[] {
Expand All @@ -42,7 +44,9 @@ export class EntitiesGraphqlQueryBuilderService {
}

protected buildFilters(request: GraphQlEntitiesRequest): GraphQlArgument[] {
return this.argBuilder.forFilters(request.filters);
return this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(request.entityType, request.filters)
);
}

public buildRequestSpecifications(request: GraphQlEntitiesRequest): GraphQlSelection[] {
Expand Down Expand Up @@ -82,7 +86,7 @@ export class EntitiesGraphqlQueryBuilderService {
}

public getRequestOptions(request: Pick<GraphQlEntitiesRequest, 'entityType'>): GraphQlRequestOptions {
if (this.entityMetetadata.get(request.entityType)?.volatile) {
if (this.entityMetadata.get(request.entityType)?.volatile) {
return { cacheability: GraphQlRequestCacheability.NotCacheable };
}

Expand Down
14 changes: 9 additions & 5 deletions ...st/handlers/entities/query/topology/entity-topology-graphql-query-handler.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { FixedTimeRange, isEqualIgnoreFunctions } from '@hypertrace/common';
import { GraphQlTimeRange, MetricAggregationType, MetricHealth } from '@hypertrace/distributed-tracing';
import { GraphQlEnumArgument } from '@hypertrace/graphql-client';
import { createServiceFactory } from '@ngneat/spectator/jest';
import { entityIdKey, entityTypeKey, ObservabilityEntityType } from '../../../../../model/schema/entity';
import { GraphQlIntervalUnit } from '../../../../../model/schema/interval/graphql-interval-unit';
import { ObservabilitySpecificationBuilder } from '../../../../builders/selections/observability-specification-builder';
Expand All @@ -15,7 +16,7 @@ import {

// tslint:disable: max-file-line-count
describe('Entity topology graphql query handler', () => {
const service = new EntityTopologyGraphQlQueryHandlerService();
const createService = createServiceFactory({ service: EntityTopologyGraphQlQueryHandlerService });

const testTimeRange = GraphQlTimeRange.fromTimeRange(
new FixedTimeRange(new Date(1568907645141), new Date(1568911245141))
Expand Down Expand Up @@ -229,14 +230,16 @@ describe('Entity topology graphql query handler', () => {
});

test('only matches topology request', () => {
expect(service.matchesRequest(buildTopologyRequest())).toBe(true);
expect(service.matchesRequest({ requestType: 'other' })).toBe(false);
const spectator = createService();
expect(spectator.service.matchesRequest(buildTopologyRequest())).toBe(true);
expect(spectator.service.matchesRequest({ requestType: 'other' })).toBe(false);
});

test('builds expected request', () => {
const spectator = createService();
const request = buildTopologyRequest();

expect(service.convertRequest(request)).toEqual({
expect(spectator.service.convertRequest(request)).toEqual({
path: 'entities',
arguments: [
{ name: 'type', value: new GraphQlEnumArgument(ObservabilityEntityType.Service) },
Expand Down Expand Up @@ -413,6 +416,7 @@ describe('Entity topology graphql query handler', () => {
});
});
test('correctly parses response', () => {
const spectator = createService();
const request = buildTopologyRequest();
const serverResponse = buildTopologyResponse();

Expand Down Expand Up @@ -544,7 +548,7 @@ describe('Entity topology graphql query handler', () => {
serviceNode3.edges.push(service3ToBackendBEdge);
backendNodeB.edges.push(service3ToBackendBEdge);

const actual = service.convertResponse(serverResponse, request);
const actual = spectator.service.convertResponse(serverResponse, request);
const expected = [serviceNode1, backendNodeA, serviceNode2, serviceNode3, backendNodeB];
// Custom equality checking because built in doesn't do well with circular references + functions (which are checked by reference)
expect(isEqualIgnoreFunctions(actual, expected)).toBe(true);
Expand Down
6 changes: 5 additions & 1 deletion ...request/handlers/entities/query/topology/entity-topology-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable } from '@angular/core';
import { Dictionary } from '@hypertrace/common';
import {
GlobalGraphQlFilterService,
GraphQlFilter,
GraphQlSelectionBuilder,
GraphQlTimeRange,
Expand All @@ -22,6 +23,7 @@ export class EntityTopologyGraphQlQueryHandlerService
private readonly specBuilder: SpecificationBuilder = new SpecificationBuilder();
private readonly argBuilder: GraphQlObservabilityArgumentBuilder = new GraphQlObservabilityArgumentBuilder();
private readonly selectionBuilder: GraphQlSelectionBuilder = new GraphQlSelectionBuilder();
public constructor(private readonly globalGraphQlFilterService: GlobalGraphQlFilterService) {}

public matchesRequest(request: unknown): request is GraphQlEntityTopologyRequest {
return (
Expand All @@ -38,7 +40,9 @@ export class EntityTopologyGraphQlQueryHandlerService
this.argBuilder.forEntityType(request.rootNodeType),
this.argBuilder.forLimit(request.rootNodeLimit),
this.argBuilder.forTimeRange(request.timeRange),
...this.argBuilder.forFilters(request.rootNodeFilters)
...this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(request.rootNodeType, request.rootNodeFilters)
)
],
children: [
{
Expand Down
7 changes: 6 additions & 1 deletion ...lity/src/shared/graphql/request/handlers/explore/explore-graphql-query-handler.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable } from '@angular/core';
import { DateCoercer, Dictionary, TimeDuration } from '@hypertrace/common';
import {
GlobalGraphQlFilterService,
GraphQlFilter,
GraphQlSelectionBuilder,
GraphQlSortBySpecification,
Expand Down Expand Up @@ -29,6 +30,8 @@ export class ExploreGraphQlQueryHandlerService
private readonly specBuilder: ExploreSpecificationBuilder = new ExploreSpecificationBuilder();
private readonly dateCoercer: DateCoercer = new DateCoercer();

public constructor(private readonly globalGraphQlFilterService: GlobalGraphQlFilterService) {}

public matchesRequest(request: unknown): request is GraphQlExploreRequest {
return (
typeof request === 'object' &&
Expand All @@ -51,7 +54,9 @@ export class ExploreGraphQlQueryHandlerService
this.argBuilder.forTimeRange(request.timeRange),
...this.argBuilder.forOffset(request.offset),
...this.argBuilder.forInterval(request.interval),
...this.argBuilder.forFilters(request.filters),
...this.argBuilder.forFilters(
this.globalGraphQlFilterService.mergeGlobalFilters(request.context, request.filters)
),
...this.argBuilder.forGroupBy(request.groupBy),
...this.argBuilder.forOrderBys(request.orderBy)
],
Expand Down