From 52ae586452d49d496ae274b0a8a0fb4db4074427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Torkel=20=C3=96degaard?= Date: Tue, 15 Feb 2022 21:05:35 +0100 Subject: [PATCH] Prometheus: Improvements to binary operations, nesting and parantheses handling (#45384) * Prometheus: Improve query nesting ux * Prometheus: Add parentheses around nested queries with binary ops * removed unnessary typing change * Fixing ts issues * Improved paranthesis logic * Fixing unit test * Progress --- .../querybuilder/PromQueryModeller.test.ts | 70 +++++++++++++++++++ .../querybuilder/PromQueryModeller.ts | 21 +++++- .../querybuilder/components/NestedQuery.tsx | 31 ++++---- .../components/NestedQueryList.tsx | 56 ++++----------- .../components/PromQueryBuilder.test.tsx | 1 - .../components/PromQueryBuilder.tsx | 6 +- .../shared/LokiAndPromQueryModellerBase.ts | 6 +- 7 files changed, 124 insertions(+), 67 deletions(-) diff --git a/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.test.ts b/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.test.ts index 170a1d7e21f..56a317419ba 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.test.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.test.ts @@ -188,6 +188,76 @@ describe('PromQueryModeller', () => { ).toBe('metric_a + metric_b + metric_c'); }); + it('Can render query with nested query with binary op', () => { + expect( + modeller.renderQuery({ + metric: 'metric_a', + labels: [], + operations: [], + binaryQueries: [ + { + operator: '/', + query: { + metric: 'metric_b', + labels: [], + operations: [{ id: PromOperationId.MultiplyBy, params: [1000] }], + }, + }, + ], + }) + ).toBe('metric_a / (metric_b * 1000)'); + }); + + it('Can render query with nested binary query with parentheses', () => { + expect( + modeller.renderQuery({ + metric: 'metric_a', + labels: [], + operations: [], + binaryQueries: [ + { + operator: '/', + query: { + metric: 'metric_b', + labels: [], + operations: [], + binaryQueries: [ + { + operator: '*', + query: { + metric: 'metric_c', + labels: [], + operations: [], + }, + }, + ], + }, + }, + ], + }) + ).toBe('metric_a / (metric_b * metric_c)'); + }); + + it('Should add parantheis around first query if it has binary op', () => { + expect( + modeller.renderQuery({ + metric: 'metric_a', + labels: [], + operations: [{ id: PromOperationId.MultiplyBy, params: [1000] }], + binaryQueries: [ + { + operator: '/', + query: { + metric: 'metric_b', + labels: [], + operations: [], + }, + }, + ], + }) + ).toBe('(metric_a * 1000) / metric_b'); + }); + it('Can render with binary queries with vectorMatches expression', () => { expect( modeller.renderQuery({ diff --git a/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.ts b/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.ts index 73cd0128d54..96ad85f530e 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.ts @@ -25,13 +25,32 @@ export class PromQueryModeller extends LokiAndPromQueryModellerBase { + const def = this.getOperationDef(op.id); + return def.category === PromVisualQueryOperationCategory.BinaryOps; + }) !== undefined + ); + } + getQueryPatterns(): PromQueryPattern[] { return [ { diff --git a/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQuery.tsx b/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQuery.tsx index e4f442b6a62..46f0e3cd5d0 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQuery.tsx +++ b/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQuery.tsx @@ -1,6 +1,6 @@ import { css } from '@emotion/css'; import { GrafanaTheme2, toOption } from '@grafana/data'; -import { FlexItem } from '@grafana/experimental'; +import { EditorRows, FlexItem } from '@grafana/experimental'; import { IconButton, Input, Select, useStyles2 } from '@grafana/ui'; import React from 'react'; import { PrometheusDatasource } from '../../datasource'; @@ -51,15 +51,17 @@ export const NestedQuery = React.memo(({ nestedQuery, index, datasource, onRemove(index)} />
- { - onChange(index, { ...nestedQuery, query: update }); - }} - /> + + { + onChange(index, { ...nestedQuery, query: update }); + }} + /> +
); @@ -79,15 +81,11 @@ NestedQuery.displayName = 'NestedQuery'; const getStyles = (theme: GrafanaTheme2) => { return { card: css({ - background: theme.colors.background.primary, - border: `1px solid ${theme.colors.border.medium}`, display: 'flex', flexDirection: 'column', - cursor: 'grab', - borderRadius: theme.shape.borderRadius(1), + gap: theme.spacing(0.5), }), header: css({ - borderBottom: `1px solid ${theme.colors.border.medium}`, padding: theme.spacing(0.5, 0.5, 0.5, 1), gap: theme.spacing(1), display: 'flex', @@ -97,8 +95,7 @@ const getStyles = (theme: GrafanaTheme2) => { whiteSpace: 'nowrap', }), body: css({ - margin: theme.spacing(1, 1, 0.5, 1), - display: 'table', + paddingLeft: theme.spacing(2), }), }; }; diff --git a/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQueryList.tsx b/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQueryList.tsx index e1aaa0bf943..dad37b8f1b3 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQueryList.tsx +++ b/public/app/plugins/datasource/prometheus/querybuilder/components/NestedQueryList.tsx @@ -1,6 +1,3 @@ -import { css } from '@emotion/css'; -import { GrafanaTheme2 } from '@grafana/data'; -import { useStyles2 } from '@grafana/ui'; import { Stack } from '@grafana/experimental'; import React from 'react'; import { PrometheusDatasource } from '../../datasource'; @@ -15,7 +12,6 @@ export interface Props { } export function NestedQueryList({ query, datasource, onChange, onRunQuery }: Props) { - const styles = useStyles2(getStyles); const nestedQueries = query.binaryQueries ?? []; const onNestedQueryUpdate = (index: number, update: PromVisualQueryBinary) => { @@ -30,44 +26,18 @@ export function NestedQueryList({ query, datasource, onChange, onRunQuery }: Pro }; return ( -
- -
Binary operations
- - {nestedQueries.map((nestedQuery, index) => ( - - ))} - - -
+ + {nestedQueries.map((nestedQuery, index) => ( + + ))} + ); } - -const getStyles = (theme: GrafanaTheme2) => { - return { - heading: css({ - fontSize: 12, - fontWeight: theme.typography.fontWeightMedium, - }), - body: css({ - width: '100%', - }), - connectingLine: css({ - height: '2px', - width: '16px', - backgroundColor: theme.colors.border.strong, - alignSelf: 'center', - }), - addOperation: css({ - paddingLeft: theme.spacing(2), - }), - }; -}; diff --git a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.test.tsx b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.test.tsx index 04f81e765c9..83745261467 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.test.tsx +++ b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.test.tsx @@ -63,7 +63,6 @@ describe('PromQueryBuilder', () => { expect(getByText(sumBys[0], 'job')).toBeInTheDocument(); expect(getByText(sumBys[1], 'app')).toBeInTheDocument(); - expect(screen.getByText('Binary operations')).toBeInTheDocument(); expect(screen.getByText('Operator')).toBeInTheDocument(); expect(screen.getByText('Vector matches')).toBeInTheDocument(); }); diff --git a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.tsx b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.tsx index ea52381ee3c..98b7d351c6c 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.tsx +++ b/public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.tsx @@ -105,11 +105,11 @@ export const PromQueryBuilder = React.memo(({ datasource, query, onChange onChange={onChange} onRunQuery={onRunQuery} /> - {query.binaryQueries && query.binaryQueries.length > 0 && ( - - )} + {query.binaryQueries && query.binaryQueries.length > 0 && ( + + )} ); }); diff --git a/public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts b/public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts index 8450274a1db..d2656d66632 100644 --- a/public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts +++ b/public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts @@ -61,10 +61,12 @@ export abstract class LokiAndPromQueryModellerBase) { let result = leftOperand + ` ${binaryQuery.operator} `; + if (binaryQuery.vectorMatches) { result += `${binaryQuery.vectorMatches} `; } - return result + `${this.renderQuery(binaryQuery.query)}`; + + return result + this.renderQuery(binaryQuery.query, true); } renderLabels(labels: QueryBuilderLabelFilter[]) { @@ -84,5 +86,5 @@ export abstract class LokiAndPromQueryModellerBase