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.

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

Loki: Display error with label filter conflicts #63109

Merged
merged 22 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
43e5d7d
feat: add logic to check for conflicting operator
gwdawson Feb 8, 2023
a41d5a6
feat: display error if there is a conflict
gwdawson Feb 8, 2023
f25ae1a
feat: check if label filter conflicts with other filters
gwdawson Feb 8, 2023
320f10a
fix: remove capitalisation from "no data"
gwdawson Feb 8, 2023
87f8314
test: add tests for isConflictingFilter function
gwdawson Feb 9, 2023
a5e1dfe
feat(labels): add validation for label matchers
gwdawson Feb 9, 2023
f258257
test(labels): add tests for isConflictingSelector
gwdawson Feb 9, 2023
5532333
refactor(labels): add suggestions from code review
gwdawson Feb 10, 2023
1b3d57d
test(labels): add case for labels without op
gwdawson Feb 10, 2023
7747537
refactor(operations): add suggestions from code review
gwdawson Feb 10, 2023
8ce4433
feat(operations): display tooltip when you have conflicts
gwdawson Feb 10, 2023
aa449ea
fix: remove unused props from test
gwdawson Feb 10, 2023
0cdd961
fix: remove old test
gwdawson Feb 10, 2023
a955e07
fix(labels): error message now displays in the correct location
gwdawson Feb 20, 2023
db90425
fix(operations): operations can be dragged, even with error
gwdawson Feb 20, 2023
e02affd
fix: remove unused vars
gwdawson Feb 20, 2023
d09bc78
Merge branch 'main' into gareth/conflicting-operations
gwdawson Feb 20, 2023
2284f77
fix: failing tests
gwdawson Feb 20, 2023
bc4c552
fix(operations): hide error message whilst dragging
gwdawson Feb 21, 2023
5c78e1e
refactor: use more appropriate variable naming
gwdawson Feb 21, 2023
9e59894
refactor: add suggestions from code review
gwdawson Feb 21, 2023
d45debf
perf: use array.some instead of array.find
gwdawson Feb 21, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
createRangeOperation,
createRangeOperationWithGrouping,
getLineFilterRenderer,
isConflictingFilter,
labelFilterRenderer,
} from './operationUtils';
import { LokiVisualQueryOperationCategory } from './types';
Expand Down Expand Up @@ -184,3 +185,23 @@ describe('labelFilterRenderer', () => {
);
});
});

describe('isConflictingFilter', () => {
it('should return true if the operation conflict with another label filter', () => {
const MOCK_OPERATION = { id: '__label_filter', params: ['abc', '!=', '123'] };
gwdawson marked this conversation as resolved.
Show resolved Hide resolved
const MOCK_QUERY_OPERATIONS = [
{ id: '__label_filter', params: ['abc', '=', '123'] },
{ id: '__label_filter', params: ['abc', '!=', '123'] },
];
expect(isConflictingFilter(MOCK_OPERATION, MOCK_QUERY_OPERATIONS)).toBe(true);
});

it("should return false if the operation doesn't conflict with another label filter", () => {
const MOCK_OPERATION = { id: '__label_filter', params: ['abc', '=', '123'] };
const MOCK_QUERY_OPERATIONS = [
{ id: '__label_filter', params: ['abc', '=', '123'] },
{ id: '__label_filter', params: ['abc', '=', '123'] },
];
expect(isConflictingFilter(MOCK_OPERATION, MOCK_QUERY_OPERATIONS)).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,33 @@ export function labelFilterRenderer(model: QueryBuilderOperation, def: QueryBuil
return `${innerExpr} | ${model.params[0]} ${model.params[1]} \`${model.params[2]}\``;
}

export function isConflictingFilter(
operation: QueryBuilderOperation,
queryOperations: QueryBuilderOperation[]
): boolean {
let conflictingFilter = false;

const labelFilters = queryOperations.filter((op) => {
return op.id === LokiOperationId.LabelFilter && op.params !== operation.params;
});

// check if the new filter conflicts with any other label filter
labelFilters.forEach((filter) => {
if (operation.params[0] !== filter.params[0] || operation.params[2] !== filter.params[2]) {
return;
}

if (
(String(operation.params[1]).startsWith('!') && !String(filter.params[1]).startsWith('!')) ||
(String(filter.params[1]).startsWith('!') && !String(operation.params[1]).startsWith('!'))
) {
conflictingFilter = true;
}
});

return conflictingFilter;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative approach to consider.

Suggested change
export function isConflictingFilter(
operation: QueryBuilderOperation,
queryOperations: QueryBuilderOperation[]
): boolean {
let conflictingFilter = false;
const labelFilters = queryOperations.filter((op) => {
return op.id === LokiOperationId.LabelFilter && op.params !== operation.params;
});
// check if the new filter conflicts with any other label filter
labelFilters.forEach((filter) => {
if (operation.params[0] !== filter.params[0] || operation.params[2] !== filter.params[2]) {
return;
}
if (
(String(operation.params[1]).startsWith('!') && !String(filter.params[1]).startsWith('!')) ||
(String(filter.params[1]).startsWith('!') && !String(operation.params[1]).startsWith('!'))
) {
conflictingFilter = true;
}
});
return conflictingFilter;
}
export function isConflictingFilter(
operation: QueryBuilderOperation,
queryOperations: QueryBuilderOperation[]
): boolean {
const operationIsNegative = operation.params[1].toString().startsWith('!');
// Discard non label filters and different params
const candidates = queryOperations.filter(queryOperation => (queryOperation.id === LokiOperationId.LabelFilter && queryOperation.params[0] === operation.params[0] && queryOperation.params[2] === operation.params[2]));
const conflict = candidates.find((candidate) => {
if (operationIsNegative && candidate.params[1].toString().startsWith('!') === false) {
return true;
}
if (operationIsNegative === false && candidate.params[1].toString().startsWith('!')) {
return true;
}
return false;
});
return conflict !== undefined;
}

A limitation of the current implementation is that it will continue iterating over all the label filters, even if one has already been found in line 180. I would suggest to change the forEach to a find or findIndex.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess some would also be possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Definitely. And it looks like it's potentially faster https://www.measurethat.net/Benchmarks/Show/4337/0/array-find-vs-some

Copy link
Member Author

Choose a reason for hiding this comment

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

@matyax cool website, based off the fact some is faster, ill update to use that.


gwdawson marked this conversation as resolved.
Show resolved Hide resolved
export function pipelineRenderer(model: QueryBuilderOperation, def: QueryBuilderOperationDef, innerExpr: string) {
return `${innerExpr} | ${model.id}`;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import React, { useState } from 'react';
import { SelectableValue, toOption } from '@grafana/data';
import { selectors } from '@grafana/e2e-selectors';
import { AccessoryButton, InputGroup } from '@grafana/experimental';
import { Select } from '@grafana/ui';
import { InlineField, Select } from '@grafana/ui';

import { isConflictingSelector } from './operationUtils';
import { QueryBuilderLabelFilter } from './types';

export interface Props {
defaultOp: string;
item: Partial<QueryBuilderLabelFilter>;
items: Array<Partial<QueryBuilderLabelFilter>>;
onChange: (value: QueryBuilderLabelFilter) => void;
onGetLabelNames: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
onGetLabelValues: (forLabel: Partial<QueryBuilderLabelFilter>) => Promise<SelectableValue[]>;
Expand All @@ -21,6 +23,7 @@ export interface Props {

export function LabelFilterItem({
item,
items,
defaultOp,
onChange,
onDelete,
Expand All @@ -35,6 +38,7 @@ export function LabelFilterItem({
isLoadingLabelNames?: boolean;
isLoadingLabelValues?: boolean;
}>({});
const CONFLICTING_LABEL_FILTER_ERROR_MESSAGE = 'You have conflicting label filters';
gwdawson marked this conversation as resolved.
Show resolved Hide resolved

const isMultiSelect = (operator = item.op) => {
return operators.find((op) => op.label === operator)?.isMultiValue;
Expand All @@ -58,94 +62,99 @@ export function LabelFilterItem({
return uniqBy([...selectedOptions, ...labelValues], 'value');
};

const isConflicting = isConflictingSelector(item, items);

return (
<div data-testid="prometheus-dimensions-filter-item">
<InputGroup>
<Select
placeholder="Select label"
aria-label={selectors.components.QueryBuilder.labelSelect}
inputId="prometheus-dimensions-filter-item-key"
width="auto"
value={item.label ? toOption(item.label) : null}
allowCustomValue
onOpenMenu={async () => {
setState({ isLoadingLabelNames: true });
const labelNames = await onGetLabelNames(item);
setState({ labelNames, isLoadingLabelNames: undefined });
}}
isLoading={state.isLoadingLabelNames}
options={state.labelNames}
onChange={(change) => {
if (change.label) {
onChange({
...item,
op: item.op ?? defaultOp,
label: change.label,
} as unknown as QueryBuilderLabelFilter);
}
}}
invalid={invalidLabel}
/>
<InlineField error={CONFLICTING_LABEL_FILTER_ERROR_MESSAGE} invalid={isConflicting ? true : undefined}>
<InputGroup>
<Select
placeholder="Select label"
aria-label={selectors.components.QueryBuilder.labelSelect}
inputId="prometheus-dimensions-filter-item-key"
width="auto"
value={item.label ? toOption(item.label) : null}
allowCustomValue
onOpenMenu={async () => {
setState({ isLoadingLabelNames: true });
const labelNames = await onGetLabelNames(item);
setState({ labelNames, isLoadingLabelNames: undefined });
}}
isLoading={state.isLoadingLabelNames}
options={state.labelNames}
onChange={(change) => {
if (change.label) {
onChange({
...item,
op: item.op ?? defaultOp,
label: change.label,
} as unknown as QueryBuilderLabelFilter);
}
}}
invalid={isConflicting || invalidLabel}
/>

<Select
aria-label={selectors.components.QueryBuilder.matchOperatorSelect}
value={toOption(item.op ?? defaultOp)}
options={operators}
width="auto"
onChange={(change) => {
if (change.value != null) {
onChange({
...item,
op: change.value,
value: isMultiSelect(change.value) ? item.value : getSelectOptionsFromString(item?.value)[0],
} as unknown as QueryBuilderLabelFilter);
}
}}
/>
<Select
aria-label={selectors.components.QueryBuilder.matchOperatorSelect}
value={toOption(item.op ?? defaultOp)}
options={operators}
width="auto"
onChange={(change) => {
if (change.value != null) {
onChange({
...item,
op: change.value,
value: isMultiSelect(change.value) ? item.value : getSelectOptionsFromString(item?.value)[0],
} as unknown as QueryBuilderLabelFilter);
}
}}
invalid={isConflicting}
/>

<Select
placeholder="Select value"
aria-label={selectors.components.QueryBuilder.valueSelect}
inputId="prometheus-dimensions-filter-item-value"
width="auto"
value={
isMultiSelect()
? getSelectOptionsFromString(item?.value).map(toOption)
: getSelectOptionsFromString(item?.value).map(toOption)[0]
}
allowCustomValue
onOpenMenu={async () => {
setState({ isLoadingLabelValues: true });
const labelValues = await onGetLabelValues(item);
setState({
...state,
labelValues,
isLoadingLabelValues: undefined,
});
}}
isMulti={isMultiSelect()}
isLoading={state.isLoadingLabelValues}
options={getOptions()}
onChange={(change) => {
if (change.value) {
onChange({
...item,
value: change.value,
op: item.op ?? defaultOp,
} as unknown as QueryBuilderLabelFilter);
} else {
const changes = change
.map((change: any) => {
return change.label;
})
.join('|');
onChange({ ...item, value: changes, op: item.op ?? defaultOp } as unknown as QueryBuilderLabelFilter);
<Select
placeholder="Select value"
aria-label={selectors.components.QueryBuilder.valueSelect}
inputId="prometheus-dimensions-filter-item-value"
width="auto"
value={
isMultiSelect()
? getSelectOptionsFromString(item?.value).map(toOption)
: getSelectOptionsFromString(item?.value).map(toOption)[0]
}
}}
invalid={invalidValue}
/>
<AccessoryButton aria-label="remove" icon="times" variant="secondary" onClick={onDelete} />
</InputGroup>
allowCustomValue
onOpenMenu={async () => {
setState({ isLoadingLabelValues: true });
const labelValues = await onGetLabelValues(item);
setState({
...state,
labelValues,
isLoadingLabelValues: undefined,
});
}}
isMulti={isMultiSelect()}
isLoading={state.isLoadingLabelValues}
options={getOptions()}
onChange={(change) => {
if (change.value) {
onChange({
...item,
value: change.value,
op: item.op ?? defaultOp,
} as unknown as QueryBuilderLabelFilter);
} else {
const changes = change
.map((change: any) => {
return change.label;
})
.join('|');
onChange({ ...item, value: changes, op: item.op ?? defaultOp } as unknown as QueryBuilderLabelFilter);
}
}}
invalid={isConflicting || invalidValue}
/>
<AccessoryButton aria-label="remove" icon="times" variant="secondary" onClick={onDelete} />
</InputGroup>
</InlineField>
</div>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export function LabelFilters({
renderItem={(item: Partial<QueryBuilderLabelFilter>, onChangeItem, onDelete) => (
<LabelFilterItem
item={item}
items={items}
defaultOp={defaultOp}
onChange={onChangeItem}
onDelete={onDelete}
Expand Down
Loading