Browse Source

chore(ts): Improve Starfish span metrics typing (#53453)

Still a lot of cleanup to do, but this takes a few initial steps.

1. Remove unused `Span` type. Literally not used anywhere
2. Remove `IndexedSpan` type. This one was a little silly. It was used
in a few places, mostly to pick off the `group` and `op` properties.
Wrong for a bunch of reasons, but mostly wrong because
`SpanIndexedFieldTypes` covers the same space but much better.
3. Improve `SpanIndexedFieldTypes` by adding a few missing fields
4. Add `SpanMetricsFieldTypes` to match the indexed version
5. Remove newly unused `useSpanMeta` hook

Also, a bunch of places were accepting a `span`, but it's just an object
with a group. I replaced it with a plain string parameter. In the
future, I'll probably just merge it into `queryFilters`, since `group`
is a filter like any other.

Next up I'll probably consolidate some of these field names, and start
looking at adding more descriptive, strict typing.
George Gritsouk 1 year ago
parent
commit
ccb55e8481

+ 0 - 22
static/app/views/starfish/queries/types.tsx

@@ -1,22 +0,0 @@
-export type Span = {
-  group_id: string;
-  span_operation: string;
-  action?: string;
-  description?: string;
-  domain?: string;
-  formatted_desc?: string;
-  span_id?: string;
-};
-
-export type IndexedSpan = {
-  action: string;
-  description: string;
-  domain: string;
-  group: string;
-  module: string;
-  'span.op': string;
-  'span.self_time': number;
-  span_id: string;
-  timestamp: string;
-  transaction_id: string;
-};

+ 0 - 57
static/app/views/starfish/queries/useSpanMeta.tsx

@@ -1,57 +0,0 @@
-import {Location} from 'history';
-
-import EventView from 'sentry/utils/discover/eventView';
-import {DiscoverDatasets} from 'sentry/utils/discover/types';
-import {useLocation} from 'sentry/utils/useLocation';
-import {SpanSummaryQueryFilters} from 'sentry/views/starfish/queries/useSpanMetrics';
-import {SpanMetricsFields} from 'sentry/views/starfish/types';
-import {useSpansQuery} from 'sentry/views/starfish/utils/useSpansQuery';
-
-const {SPAN_OP, SPAN_ACTION, SPAN_DESCRIPTION, SPAN_DOMAIN, SPAN_GROUP} =
-  SpanMetricsFields;
-
-export type SpanMeta = {
-  'span.action': string;
-  'span.description': string;
-  'span.domain': string;
-  'span.op': string;
-};
-
-export const useSpanMeta = (
-  group: string,
-  queryFilters: SpanSummaryQueryFilters,
-  referrer: string = 'span-metrics'
-) => {
-  const location = useLocation();
-
-  return useSpansQuery<SpanMeta[]>({
-    eventView: getEventView(group, location, queryFilters),
-    initialData: [],
-    referrer,
-  });
-};
-
-function getEventView(
-  groupId,
-  location: Location,
-  queryFilters: SpanSummaryQueryFilters
-) {
-  return EventView.fromNewQueryWithLocation(
-    {
-      name: '',
-      query: `${SPAN_GROUP}:${groupId}${
-        queryFilters?.transactionName
-          ? ` transaction:${queryFilters?.transactionName}`
-          : ''
-      }${
-        queryFilters?.['transaction.method']
-          ? ` transaction.method:${queryFilters?.['transaction.method']}`
-          : ''
-      }`,
-      fields: [SPAN_OP, SPAN_DESCRIPTION, SPAN_ACTION, SPAN_DOMAIN, 'count()'], // TODO: Failing to pass a field like `count()` causes an error
-      dataset: DiscoverDatasets.SPANS_METRICS,
-      version: 2,
-    },
-    location
-  );
-}

+ 7 - 8
static/app/views/starfish/queries/useSpanMetrics.tsx

@@ -3,7 +3,6 @@ import {Location} from 'history';
 import EventView from 'sentry/utils/discover/eventView';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
 import {useLocation} from 'sentry/utils/useLocation';
-import type {IndexedSpan} from 'sentry/views/starfish/queries/types';
 import {SpanMetricsFields} from 'sentry/views/starfish/types';
 import {useSpansQuery} from 'sentry/views/starfish/utils/useSpansQuery';
 
@@ -24,16 +23,18 @@ export type SpanSummaryQueryFilters = {
 };
 
 export const useSpanMetrics = (
-  span: Pick<IndexedSpan, 'group'>,
+  group: string,
   queryFilters: SpanSummaryQueryFilters,
   fields: string[] = [],
   referrer: string = 'span-metrics'
 ) => {
   const location = useLocation();
-  const eventView = span ? getEventView(span, location, queryFilters, fields) : undefined;
+  const eventView = group
+    ? getEventView(group, location, queryFilters, fields)
+    : undefined;
 
   const enabled =
-    Boolean(span?.group) && Object.values(queryFilters).every(value => Boolean(value));
+    Boolean(group) && Object.values(queryFilters).every(value => Boolean(value));
 
   // TODO: Add referrer
   const result = useSpansQuery<SpanMetrics[]>({
@@ -47,17 +48,15 @@ export const useSpanMetrics = (
 };
 
 function getEventView(
-  span: {group: string},
+  group: string,
   location: Location,
   queryFilters?: SpanSummaryQueryFilters,
   fields: string[] = []
 ) {
-  const cleanGroupId = span.group.replaceAll('-', '').slice(-16);
-
   return EventView.fromNewQueryWithLocation(
     {
       name: '',
-      query: `${SPAN_GROUP}:${cleanGroupId}${
+      query: `${SPAN_GROUP}:${group}${
         queryFilters?.transactionName
           ? ` transaction:${queryFilters?.transactionName}`
           : ''

+ 6 - 9
static/app/views/starfish/queries/useSpanMetricsSeries.tsx

@@ -6,7 +6,6 @@ import {Series} from 'sentry/types/echarts';
 import EventView from 'sentry/utils/discover/eventView';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
 import usePageFilters from 'sentry/utils/usePageFilters';
-import type {IndexedSpan} from 'sentry/views/starfish/queries/types';
 import {SpanSummaryQueryFilters} from 'sentry/views/starfish/queries/useSpanMetrics';
 import {SpanMetricsFields} from 'sentry/views/starfish/types';
 import {STARFISH_CHART_INTERVAL_FIDELITY} from 'sentry/views/starfish/utils/constants';
@@ -23,19 +22,19 @@ export type SpanMetrics = {
 };
 
 export const useSpanMetricsSeries = (
-  span: Pick<IndexedSpan, 'group'>,
+  group: string,
   queryFilters: SpanSummaryQueryFilters,
   yAxis: string[] = [],
   referrer = 'span-metrics-series'
 ) => {
   const pageFilters = usePageFilters();
 
-  const eventView = span
-    ? getEventView(span, pageFilters.selection, yAxis, queryFilters)
+  const eventView = group
+    ? getEventView(group, pageFilters.selection, yAxis, queryFilters)
     : undefined;
 
   const enabled =
-    Boolean(span?.group) && Object.values(queryFilters).every(value => Boolean(value));
+    Boolean(group) && Object.values(queryFilters).every(value => Boolean(value));
 
   // TODO: Add referrer
   const result = useSpansQuery<SpanMetrics[]>({
@@ -64,17 +63,15 @@ export const useSpanMetricsSeries = (
 };
 
 function getEventView(
-  span: {group: string},
+  group: string,
   pageFilters: PageFilters,
   yAxis: string[],
   queryFilters: SpanSummaryQueryFilters
 ) {
-  const cleanGroupId = span.group.replaceAll('-', '').slice(-16);
-
   return EventView.fromNewQueryWithPageFilters(
     {
       name: '',
-      query: `${SPAN_GROUP}:${cleanGroupId}${
+      query: `${SPAN_GROUP}:${group}${
         queryFilters?.transactionName
           ? ` transaction:${queryFilters?.transactionName}`
           : ''

+ 1 - 1
static/app/views/starfish/queries/useSpanSamples.tsx

@@ -47,7 +47,7 @@ export const useSpanSamples = (options: Options) => {
   const dateCondtions = getDateConditions(pageFilter.selection);
 
   const {isLoading: isLoadingSeries, data: spanMetricsSeriesData} = useSpanMetricsSeries(
-    {group: groupId},
+    groupId,
     {transactionName, 'transaction.method': transactionMethod},
     [`p95(${SPAN_SELF_TIME})`],
     'api.starfish.sidebar-span-metrics'

+ 5 - 8
static/app/views/starfish/queries/useSpanTransactionMetrics.tsx

@@ -5,7 +5,6 @@ import {Sort} from 'sentry/utils/discover/fields';
 import {DiscoverDatasets} from 'sentry/utils/discover/types';
 import {MutableSearch} from 'sentry/utils/tokenizeSearch';
 import {useLocation} from 'sentry/utils/useLocation';
-import type {IndexedSpan} from 'sentry/views/starfish/queries/types';
 import {SpanMetricsFields} from 'sentry/views/starfish/types';
 import {useWrappedDiscoverQuery} from 'sentry/views/starfish/utils/useSpansQuery';
 
@@ -26,7 +25,7 @@ export type SpanTransactionMetrics = {
 };
 
 export const useSpanTransactionMetrics = (
-  span: Pick<IndexedSpan, 'group'>,
+  group: string,
   options: {sorts?: Sort[]; transactions?: string[]},
   _referrer = 'api.starfish.span-transaction-metrics'
 ) => {
@@ -34,27 +33,25 @@ export const useSpanTransactionMetrics = (
 
   const {transactions, sorts} = options;
 
-  const eventView = getEventView(span, location, transactions ?? [], sorts);
+  const eventView = getEventView(group, location, transactions ?? [], sorts);
 
   return useWrappedDiscoverQuery<SpanTransactionMetrics[]>({
     eventView,
     initialData: [],
-    enabled: Boolean(span),
+    enabled: Boolean(group),
     limit: 25,
     referrer: _referrer,
   });
 };
 
 function getEventView(
-  span: {group: string},
+  group: string,
   location: Location,
   transactions: string[],
   sorts?: Sort[]
 ) {
-  const cleanGroupId = span.group.replaceAll('-', '').slice(-16);
-
   const search = new MutableSearch('');
-  search.addFilterValues(SPAN_GROUP, [cleanGroupId]);
+  search.addFilterValues(SPAN_GROUP, [group]);
   search.addFilterValues('transaction.op', ['http.server']);
 
   if (transactions.length > 0) {

+ 30 - 10
static/app/views/starfish/types.tsx

@@ -26,6 +26,17 @@ export enum SpanMetricsFields {
   SPAN_SELF_TIME = 'span.self_time',
 }
 
+export type SpanMetricsFieldTypes = {
+  [SpanMetricsFields.SPAN_OP]: string;
+  [SpanMetricsFields.SPAN_DESCRIPTION]: string;
+  [SpanMetricsFields.SPAN_MODULE]: string;
+  [SpanMetricsFields.SPAN_ACTION]: string;
+  [SpanMetricsFields.SPAN_DOMAIN]: string;
+  [SpanMetricsFields.SPAN_GROUP]: string;
+  [SpanMetricsFields.SPAN_SELF_TIME]: number;
+  [SpanMetricsFields.SPAN_DURATION]: number;
+};
+
 export enum SpanIndexedFields {
   SPAN_SELF_TIME = 'span.self_time',
   SPAN_GROUP = 'span.group',
@@ -35,29 +46,38 @@ export enum SpanIndexedFields {
   ID = 'span_id',
   SPAN_ACTION = 'span.action',
   TRANSACTION_ID = 'transaction.id',
+  TRANSACTION_METHOD = 'transaction.method',
+  TRANSACTION_OP = 'transaction.op',
   SPAN_DOMAIN = 'span.domain',
   TIMESTAMP = 'timestamp',
-  GROUP = 'span.group',
   PROJECT = 'project',
 }
 
-export enum StarfishFunctions {
-  SPS = 'sps',
-  SPS_PERCENENT_CHANGE = 'sps_percent_change',
-  TIME_SPENT_PERCENTAGE = 'time_spent_percentage',
-  HTTP_ERROR_COUNT = 'http_error_count',
-}
-
 export type SpanIndexedFieldTypes = {
   [SpanIndexedFields.SPAN_SELF_TIME]: number;
-  [SpanIndexedFields.TIMESTAMP]: string;
+  [SpanIndexedFields.SPAN_GROUP]: string;
+  [SpanIndexedFields.SPAN_MODULE]: string;
+  [SpanIndexedFields.SPAN_DESCRIPTION]: string;
+  [SpanIndexedFields.SPAN_OP]: string;
+  [SpanIndexedFields.ID]: string;
   [SpanIndexedFields.SPAN_ACTION]: string;
   [SpanIndexedFields.TRANSACTION_ID]: string;
+  [SpanIndexedFields.TRANSACTION_METHOD]: string;
+  [SpanIndexedFields.TRANSACTION_OP]: string;
   [SpanIndexedFields.SPAN_DOMAIN]: string;
+  [SpanIndexedFields.TIMESTAMP]: string;
   [SpanIndexedFields.PROJECT]: string;
-  [SpanIndexedFields.ID]: string;
 };
 
+export type Op = SpanIndexedFieldTypes[SpanIndexedFields.SPAN_OP];
+
+export enum StarfishFunctions {
+  SPS = 'sps',
+  SPS_PERCENENT_CHANGE = 'sps_percent_change',
+  TIME_SPENT_PERCENTAGE = 'time_spent_percentage',
+  HTTP_ERROR_COUNT = 'http_error_count',
+}
+
 export const StarfishDatasetFields = {
   [DiscoverDatasets.SPANS_METRICS]: SpanIndexedFields,
   [DiscoverDatasets.SPANS_INDEXED]: SpanIndexedFields,

+ 49 - 32
static/app/views/starfish/views/spanSummaryPage/index.tsx

@@ -29,9 +29,7 @@ import {CountCell} from 'sentry/views/starfish/components/tableCells/countCell';
 import DurationCell from 'sentry/views/starfish/components/tableCells/durationCell';
 import ThroughputCell from 'sentry/views/starfish/components/tableCells/throughputCell';
 import {TimeSpentCell} from 'sentry/views/starfish/components/tableCells/timeSpentCell';
-import {SpanMeta} from 'sentry/views/starfish/queries/useSpanMeta';
 import {
-  SpanMetrics,
   SpanSummaryQueryFilters,
   useSpanMetrics,
 } from 'sentry/views/starfish/queries/useSpanMetrics';
@@ -52,9 +50,6 @@ import {
   SpanTransactionsTable,
 } from 'sentry/views/starfish/views/spanSummaryPage/spanTransactionsTable';
 
-const {SPAN_SELF_TIME, SPAN_OP, SPAN_DESCRIPTION, SPAN_ACTION, SPAN_DOMAIN} =
-  SpanMetricsFields;
-
 const DEFAULT_SORT: Sort = {
   kind: 'desc',
   field: 'time_spent_percentage(local)',
@@ -81,42 +76,53 @@ function SpanSummaryPage({params, location}: Props) {
   }
 
   const {data: spanMetrics, isLoading: isSpanMetricsLoading} = useSpanMetrics(
-    {group: groupId},
+    groupId,
     queryFilter,
     [
-      SPAN_OP,
-      SPAN_DESCRIPTION,
-      SPAN_ACTION,
-      SPAN_DOMAIN,
+      SpanMetricsFields.SPAN_OP,
+      SpanMetricsFields.SPAN_DESCRIPTION,
+      SpanMetricsFields.SPAN_ACTION,
+      SpanMetricsFields.SPAN_DOMAIN,
       'count()',
       'sps()',
-      `sum(${SPAN_SELF_TIME})`,
-      `p95(${SPAN_SELF_TIME})`,
+      `sum(${SpanMetricsFields.SPAN_SELF_TIME})`,
+      `p95(${SpanMetricsFields.SPAN_SELF_TIME})`,
       'time_spent_percentage()',
       'http_error_count()',
     ],
     'api.starfish.span-summary-page-metrics'
   );
 
-  const span = Object.assign({group: groupId}, spanMetrics as SpanMetrics & SpanMeta);
+  const span = {
+    ...spanMetrics,
+    [SpanMetricsFields.SPAN_GROUP]: groupId,
+  } as {
+    [SpanMetricsFields.SPAN_OP]: string;
+    [SpanMetricsFields.SPAN_DESCRIPTION]: string;
+    [SpanMetricsFields.SPAN_ACTION]: string;
+    [SpanMetricsFields.SPAN_DOMAIN]: string;
+    [SpanMetricsFields.SPAN_GROUP]: string;
+  };
 
   const {isLoading: areSpanMetricsSeriesLoading, data: spanMetricsSeriesData} =
     useSpanMetricsSeries(
-      {group: groupId},
+      groupId,
       queryFilter,
-      [`p95(${SPAN_SELF_TIME})`, 'sps()', 'http_error_count()'],
+      [`p95(${SpanMetricsFields.SPAN_SELF_TIME})`, 'sps()', 'http_error_count()'],
       'api.starfish.span-summary-page-metrics-chart'
     );
 
   useSynchronizeCharts([!areSpanMetricsSeriesLoading]);
 
   const spanMetricsThroughputSeries = {
-    seriesName: span?.[SPAN_OP]?.startsWith('db') ? 'Queries' : 'Requests',
+    seriesName: span?.[SpanMetricsFields.SPAN_OP]?.startsWith('db')
+      ? 'Queries'
+      : 'Requests',
     data: spanMetricsSeriesData?.['sps()'].data,
   };
 
-  const title = getDescriptionLabel(span, true);
-  const spanDescriptionCardTitle = getDescriptionLabel(span);
+  const title = getDescriptionLabel(span[SpanMetricsFields.SPAN_OP], true);
+  const spanDescriptionCardTitle = getDescriptionLabel(span[SpanMetricsFields.SPAN_OP]);
 
   const crumbs: Crumb[] = [];
   crumbs.push({
@@ -164,12 +170,14 @@ function SpanSummaryPage({params, location}: Props) {
                     <StarfishDatePicker />
                   </FilterOptionsContainer>
                   <BlockContainer>
-                    {span?.[SPAN_OP]?.startsWith('db') &&
-                      span?.[SPAN_OP] !== 'db.redis' && (
-                        <Block title={t('Table')}>{span?.[SPAN_DOMAIN]}</Block>
+                    {span?.[SpanMetricsFields.SPAN_OP]?.startsWith('db') &&
+                      span?.[SpanMetricsFields.SPAN_OP] !== 'db.redis' && (
+                        <Block title={t('Table')}>
+                          {span?.[SpanMetricsFields.SPAN_DOMAIN]}
+                        </Block>
                       )}
                     <Block
-                      title={getThroughputTitle(span?.[SPAN_OP])}
+                      title={getThroughputTitle(span?.[SpanMetricsFields.SPAN_OP])}
                       description={tct('Throughput of this [spanType] per second', {
                         spanType: spanDescriptionCardTitle,
                       })}
@@ -188,10 +196,12 @@ function SpanSummaryPage({params, location}: Props) {
                       )}
                     >
                       <DurationCell
-                        milliseconds={spanMetrics?.[`p95(${SPAN_SELF_TIME})`]}
+                        milliseconds={
+                          spanMetrics?.[`p95(${SpanMetricsFields.SPAN_SELF_TIME})`]
+                        }
                       />
                     </Block>
-                    {span?.[SPAN_OP]?.startsWith('http') && (
+                    {span?.[SpanMetricsFields.SPAN_OP]?.startsWith('http') && (
                       <Block
                         title={t('5XX Responses')}
                         description={t('5XX responses in this span')}
@@ -207,13 +217,15 @@ function SpanSummaryPage({params, location}: Props) {
                     >
                       <TimeSpentCell
                         timeSpentPercentage={spanMetrics?.['time_spent_percentage()']}
-                        totalSpanTime={spanMetrics?.[`p95(${SPAN_SELF_TIME})`]}
+                        totalSpanTime={
+                          spanMetrics?.[`p95(${SpanMetricsFields.SPAN_SELF_TIME})`]
+                        }
                       />
                     </Block>
                   </BlockContainer>
                 </BlockContainer>
 
-                {span?.[SPAN_DESCRIPTION] && (
+                {span?.[SpanMetricsFields.SPAN_DESCRIPTION] && (
                   <BlockContainer>
                     <Block>
                       <Panel>
@@ -229,7 +241,9 @@ function SpanSummaryPage({params, location}: Props) {
                     </Block>
 
                     <Block>
-                      <ChartPanel title={getThroughputChartTitle(span?.[SPAN_OP])}>
+                      <ChartPanel
+                        title={getThroughputChartTitle(span?.[SpanMetricsFields.SPAN_OP])}
+                      >
                         <Chart
                           height={140}
                           data={[spanMetricsThroughputSeries]}
@@ -250,7 +264,11 @@ function SpanSummaryPage({params, location}: Props) {
                       <ChartPanel title={DataTitles.p95}>
                         <Chart
                           height={140}
-                          data={[spanMetricsSeriesData?.[`p95(${SPAN_SELF_TIME})`]]}
+                          data={[
+                            spanMetricsSeriesData?.[
+                              `p95(${SpanMetricsFields.SPAN_SELF_TIME})`
+                            ],
+                          ]}
                           loading={areSpanMetricsSeriesLoading}
                           utc={false}
                           chartColors={[P95_COLOR]}
@@ -260,7 +278,7 @@ function SpanSummaryPage({params, location}: Props) {
                       </ChartPanel>
                     </Block>
 
-                    {span?.[SPAN_OP]?.startsWith('http') && (
+                    {span?.[SpanMetricsFields.SPAN_OP]?.startsWith('http') && (
                       <Block>
                         <ChartPanel title={DataTitles.errorCount}>
                           <Chart
@@ -288,7 +306,7 @@ function SpanSummaryPage({params, location}: Props) {
                 )}
 
                 <SampleList
-                  groupId={span.group}
+                  groupId={span[SpanMetricsFields.SPAN_GROUP]}
                   transactionName={transaction}
                   transactionMethod={transactionMethod}
                 />
@@ -383,8 +401,7 @@ const DescriptionTitle = styled('h4')`
 
 export default SpanSummaryPage;
 
-const getDescriptionLabel = (spanMeta: SpanMeta, title?: boolean) => {
-  const spanOp = spanMeta[SPAN_OP];
+const getDescriptionLabel = (spanOp: string, title?: boolean) => {
   if (spanOp?.startsWith('http')) {
     return title ? t('URL Request Summary') : t('URL Request');
   }

+ 2 - 2
static/app/views/starfish/views/spanSummaryPage/sampleList/durationChart/index.tsx

@@ -64,14 +64,14 @@ function DurationChart({
     data: spanMetricsSeriesData,
     error: spanMetricsSeriesError,
   } = useSpanMetricsSeries(
-    {group: groupId},
+    groupId,
     {transactionName, 'transaction.method': transactionMethod},
     [`p95(${SPAN_SELF_TIME})`],
     'api.starfish.sidebar-span-metrics-chart'
   );
 
   const {data: spanMetrics, error: spanMetricsError} = useSpanMetrics(
-    {group: groupId},
+    groupId,
     {transactionName, 'transaction.method': transactionMethod},
     [`p95(${SPAN_SELF_TIME})`, SPAN_OP],
     'api.starfish.span-summary-panel-samples-table-p95'

+ 1 - 1
static/app/views/starfish/views/spanSummaryPage/sampleList/sampleInfo/index.tsx

@@ -21,7 +21,7 @@ function SampleInfo(props: Props) {
   const {setPageError} = usePageError();
 
   const {data: spanMetrics, error} = useSpanMetrics(
-    {group: groupId},
+    groupId,
     {transactionName, 'transaction.method': transactionMethod},
     [
       SPAN_OP,

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