Browse Source

fix(starfish): Use field renderers in endpoint list (#51538)

More work to improve consistency and alignment. The `tps()` field still
needs a custom renderer for now. Also includes a lot of cleanup of dead
code.

- Simplify `PercentChangeCell` API
- Add tps percent change renderer
- Use built-in renderes for number columns
- Remove unused table cell renderer
- Bring back custom tps rendering
- Remove unused delta props
George Gritsouk 1 year ago
parent
commit
07bb6c73d6

+ 8 - 20
static/app/utils/discover/fieldRenderers.tsx

@@ -316,17 +316,7 @@ export const FIELD_FORMATTERS: FieldFormatters = {
   percent_change: {
     isSortable: true,
     renderFunc: (fieldName, data) => {
-      const deltaValue = data[fieldName];
-
-      const sign = deltaValue >= 0 ? '+' : '-';
-      const delta = formatPercentage(Math.abs(deltaValue), 2);
-      const trendDirection = deltaValue < 0 ? 'good' : deltaValue > 0 ? 'bad' : 'neutral';
-
-      return (
-        <PercentChangeCell
-          trendDirection={trendDirection}
-        >{`${sign}${delta}`}</PercentChangeCell>
-      );
+      return <PercentChangeCell deltaValue={data[fieldName]} />;
     },
   },
 };
@@ -714,6 +704,7 @@ type SpecialFunctionFieldRenderer = (
 type SpecialFunctions = {
   sps_percent_change: SpecialFunctionFieldRenderer;
   time_spent_percentage: SpecialFunctionFieldRenderer;
+  tps_percent_change: SpecialFunctionFieldRenderer;
   user_misery: SpecialFunctionFieldRenderer;
 };
 
@@ -781,16 +772,13 @@ const SPECIAL_FUNCTIONS: SpecialFunctions = {
       </BarContainer>
     );
   },
+  // N.B. Do not colorize any throughput percent change renderers, since a
+  // change in throughput is not inherently good or bad
+  tps_percent_change: fieldName => data => {
+    return <PercentChangeCell deltaValue={data[fieldName]} colorize={false} />;
+  },
   sps_percent_change: fieldName => data => {
-    const deltaValue = data[fieldName];
-
-    const sign = deltaValue >= 0 ? '+' : '-';
-    const delta = formatPercentage(Math.abs(deltaValue), 2);
-
-    return (
-      // N.B. For throughput, the change is neither good nor bad regardless of value! Throughput is just throughput
-      <PercentChangeCell trendDirection="neutral">{`${sign}${delta}`}</PercentChangeCell>
-    );
+    return <PercentChangeCell deltaValue={data[fieldName]} colorize={false} />;
   },
   time_spent_percentage: fieldName => data => {
     return (

+ 0 - 32
static/app/views/starfish/components/tableCells/countCell.tsx

@@ -1,32 +0,0 @@
-import styled from '@emotion/styled';
-
-import {space} from 'sentry/styles/space';
-import {formatPercentage} from 'sentry/utils/formatters';
-import {ComparisonLabel} from 'sentry/views/starfish/components/samplesTable/common';
-
-type Props = {
-  count: number;
-  delta?: number;
-};
-
-export default function CountCell({count, delta}: Props) {
-  return (
-    <Container>
-      {count}
-      {delta ? (
-        <ComparisonLabel value={delta}>
-          {delta > 0 ? '+' : ''}
-          {formatPercentage(delta)}
-        </ComparisonLabel>
-      ) : null}
-    </Container>
-  );
-}
-
-const Container = styled('div')`
-  display: flex;
-  width: 100%;
-  justify-content: flex-end;
-  text-align: right;
-  gap: ${space(1)};
-`;

+ 4 - 23
static/app/views/starfish/components/tableCells/durationCell.tsx

@@ -1,33 +1,14 @@
-import styled from '@emotion/styled';
-
 import Duration from 'sentry/components/duration';
-import {space} from 'sentry/styles/space';
-import {formatPercentage} from 'sentry/utils/formatters';
-import {ComparisonLabel} from 'sentry/views/starfish/components/samplesTable/common';
+import {NumberContainer} from 'sentry/utils/discover/styles';
 
 type Props = {
   milliseconds: number;
-  delta?: number;
 };
 
-export default function DurationCell({milliseconds, delta}: Props) {
+export default function DurationCell({milliseconds}: Props) {
   return (
-    <Container>
+    <NumberContainer>
       <Duration seconds={milliseconds / 1000} fixedDigits={2} abbreviation />
-      {delta ? (
-        <ComparisonLabel value={delta}>
-          {delta > 0 ? '+' : ''}
-          {formatPercentage(delta)}
-        </ComparisonLabel>
-      ) : null}
-    </Container>
+    </NumberContainer>
   );
 }
-
-const Container = styled('div')`
-  display: flex;
-  width: 100%;
-  justify-content: flex-end;
-  margin-left: auto;
-  gap: ${space(1)};
-`;

+ 19 - 7
static/app/views/starfish/components/tableCells/percentChangeCell.tsx

@@ -1,22 +1,34 @@
-import React from 'react';
 import styled from '@emotion/styled';
 
 import {NumberContainer} from 'sentry/utils/discover/styles';
+import {formatPercentage} from 'sentry/utils/formatters';
 
-type Props = {
-  children: React.ReactNode;
-  trendDirection: 'good' | 'bad' | 'neutral';
+type PercentChangeCellProps = {
+  deltaValue: number;
+  colorize?: boolean;
 };
 
-export function PercentChangeCell({trendDirection, children}: Props) {
+export function PercentChangeCell({deltaValue, colorize = true}: PercentChangeCellProps) {
+  const sign = deltaValue >= 0 ? '+' : '-';
+  const delta = formatPercentage(Math.abs(deltaValue), 2);
+  const trendDirection = deltaValue < 0 ? 'good' : deltaValue > 0 ? 'bad' : 'neutral';
+
   return (
     <NumberContainer>
-      <Colorized trendDirection={trendDirection}>{children}</Colorized>
+      <Colorized trendDirection={colorize ? trendDirection : 'neutral'}>
+        {sign}
+        {delta}
+      </Colorized>
     </NumberContainer>
   );
 }
 
-const Colorized = styled('div')<Props>`
+type ColorizedProps = {
+  children: React.ReactNode;
+  trendDirection: 'good' | 'bad' | 'neutral';
+};
+
+const Colorized = styled('div')<ColorizedProps>`
   color: ${p =>
     p.trendDirection === 'good'
       ? p.theme.successText

+ 7 - 25
static/app/views/starfish/components/tableCells/throughputCell.tsx

@@ -1,35 +1,17 @@
-import styled from '@emotion/styled';
-
 import {t} from 'sentry/locale';
-import {space} from 'sentry/styles/space';
-import {formatAbbreviatedNumber, formatPercentage} from 'sentry/utils/formatters';
-import {ComparisonLabel} from 'sentry/views/starfish/components/samplesTable/common';
+import {NumberContainer} from 'sentry/utils/discover/styles';
+import {formatAbbreviatedNumber} from 'sentry/utils/formatters';
 
 type Props = {
-  delta?: number;
   throughputPerSecond?: number;
 };
 
-export default function ThroughputCell({throughputPerSecond, delta}: Props) {
+export default function ThroughputCell({throughputPerSecond}: Props) {
   const throughput = throughputPerSecond ? throughputPerSecond.toFixed(2) : '--';
+
   return (
-    <Container>
-      <span>{`${formatAbbreviatedNumber(throughput)}/${t('s')}`}</span>
-      {delta ? (
-        // Don't highlight throughput red or green, since throughput delta isn't good or bad
-        <ComparisonLabel value={0}>
-          {delta > 0 ? '+' : ''}
-          {formatPercentage(delta)}
-        </ComparisonLabel>
-      ) : null}
-    </Container>
+    <NumberContainer>
+      {formatAbbreviatedNumber(throughput)}/{t('s')}
+    </NumberContainer>
   );
 }
-
-const Container = styled('div')`
-  display: flex;
-  width: 100%;
-  justify-content: flex-end;
-  text-align: right;
-  gap: ${space(1)};
-`;

+ 3 - 42
static/app/views/starfish/views/webServiceView/endpointList.tsx

@@ -27,7 +27,6 @@ import {getAggregateAlias} from 'sentry/utils/discover/fields';
 import {NumberContainer} from 'sentry/utils/discover/styles';
 import {formatPercentage} from 'sentry/utils/formatters';
 import {TableColumn} from 'sentry/views/discover/table/types';
-import {PercentChangeCell} from 'sentry/views/starfish/components/tableCells/percentChangeCell';
 import ThroughputCell from 'sentry/views/starfish/components/tableCells/throughputCell';
 import {TIME_SPENT_IN_SERVICE} from 'sentry/views/starfish/utils/generatePerformanceEventView';
 import {DataTitles} from 'sentry/views/starfish/views/spans/types';
@@ -124,48 +123,10 @@ function EndpointList({eventView, location, organization, setError}: Props) {
       );
     }
 
+    // TODO: This can be removed if/when the backend returns this field's type
+    // as `"rate"` and its unit as `"1/second"
     if (field === 'tps()') {
-      return (
-        <NumberContainer>
-          <ThroughputCell throughputPerSecond={dataRow[field] as number} />
-        </NumberContainer>
-      );
-    }
-
-    if (
-      [
-        'percentile_percent_change(transaction.duration,0.95)',
-        'http_error_count_percent_change()',
-      ].includes(field)
-    ) {
-      const deltaValue = dataRow[field] as number;
-      const trendDirection = deltaValue < 0 ? 'good' : deltaValue > 0 ? 'bad' : 'neutral';
-
-      return (
-        <NumberContainer>
-          <PercentChangeCell trendDirection={trendDirection}>
-            {tct('[sign][delta]', {
-              sign: deltaValue >= 0 ? '+' : '-',
-              delta: formatPercentage(Math.abs(deltaValue), 2),
-            })}
-          </PercentChangeCell>
-        </NumberContainer>
-      );
-    }
-
-    if (field === 'tps_percent_change()') {
-      const deltaValue = dataRow[field] as number;
-
-      return (
-        <NumberContainer>
-          <PercentChangeCell trendDirection="neutral">
-            {tct('[sign][delta]', {
-              sign: deltaValue >= 0 ? '+' : '-',
-              delta: formatPercentage(Math.abs(deltaValue), 2),
-            })}
-          </PercentChangeCell>
-        </NumberContainer>
-      );
+      return <ThroughputCell throughputPerSecond={dataRow[field] as number} />;
     }
 
     if (field === 'project') {

+ 2 - 16
static/app/views/starfish/views/webServiceView/failureDetailPanel/failureDetailTable.tsx

@@ -7,13 +7,11 @@ import GridEditable, {GridColumnHeader} from 'sentry/components/gridEditable';
 import {Alignments} from 'sentry/components/gridEditable/sortLink';
 import Link from 'sentry/components/links/link';
 import Pagination from 'sentry/components/pagination';
-import {t, tct} from 'sentry/locale';
+import {t} from 'sentry/locale';
 import {Organization} from 'sentry/types';
 import {TableData, TableDataRow} from 'sentry/utils/discover/discoverQuery';
 import EventView from 'sentry/utils/discover/eventView';
 import {getFieldRenderer} from 'sentry/utils/discover/fieldRenderers';
-import {NumberContainer} from 'sentry/utils/discover/styles';
-import {formatPercentage} from 'sentry/utils/formatters';
 import {TableColumn} from 'sentry/views/discover/table/types';
 import {PercentChangeCell} from 'sentry/views/starfish/components/tableCells/percentChangeCell';
 import {FailureSpike} from 'sentry/views/starfish/views/webServiceView/types';
@@ -89,19 +87,7 @@ export default function FailureDetailTable({
     }
 
     if (field === 'http_error_count_percent_change()') {
-      const deltaValue = dataRow[field] as number;
-      const trendDirection = deltaValue < 0 ? 'good' : deltaValue > 0 ? 'bad' : 'neutral';
-
-      return (
-        <NumberContainer>
-          <PercentChangeCell trendDirection={trendDirection}>
-            {tct('[sign][delta]', {
-              sign: deltaValue >= 0 ? '+' : '-',
-              delta: formatPercentage(Math.abs(deltaValue), 2),
-            })}
-          </PercentChangeCell>
-        </NumberContainer>
-      );
+      return <PercentChangeCell deltaValue={dataRow[field] as number} />;
     }
 
     return rendered;