ref(database): Improve useSpanMetricsSeries (#59932)

gggritso · antonpirker · commit d09bb99aa747 · 2023-11-20T09:57:52.000+01:00
- Add spec for `useSpanMetricsSeries`
- Improve `useSpanMetricsSeries` API
    - allow passing _any_ string filter
    - remove `groupId` argument, pass it as a filter instead
    - pass standard filter names

This is simpler and more consistent. Removing the `groupId` argument,
and allowing _all_ string filters will make it easier to re-use this in
other contexts. Adds a sorely needed spec.
diff --git a/static/app/views/performance/browser/resources/resourceSummaryPage/resourceSummaryCharts.tsx b/static/app/views/performance/browser/resources/resourceSummaryPage/resourceSummaryCharts.tsx
@@ -35,8 +35,8 @@ function ResourceSummaryCharts(props: {groupId: string}) {
 
   const {data: spanMetricsSeriesData, isLoading: areSpanMetricsSeriesLoading} =
     useSpanMetricsSeries(
-      props.groupId,
       {
+        'span.group': props.groupId,
         ...(filters[RESOURCE_RENDER_BLOCKING_STATUS]
           ? {[RESOURCE_RENDER_BLOCKING_STATUS]: filters[RESOURCE_RENDER_BLOCKING_STATUS]}
           : {}),
diff --git a/static/app/views/performance/database/databaseSpanSummaryPage.tsx b/static/app/views/performance/database/databaseSpanSummaryPage.tsx
@@ -94,15 +94,13 @@ function SpanSummaryPage({params}: Props) {
   };
 
   const {isLoading: isThroughputDataLoading, data: throughputData} = useSpanMetricsSeries(
-    groupId,
-    queryFilter,
+    {...queryFilter, 'span.group': groupId},
     ['spm()'],
     'api.starfish.span-summary-page-metrics-chart'
   );
 
   const {isLoading: isDurationDataLoading, data: durationData} = useSpanMetricsSeries(
-    groupId,
-    queryFilter,
+    {...queryFilter, 'span.group': groupId},
     [`${selectedAggregate}(${SpanMetricsField.SPAN_SELF_TIME})`],
     'api.starfish.span-summary-page-metrics-chart'
   );
diff --git a/static/app/views/starfish/queries/useSpanMetricsSeries.spec.tsx b/static/app/views/starfish/queries/useSpanMetricsSeries.spec.tsx
@@ -0,0 +1,115 @@
+import {ReactNode} from 'react';
+import {Organization} from 'sentry-fixture/organization';
+
+import {makeTestQueryClient} from 'sentry-test/queryClient';
+import {reactHooks} from 'sentry-test/reactTestingLibrary';
+
+import {QueryClientProvider} from 'sentry/utils/queryClient';
+import {useLocation} from 'sentry/utils/useLocation';
+import useOrganization from 'sentry/utils/useOrganization';
+import usePageFilters from 'sentry/utils/usePageFilters';
+import {useSpanMetricsSeries} from 'sentry/views/starfish/queries/useSpanMetricsSeries';
+
+jest.mock('sentry/utils/useLocation');
+jest.mock('sentry/utils/usePageFilters');
+jest.mock('sentry/utils/useOrganization');
+
+function Wrapper({children}: {children?: ReactNode}) {
+  return (
+    <QueryClientProvider client={makeTestQueryClient()}>{children}</QueryClientProvider>
+  );
+}
+
+describe('useSpanMetricsSeries', () => {
+  const organization = Organization();
+
+  jest.mocked(usePageFilters).mockReturnValue({
+    isReady: true,
+    desyncedFilters: new Set(),
+    pinnedFilters: new Set(),
+    shouldPersist: true,
+    selection: {
+      datetime: {
+        period: '10d',
+        start: null,
+        end: null,
+        utc: false,
+      },
+      environments: [],
+      projects: [],
+    },
+  });
+
+  jest.mocked(useLocation).mockReturnValue({
+    pathname: '',
+    search: '',
+    query: {},
+    hash: '',
+    state: undefined,
+    action: 'PUSH',
+    key: '',
+  });
+
+  jest.mocked(useOrganization).mockReturnValue(organization);
+
+  it('queries for current selection', async () => {
+    const eventsRequest = MockApiClient.addMockResponse({
+      url: `/organizations/${organization.slug}/events-stats/`,
+      method: 'GET',
+      body: {
+        'spm()': {
+          data: [
+            [1699907700, [{count: 7810.2}]],
+            [1699908000, [{count: 1216.8}]],
+          ],
+        },
+      },
+    });
+
+    const {result, waitForNextUpdate} = reactHooks.renderHook(
+      ({filters, yAxis}) => useSpanMetricsSeries(filters, yAxis),
+      {
+        wrapper: Wrapper,
+        initialProps: {
+          filters: {
+            'span.group': '221aa7ebd216',
+            transaction: '/api/details',
+            release: '0.0.1',
+            'resource.render_blocking_status': 'blocking' as const,
+          },
+          yAxis: ['spm()'],
+        },
+      }
+    );
+
+    expect(result.current.isLoading).toEqual(true);
+
+    expect(eventsRequest).toHaveBeenCalledWith(
+      '/organizations/org-slug/events-stats/',
+      expect.objectContaining({
+        method: 'GET',
+        query: expect.objectContaining({
+          query: `span.group:221aa7ebd216 transaction:/api/details release:0.0.1 resource.render_blocking_status:blocking`,
+          dataset: 'spansMetrics',
+          statsPeriod: '10d',
+          referrer: 'span-metrics-series',
+          interval: '30m',
+          yAxis: 'spm()',
+        }),
+      })
+    );
+
+    await waitForNextUpdate();
+
+    expect(result.current.isLoading).toEqual(false);
+    expect(result.current.data).toEqual({
+      'spm()': {
+        data: [
+          {name: '2023-11-13T20:35:00+00:00', value: 7810.2},
+          {name: '2023-11-13T20:40:00+00:00', value: 1216.8},
+        ],
+        seriesName: 'spm()',
+      },
+    });
+  });
+});
diff --git a/static/app/views/starfish/queries/useSpanMetricsSeries.tsx b/static/app/views/starfish/queries/useSpanMetricsSeries.tsx
@@ -5,13 +5,12 @@ import {PageFilters} from 'sentry/types';
 import {Series} from 'sentry/types/echarts';
 import EventView from 'sentry/utils/discover/eventView';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
+import {MutableSearch} from 'sentry/utils/tokenizeSearch';
 import usePageFilters from 'sentry/utils/usePageFilters';
-import {SpanSummaryQueryFilters} from 'sentry/views/starfish/queries/useSpanMetrics';
-import {SpanMetricsField} from 'sentry/views/starfish/types';
+import {SpanMetricsQueryFilters} from 'sentry/views/starfish/types';
 import {STARFISH_CHART_INTERVAL_FIDELITY} from 'sentry/views/starfish/utils/constants';
 import {useSpansQuery} from 'sentry/views/starfish/utils/useSpansQuery';
-
-const {SPAN_GROUP, RESOURCE_RENDER_BLOCKING_STATUS} = SpanMetricsField;
+import {EMPTY_OPTION_VALUE} from 'sentry/views/starfish/views/spans/selectors/emptyOption';
 
 export type SpanMetrics = {
   interval: number;
@@ -22,19 +21,15 @@ export type SpanMetrics = {
 };
 
 export const useSpanMetricsSeries = (
-  group: string,
-  queryFilters: SpanSummaryQueryFilters,
+  filters: SpanMetricsQueryFilters,
   yAxis: string[] = [],
   referrer = 'span-metrics-series'
 ) => {
   const pageFilters = usePageFilters();
 
-  const eventView = group
-    ? getEventView(group, pageFilters.selection, yAxis, queryFilters)
-    : undefined;
+  const eventView = getEventView(filters, pageFilters.selection, yAxis);
 
-  const enabled =
-    Boolean(group) && Object.values(queryFilters).every(value => Boolean(value));
+  const enabled = Object.values(filters).every(value => Boolean(value));
 
   const result = useSpansQuery<SpanMetrics[]>({
     eventView,
@@ -62,31 +57,31 @@ export const useSpanMetricsSeries = (
 };
 
 function getEventView(
-  group: string,
+  filters: SpanMetricsQueryFilters,
   pageFilters: PageFilters,
-  yAxis: string[],
-  queryFilters: SpanSummaryQueryFilters
+  yAxis: string[]
 ) {
-  const query = [
-    `${SPAN_GROUP}:${group}`,
-    ...(queryFilters?.transactionName
-      ? [`transaction:"${queryFilters?.transactionName}"`]
-      : []),
-    ...(queryFilters?.['transaction.method']
-      ? [`transaction.method:${queryFilters?.['transaction.method']}`]
-      : []),
-    ...(queryFilters?.release ? [`release:${queryFilters?.release}`] : []),
-    ...(queryFilters?.[RESOURCE_RENDER_BLOCKING_STATUS]
-      ? [
-          `${RESOURCE_RENDER_BLOCKING_STATUS}:${queryFilters[RESOURCE_RENDER_BLOCKING_STATUS]}`,
-        ]
-      : []),
-  ].join(' ');
+  const query = new MutableSearch('');
+
+  Object.entries(filters).forEach(([key, value]) => {
+    if (!value) {
+      return;
+    }
+
+    if (value === EMPTY_OPTION_VALUE) {
+      query.addFilterValue('!has', key);
+    }
+
+    query.addFilterValue(key, value, !ALLOWED_WILDCARD_FIELDS.includes(key));
+  });
+
+  // TODO: This condition should be enforced everywhere
+  // query.addFilterValue('has', 'span.description');
 
   return EventView.fromNewQueryWithPageFilters(
     {
       name: '',
-      query,
+      query: query.formatString(),
       fields: [],
       yAxis,
       dataset: DiscoverDatasets.SPANS_METRICS,
@@ -96,3 +91,5 @@ function getEventView(
     pageFilters
   );
 }
+
+const ALLOWED_WILDCARD_FIELDS = ['span.description'];
diff --git a/static/app/views/starfish/queries/useSpanSamples.tsx b/static/app/views/starfish/queries/useSpanSamples.tsx
@@ -74,8 +74,7 @@ export const useSpanSamples = (options: Options) => {
   const dateCondtions = getDateConditions(pageFilter.selection);
 
   const {isLoading: isLoadingSeries, data: spanMetricsSeriesData} = useSpanMetricsSeries(
-    groupId,
-    filters,
+    {'span.group': groupId, ...filters},
     [`avg(${SPAN_SELF_TIME})`],
     'api.starfish.sidebar-span-metrics'
   );
diff --git a/static/app/views/starfish/types.tsx b/static/app/views/starfish/types.tsx
@@ -49,7 +49,8 @@ export type SpanStringFields =
   | 'span.group'
   | 'project.id'
   | 'transaction'
-  | 'transaction.method';
+  | 'transaction.method'
+  | 'release';
 
 export type SpanMetricsQueryFilters = {
   [Field in SpanStringFields]?: string;
diff --git a/static/app/views/starfish/views/spanSummaryPage/sampleList/durationChart/index.tsx b/static/app/views/starfish/views/spanSummaryPage/sampleList/durationChart/index.tsx
@@ -93,8 +93,7 @@ function DurationChart({
     data: spanMetricsSeriesData,
     error: spanMetricsSeriesError,
   } = useSpanMetricsSeries(
-    groupId,
-    filters,
+    {...filters, 'span.group': groupId},
     [`avg(${SPAN_SELF_TIME})`],
     'api.starfish.sidebar-span-metrics-chart'
   );
diff --git a/static/app/views/starfish/views/spanSummaryPage/spanSummaryView.tsx b/static/app/views/starfish/views/spanSummaryPage/spanSummaryView.tsx
@@ -11,10 +11,7 @@ import ChartPanel from 'sentry/views/starfish/components/chartPanel';
 import StarfishDatePicker from 'sentry/views/starfish/components/datePicker';
 import {SpanDescription} from 'sentry/views/starfish/components/spanDescription';
 import {useFullSpanFromTrace} from 'sentry/views/starfish/queries/useFullSpanFromTrace';
-import {
-  SpanSummaryQueryFilters,
-  useSpanMetrics,
-} from 'sentry/views/starfish/queries/useSpanMetrics';
+import {useSpanMetrics} from 'sentry/views/starfish/queries/useSpanMetrics';
 import {useSpanMetricsSeries} from 'sentry/views/starfish/queries/useSpanMetricsSeries';
 import {SpanFunction, SpanMetricsField} from 'sentry/views/starfish/types';
 import {
@@ -43,8 +40,11 @@ export function SpanSummaryView({groupId}: Props) {
 
   const {data: fullSpan} = useFullSpanFromTrace(groupId);
 
-  const queryFilter: SpanSummaryQueryFilters = endpoint
-    ? {transactionName: endpoint, 'transaction.method': endpointMethod}
+  const queryFilter = endpoint
+    ? {
+        transactionName: endpoint,
+        'transaction.method': endpointMethod,
+      }
     : {};
 
   const {data: spanMetrics} = useSpanMetrics(
@@ -78,8 +78,7 @@ export function SpanSummaryView({groupId}: Props) {
 
   const {isLoading: areSpanMetricsSeriesLoading, data: spanMetricsSeriesData} =
     useSpanMetricsSeries(
-      groupId,
-      queryFilter,
+      {'span.group': groupId, ...queryFilter},
       [`avg(${SpanMetricsField.SPAN_SELF_TIME})`, 'spm()', 'http_error_count()'],
       'api.starfish.span-summary-page-metrics-chart'
     );

Original file line number	Diff line number	Diff line change
`@@ -35,8 +35,8 @@ function ResourceSummaryCharts(props: {groupId: string}) {`
`35`	`35`
`36`	`36`	`const {data: spanMetricsSeriesData, isLoading: areSpanMetricsSeriesLoading} =`
`37`	`37`	`useSpanMetricsSeries(`
`38`		`- props.groupId,`
`39`	`38`	`{`
	`39`	`+ 'span.group': props.groupId,`
`40`	`40`	`...(filters[RESOURCE_RENDER_BLOCKING_STATUS]`
`41`	`41`	`? {[RESOURCE_RENDER_BLOCKING_STATUS]: filters[RESOURCE_RENDER_BLOCKING_STATUS]}`
`42`	`42`	`: {}),`