Loki: Display error with label filter conflicts (#63109)

* feat: add logic to check for conflicting operator * feat: display error if there is a conflict * feat: check if label filter conflicts with other filters * fix: remove capitalisation from "no data" * test: add tests for isConflictingFilter function * feat(labels): add validation for label matchers * test(labels): add tests for isConflictingSelector * refactor(labels): add suggestions from code review * test(labels): add case for labels without op * refactor(operations): add suggestions from code review * feat(operations): display tooltip when you have conflicts * fix: remove unused props from test * fix: remove old test * fix(labels): error message now displays in the correct location * fix(operations): operations can be dragged, even with error * fix: remove unused vars * fix: failing tests * fix(operations): hide error message whilst dragging * refactor: use more appropriate variable naming * refactor: add suggestions from code review * perf: use array.some instead of array.find
2025-02-25 18:55:37 -06:00 · 2023-02-22 09:56:20 +00:00 · 2023-02-22 09:56:20 +00:00 · c5ed9391b6
commit c5ed9391b6
parent 3a57304ef0
7 changed files with 264 additions and 110 deletions
--- a/public/app/plugins/datasource/loki/querybuilder/operationUtils.test.ts
+++ b/public/app/plugins/datasource/loki/querybuilder/operationUtils.test.ts
@ -4,6 +4,7 @@ import {
  createRangeOperation,
  createRangeOperationWithGrouping,
  getLineFilterRenderer,
+  isConflictingFilter,
  labelFilterRenderer,
 } from './operationUtils';
 import { LokiVisualQueryOperationCategory } from './types';
@ -184,3 +185,23 @@ describe('labelFilterRenderer', () => {
    );
  });
 });
+
+describe('isConflictingFilter', () => {
+  it('should return true if the operation conflict with another label filter', () => {
+    const operation = { id: '__label_filter', params: ['abc', '!=', '123'] };
+    const queryOperations = [
+      { id: '__label_filter', params: ['abc', '=', '123'] },
+      { id: '__label_filter', params: ['abc', '!=', '123'] },
+    ];
+    expect(isConflictingFilter(operation, queryOperations)).toBe(true);
+  });
+
+  it("should return false if the operation doesn't conflict with another label filter", () => {
+    const operation = { id: '__label_filter', params: ['abc', '=', '123'] };
+    const queryOperations = [
+      { id: '__label_filter', params: ['abc', '=', '123'] },
+      { id: '__label_filter', params: ['abc', '=', '123'] },
+    ];
+    expect(isConflictingFilter(operation, queryOperations)).toBe(false);
+  });
+});
--- a/public/app/plugins/datasource/loki/querybuilder/operationUtils.ts
+++ b/public/app/plugins/datasource/loki/querybuilder/operationUtils.ts
@ -157,6 +157,32 @@ 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 {
+  const operationIsNegative = operation.params[1].toString().startsWith('!');
+
+  const candidates = queryOperations.filter(
+    (queryOperation) =>
+      queryOperation.id === LokiOperationId.LabelFilter &&
+      queryOperation.params[0] === operation.params[0] &&
+      queryOperation.params[2] === operation.params[2]
+  );
+
+  const conflict = candidates.some((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;
+}
+
 export function pipelineRenderer(model: QueryBuilderOperation, def: QueryBuilderOperationDef, innerExpr: string) {
  return `${innerExpr} | ${model.id}`;
 }
--- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx
+++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilterItem.tsx
@ -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[]>;
@ -21,6 +23,7 @@ export interface Props {

 export function LabelFilterItem({
  item,
+  items,
  defaultOp,
  onChange,
  onDelete,
@ -35,6 +38,7 @@ export function LabelFilterItem({
    isLoadingLabelNames?: boolean;
    isLoadingLabelValues?: boolean;
  }>({});
+  const CONFLICTING_LABEL_FILTER_ERROR_MESSAGE = 'You have conflicting label filters';

  const isMultiSelect = (operator = item.op) => {
    return operators.find((op) => op.label === operator)?.isMultiValue;
@ -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>
  );
 }
--- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx
+++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/LabelFilters.tsx
@ -62,6 +62,7 @@ export function LabelFilters({
          renderItem={(item: Partial<QueryBuilderLabelFilter>, onChangeItem, onDelete) => (
            <LabelFilterItem
              item={item}
+              items={items}
              defaultOp={defaultOp}
              onChange={onChangeItem}
              onDelete={onDelete}
--- a/public/app/plugins/datasource/prometheus/querybuilder/shared/OperationEditor.tsx
+++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/OperationEditor.tsx
@ -4,7 +4,9 @@ import { Draggable } from 'react-beautiful-dnd';

 import { DataSourceApi, GrafanaTheme2 } from '@grafana/data';
 import { Stack } from '@grafana/experimental';
-import { Button, Icon, Tooltip, useStyles2 } from '@grafana/ui';
+import { Button, Icon, InlineField, Tooltip, useStyles2 } from '@grafana/ui';
+import { isConflictingFilter } from 'app/plugins/datasource/loki/querybuilder/operationUtils';
+import { LokiOperationId } from 'app/plugins/datasource/loki/querybuilder/types';

 import { OperationHeader } from './OperationHeader';
 import { getOperationParamEditor } from './OperationParamEditor';
@ -126,33 +128,56 @@ export function OperationEditor({
    }
  }

+  let isConflicting = false;
+  if (operation.id === LokiOperationId.LabelFilter) {
+    isConflicting = isConflictingFilter(operation, query.operations);
+  }
+
+  const isInvalid = (isDragging: boolean) => {
+    if (isDragging) {
+      return undefined;
+    }
+
+    return isConflicting ? true : undefined;
+  };
+
  return (
    <Draggable draggableId={`operation-${index}`} index={index}>
-      {(provided) => (
-        <div
-          className={cx(styles.card, (shouldFlash || highlight) && styles.cardHighlight)}
-          ref={provided.innerRef}
-          {...provided.draggableProps}
-          data-testid={`operations.${index}.wrapper`}
+      {(provided, snapshot) => (
+        <InlineField
+          error={'You have conflicting label filters'}
+          invalid={isInvalid(snapshot.isDragging)}
+          className={styles.error}
        >
-          <OperationHeader
-            operation={operation}
-            dragHandleProps={provided.dragHandleProps}
-            def={def}
-            index={index}
-            onChange={onChange}
-            onRemove={onRemove}
-            queryModeller={queryModeller}
-          />
-          <div className={styles.body}>{operationElements}</div>
-          {restParam}
-          {index < query.operations.length - 1 && (
-            <div className={styles.arrow}>
-              <div className={styles.arrowLine} />
-              <div className={styles.arrowArrow} />
-            </div>
-          )}
-        </div>
+          <div
+            className={cx(
+              styles.card,
+              (shouldFlash || highlight) && styles.cardHighlight,
+              isConflicting && styles.cardError
+            )}
+            ref={provided.innerRef}
+            {...provided.draggableProps}
+            data-testid={`operations.${index}.wrapper`}
+          >
+            <OperationHeader
+              operation={operation}
+              dragHandleProps={provided.dragHandleProps}
+              def={def}
+              index={index}
+              onChange={onChange}
+              onRemove={onRemove}
+              queryModeller={queryModeller}
+            />
+            <div className={styles.body}>{operationElements}</div>
+            {restParam}
+            {index < query.operations.length - 1 && (
+              <div className={styles.arrow}>
+                <div className={styles.arrowLine} />
+                <div className={styles.arrowArrow} />
+              </div>
+            )}
+          </div>
+        </InlineField>
      )}
    </Draggable>
  );
@ -220,6 +245,9 @@ function callParamChangedThenOnChange(

 const getStyles = (theme: GrafanaTheme2) => {
  return {
+    error: css({
+      marginBottom: theme.spacing(1),
+    }),
    card: css({
      background: theme.colors.background.primary,
      border: `1px solid ${theme.colors.border.medium}`,
@ -231,6 +259,10 @@ const getStyles = (theme: GrafanaTheme2) => {
      position: 'relative',
      transition: 'all 0.5s ease-in 0s',
    }),
+    cardError: css({
+      boxShadow: `0px 0px 4px 0px ${theme.colors.warning.main}`,
+      border: `1px solid ${theme.colors.warning.main}`,
+    }),
    cardHighlight: css({
      boxShadow: `0px 0px 4px 0px ${theme.colors.primary.border}`,
      border: `1px solid ${theme.colors.primary.border}`,
--- a/public/app/plugins/datasource/prometheus/querybuilder/shared/operationUtils.test.ts
+++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/operationUtils.test.ts
@ -1,4 +1,8 @@
-import { createAggregationOperation, createAggregationOperationWithParam } from './operationUtils';
+import {
+  createAggregationOperation,
+  createAggregationOperationWithParam,
+  isConflictingSelector,
+} from './operationUtils';

 describe('createAggregationOperation', () => {
  it('returns correct aggregation definitions with overrides', () => {
@ -164,3 +168,32 @@ describe('createAggregationOperationWithParams', () => {
    ).toBe('test_aggregation by(source, place) ("5", rate({place="luna"} |= `` [5m]))');
  });
 });
+
+describe('isConflictingSelector', () => {
+  it('returns true if selector is conflicting', () => {
+    const newLabel = { label: 'job', op: '!=', value: 'tns/app' };
+    const labels = [
+      { label: 'job', op: '=', value: 'tns/app' },
+      { label: 'job', op: '!=', value: 'tns/app' },
+    ];
+    expect(isConflictingSelector(newLabel, labels)).toBe(true);
+  });
+
+  it('returns false if selector is not complete', () => {
+    const newLabel = { label: 'job', op: '', value: 'tns/app' };
+    const labels = [
+      { label: 'job', op: '=', value: 'tns/app' },
+      { label: 'job', op: '', value: 'tns/app' },
+    ];
+    expect(isConflictingSelector(newLabel, labels)).toBe(false);
+  });
+
+  it('returns false if selector is not conflicting', () => {
+    const newLabel = { label: 'host', op: '=', value: 'docker-desktop' };
+    const labels = [
+      { label: 'job', op: '=', value: 'tns/app' },
+      { label: 'host', op: '=', value: 'docker-desktop' },
+    ];
+    expect(isConflictingSelector(newLabel, labels)).toBe(false);
+  });
+});
--- a/public/app/plugins/datasource/prometheus/querybuilder/shared/operationUtils.ts
+++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/operationUtils.ts
@ -7,6 +7,7 @@ import { LabelParamEditor } from '../components/LabelParamEditor';
 import { PromVisualQueryOperationCategory } from '../types';

 import {
+  QueryBuilderLabelFilter,
  QueryBuilderOperation,
  QueryBuilderOperationDef,
  QueryBuilderOperationParamDef,
@ -321,3 +322,34 @@ export function getOnLabelAddedHandler(changeToOperationId: string) {
    return op;
  };
 }
+
+export function isConflictingSelector(
+  newLabel: Partial<QueryBuilderLabelFilter>,
+  labels: Array<Partial<QueryBuilderLabelFilter>>
+): boolean {
+  if (!newLabel.label || !newLabel.op || !newLabel.value) {
+    return false;
+  }
+
+  if (labels.length < 2) {
+    return false;
+  }
+
+  const operationIsNegative = newLabel.op.toString().startsWith('!');
+
+  const candidates = labels.filter(
+    (label) => label.label === newLabel.label && label.value === newLabel.value && label.op !== newLabel.op
+  );
+
+  const conflict = candidates.some((candidate) => {
+    if (operationIsNegative && candidate?.op?.toString().startsWith('!') === false) {
+      return true;
+    }
+    if (operationIsNegative === false && candidate?.op?.toString().startsWith('!')) {
+      return true;
+    }
+    return false;
+  });
+
+  return conflict;
+}