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

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({

public/app/plugins/datasource/prometheus/querybuilder/PromQueryModeller.ts

@@ -25,13 +25,32 @@ export class PromQueryModeller extends LokiAndPromQueryModellerBase<PromVisualQu
     ]);
   }
 
-  renderQuery(query: PromVisualQuery) {
+  renderQuery(query: PromVisualQuery, nested?: boolean) {
     let queryString = `${query.metric}${this.renderLabels(query.labels)}`;
     queryString = this.renderOperations(queryString, query.operations);
+
+    if (!nested && this.hasBinaryOp(query) && Boolean(query.binaryQueries?.length)) {
+      queryString = `(${queryString})`;
+    }
+
     queryString = this.renderBinaryQueries(queryString, query.binaryQueries);
+
+    if (nested && (this.hasBinaryOp(query) || Boolean(query.binaryQueries?.length))) {
+      queryString = `(${queryString})`;
+    }
+
     return queryString;
   }
 
+  hasBinaryOp(query: PromVisualQuery): boolean {
+    return (
+      query.operations.find((op) => {
+        const def = this.getOperationDef(op.id);
+        return def.category === PromVisualQueryOperationCategory.BinaryOps;
+      }) !== undefined
+    );
+  }
+
   getQueryPatterns(): PromQueryPattern[] {
     return [
       {

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,6 +51,7 @@ export const NestedQuery = React.memo<Props>(({ nestedQuery, index, datasource,
         <IconButton name="times" size="sm" onClick={() => onRemove(index)} />
       </div>
       <div className={styles.body}>
+        <EditorRows>
           <PromQueryBuilder
             query={nestedQuery.query}
             datasource={datasource}
@@ -60,6 +61,7 @@ export const NestedQuery = React.memo<Props>(({ nestedQuery, index, datasource,
               onChange(index, { ...nestedQuery, query: update });
             }}
           />
+        </EditorRows>
       </div>
     </div>
   );
@@ -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',
+      gap: theme.spacing(0.5),
-      borderRadius: theme.shape.borderRadius(1),
     }),
     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),
+      paddingLeft: theme.spacing(2),
-      display: 'table',
     }),
   };
 };

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,10 +26,7 @@ export function NestedQueryList({ query, datasource, onChange, onRunQuery }: Pro
   };
 
   return (
-    <div className={styles.body}>
+    <Stack direction="column" gap={1}>
-      <Stack gap={1} direction="column">
-        <h5 className={styles.heading}>Binary operations</h5>
-        <Stack gap={1} direction="column">
       {nestedQueries.map((nestedQuery, index) => (
         <NestedQuery
           key={index.toString()}
@@ -46,28 +39,5 @@ export function NestedQueryList({ query, datasource, onChange, onRunQuery }: Pro
         />
       ))}
     </Stack>
-      </Stack>
-    </div>
   );
 }
-
-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),
-    }),
-  };
-};

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();
   });

public/app/plugins/datasource/prometheus/querybuilder/components/PromQueryBuilder.tsx

@@ -105,11 +105,11 @@ export const PromQueryBuilder = React.memo<Props>(({ datasource, query, onChange
           onChange={onChange}
           onRunQuery={onRunQuery}
         />
+        <PromQueryBuilderHints datasource={datasource} query={query} onChange={onChange} data={data} />
+      </OperationsEditorRow>
       {query.binaryQueries && query.binaryQueries.length > 0 && (
         <NestedQueryList query={query} datasource={datasource} onChange={onChange} onRunQuery={onRunQuery} />
       )}
-        <PromQueryBuilderHints datasource={datasource} query={query} onChange={onChange} data={data} />
-      </OperationsEditorRow>
     </>
   );
 });

public/app/plugins/datasource/prometheus/querybuilder/shared/LokiAndPromQueryModellerBase.ts

@@ -61,10 +61,12 @@ export abstract class LokiAndPromQueryModellerBase<T extends QueryWithOperations
 
   private renderBinaryQuery(leftOperand: string, binaryQuery: VisualQueryBinary<T>) {
     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<T extends QueryWithOperations
     return expr + `}`;
   }
 
-  abstract renderQuery(query: T): string;
+  abstract renderQuery(query: T, nested?: boolean): string;
 }