Browse Source

feat(ddm): Persistent query ids (#65406)

Assign persistent ids to queries, so they can be referenced in formulas.
ArthurKnaus 1 year ago
parent
commit
cd07c82686

+ 3 - 0
static/app/utils/metrics/constants.tsx

@@ -25,7 +25,10 @@ export const DEFAULT_SORT_STATE: SortState = {
   order: 'asc',
 };
 
+export const NO_QUERY_ID = -1;
+
 export const emptyWidget: MetricWidgetQueryParams = {
+  id: NO_QUERY_ID,
   mri: 'd:transactions/duration@millisecond' satisfies MRI,
   op: 'avg',
   query: '',

+ 2 - 1
static/app/utils/metrics/dashboard.tsx

@@ -1,7 +1,7 @@
 import {urlEncode} from '@sentry/utils';
 
 import type {MRI, PageFilters} from 'sentry/types';
-import {emptyWidget} from 'sentry/utils/metrics/constants';
+import {emptyWidget, NO_QUERY_ID} from 'sentry/utils/metrics/constants';
 import {MRIToField, parseField} from 'sentry/utils/metrics/mri';
 import type {MetricsQuery, MetricWidgetQueryParams} from 'sentry/utils/metrics/types';
 import {MetricDisplayType} from 'sentry/utils/metrics/types';
@@ -31,6 +31,7 @@ export function convertToMetricWidget(widget: Widget): MetricWidgetQueryParams {
   const parsed = parseField(query.aggregates[0]) || {mri: '' as MRI, op: ''};
 
   return {
+    id: NO_QUERY_ID,
     mri: parsed.mri,
     op: parsed.op,
     query: query.conditions,

+ 1 - 1
static/app/utils/metrics/index.tsx

@@ -75,7 +75,7 @@ export function getDdmUrl(
     project,
     ...otherParams
   }: Omit<DdmQueryParams, 'project' | 'widgets'> & {
-    widgets: MetricWidgetQueryParams[];
+    widgets: Partial<MetricWidgetQueryParams>[];
     project?: (string | number)[];
   }
 ) {

+ 1 - 0
static/app/utils/metrics/types.tsx

@@ -22,6 +22,7 @@ export interface FocusedMetricsSeries {
 
 export interface MetricWidgetQueryParams extends MetricsQuerySubject {
   displayType: MetricDisplayType;
+  id: number;
   focusedSeries?: FocusedMetricsSeries[];
   highlightedSample?: string | null;
   powerUserMode?: boolean;

+ 1 - 1
static/app/views/dashboards/utils.tsx

@@ -426,7 +426,7 @@ export function getWidgetDDMUrl(
         groupBy: query.columns,
         query: query.conditions ?? '',
         displayType: getMetricDisplayType(_widget.displayType),
-      } satisfies MetricWidgetQueryParams;
+      } satisfies Partial<MetricWidgetQueryParams>;
     }),
   });
 

+ 6 - 2
static/app/views/ddm/context.tsx

@@ -10,7 +10,7 @@ import * as Sentry from '@sentry/react';
 import isEqual from 'lodash/isEqual';
 
 import {useInstantRef, useUpdateQuery} from 'sentry/utils/metrics';
-import {emptyWidget} from 'sentry/utils/metrics/constants';
+import {emptyWidget, NO_QUERY_ID} from 'sentry/utils/metrics/constants';
 import type {MetricWidgetQueryParams} from 'sentry/utils/metrics/types';
 import {decodeInteger, decodeScalar} from 'sentry/utils/queryString';
 import useLocationQuery from 'sentry/utils/url/useLocationQuery';
@@ -122,6 +122,8 @@ export function useMetricWidgets() {
         ...lastWidget,
       };
 
+      newWidget.id = NO_QUERY_ID;
+
       return [...currentWidgets, newWidget];
     });
   }, [setWidgets]);
@@ -141,7 +143,9 @@ export function useMetricWidgets() {
     (index: number) => {
       setWidgets(currentWidgets => {
         const newWidgets = [...currentWidgets];
-        newWidgets.splice(index, 0, currentWidgets[index]);
+        const newWidget = {...currentWidgets[index]};
+        newWidget.id = NO_QUERY_ID;
+        newWidgets.splice(index + 1, 0, newWidget);
         return newWidgets;
       });
     },

+ 1 - 1
static/app/views/ddm/pageHeaderActions.tsx

@@ -133,7 +133,7 @@ export function PageHeaderActions({showCustomMetricButton, addCustomMetric}: Pro
             ? [
                 <QuerySymbol
                   key="icon"
-                  index={index}
+                  queryId={widget.id}
                   isSelected={index === selectedWidgetIndex}
                 />,
               ]

+ 2 - 3
static/app/views/ddm/queries.tsx

@@ -53,7 +53,7 @@ export function Queries() {
               symbol={
                 showQuerySymbols && (
                   <StyledQuerySymbol
-                    index={index}
+                    queryId={widget.id}
                     isClickable={isMultiChartMode}
                     isSelected={index === selectedWidgetIndex}
                     onClick={() => setSelectedWidgetIndex(index)}
@@ -139,8 +139,7 @@ const StyledQuerySymbol = styled(QuerySymbol)<{isClickable: boolean}>`
   ${p => p.isClickable && `cursor: pointer;`}
 `;
 
-const Wrapper = styled('div')<{showQuerySymbols: boolean}>`
-`;
+const Wrapper = styled('div')<{showQuerySymbols: boolean}>``;
 
 const Row = styled('div')`
   display: contents;

+ 4 - 4
static/app/views/ddm/querySymbol.tsx

@@ -39,17 +39,17 @@ const Symbol = styled('div')<{isSelected: boolean}>`
 `;
 
 export function QuerySymbol({
-  index,
+  queryId,
   isSelected,
   ...props
-}: React.ComponentProps<typeof Symbol> & {index: number; isSelected: boolean}) {
+}: React.ComponentProps<typeof Symbol> & {isSelected: boolean; queryId: number}) {
   const {showQuerySymbols, isMultiChartMode} = useDDMContext();
-  if (!showQuerySymbols) {
+  if (!showQuerySymbols || queryId < 0) {
     return null;
   }
   return (
     <Symbol isSelected={isMultiChartMode && isSelected} {...props}>
-      <span>{getQuerySymbol(index)}</span>
+      <span>{getQuerySymbol(queryId)}</span>
     </Symbol>
   );
 }

+ 81 - 0
static/app/views/ddm/utils/parseMetricWidgetsQueryParam.spec.tsx

@@ -18,6 +18,7 @@ describe('parseMetricWidgetQueryParam', () => {
       parseMetricWidgetsQueryParam(
         JSON.stringify([
           {
+            id: 0,
             mri: 'd:transactions/duration@millisecond',
             op: 'sum',
             query: 'test:query',
@@ -31,6 +32,7 @@ describe('parseMetricWidgetQueryParam', () => {
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'sum',
         query: 'test:query',
@@ -48,6 +50,7 @@ describe('parseMetricWidgetQueryParam', () => {
       parseMetricWidgetsQueryParam(
         JSON.stringify([
           {
+            id: 0,
             mri: 'd:transactions/duration@millisecond',
             op: 'sum',
             query: 'test:query',
@@ -58,6 +61,7 @@ describe('parseMetricWidgetQueryParam', () => {
             sort: {name: 'avg', order: 'desc'},
           },
           {
+            id: 1,
             mri: 'd:custom/sentry.event_manager.save@second',
             op: 'avg',
             query: '',
@@ -71,6 +75,7 @@ describe('parseMetricWidgetQueryParam', () => {
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'sum',
         query: 'test:query',
@@ -81,6 +86,7 @@ describe('parseMetricWidgetQueryParam', () => {
         sort: {name: 'avg', order: 'desc'},
       },
       {
+        id: 1,
         mri: 'd:custom/sentry.event_manager.save@second',
         op: 'avg',
         query: '',
@@ -105,6 +111,7 @@ describe('parseMetricWidgetQueryParam', () => {
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'avg',
         query: '',
@@ -121,6 +128,7 @@ describe('parseMetricWidgetQueryParam', () => {
       parseMetricWidgetsQueryParam(
         JSON.stringify([
           {
+            id: 'invalid',
             mri: 'd:transactions/duration@millisecond',
             op: 1,
             query: 12,
@@ -134,6 +142,7 @@ describe('parseMetricWidgetQueryParam', () => {
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'avg',
         query: '',
@@ -151,6 +160,7 @@ describe('parseMetricWidgetQueryParam', () => {
       parseMetricWidgetsQueryParam(
         JSON.stringify([
           {
+            id: 0,
             mri: 'd:transactions/duration@millisecond',
           },
           {
@@ -160,10 +170,16 @@ describe('parseMetricWidgetQueryParam', () => {
             // Mallformed MRI
             mri: 'transactions/duration@millisecond',
           },
+          {
+            // Duplicate id
+            id: 0,
+            mri: 'd:transactions/duration@second',
+          },
         ])
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'avg',
         query: '',
@@ -193,6 +209,7 @@ describe('parseMetricWidgetQueryParam', () => {
       parseMetricWidgetsQueryParam(
         JSON.stringify([
           {
+            id: 0,
             mri: 'd:transactions/duration@millisecond',
             op: 'sum',
             query: 'test:query',
@@ -206,6 +223,7 @@ describe('parseMetricWidgetQueryParam', () => {
       )
     ).toEqual([
       {
+        id: 0,
         mri: 'd:transactions/duration@millisecond',
         op: 'sum',
         query: 'test:query',
@@ -217,4 +235,67 @@ describe('parseMetricWidgetQueryParam', () => {
       },
     ]);
   });
+
+  it('adds missing ids', () => {
+    const widgetWithId = (id: number | undefined) => ({
+      id,
+      mri: 'd:transactions/duration@millisecond',
+      op: 'sum',
+      query: 'test:query',
+      groupBy: ['dist'],
+      displayType: 'line',
+      focusedSeries: [{seriesName: 'default', groupBy: {dist: 'default'}}],
+      powerUserMode: true,
+      sort: {name: 'avg', order: 'desc'},
+    });
+    expect(
+      parseMetricWidgetsQueryParam(
+        JSON.stringify([
+          widgetWithId(0),
+          widgetWithId(undefined),
+          widgetWithId(2),
+          widgetWithId(undefined),
+          widgetWithId(3),
+        ])
+      )
+    ).toEqual([
+      widgetWithId(0),
+      widgetWithId(1),
+      widgetWithId(2),
+      widgetWithId(4),
+      widgetWithId(3),
+    ]);
+  });
+
+  it('resets the id of a single widget to 0', () => {
+    expect(
+      parseMetricWidgetsQueryParam(
+        JSON.stringify([
+          {
+            id: 5,
+            mri: 'd:transactions/duration@millisecond',
+            op: 'sum',
+            query: 'test:query',
+            groupBy: ['dist'],
+            displayType: 'line',
+            focusedSeries: [{seriesName: 'default', groupBy: {dist: 'default'}}],
+            powerUserMode: true,
+            sort: {name: 'avg', order: 'desc'},
+          },
+        ])
+      )
+    ).toEqual([
+      {
+        id: 0,
+        mri: 'd:transactions/duration@millisecond',
+        op: 'sum',
+        query: 'test:query',
+        groupBy: ['dist'],
+        displayType: 'line',
+        focusedSeries: [{seriesName: 'default', groupBy: {dist: 'default'}}],
+        powerUserMode: true,
+        sort: {name: 'avg', order: 'desc'},
+      },
+    ]);
+  });
 });

Some files were not shown because too many files changed in this diff