Browse Source

feat(starfish): Switch all `sps()` measurements to `spm()` (#53545)

This was a bit of a hike because we hard-coded the rate formatting in a
bunch of places, and also duplicated a bunch of code, so I had to do
some cleanup, too

The upshot is that "Spans per Second" is now "Spans per Minute" because
for lower-throughput projects, they were just getting 0 everywhere which
is a huge bummer. It's much easier to format a large number to be
friendly than to show a very small number.

## Changes

- Allow passing a rate _unit_ through to formatters. Surprisingly
there's not much precedent for this, even though even a `"number"` can
have a unit
- Improve `formatAbbreviatedNumber` by adding a few specs, and enforcing
consistent return type and formatting
- Change all references to `sps()` to `spm()`, and use abbreviated
number formatting

---------

Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
George Gritsouk 1 year ago
parent
commit
f3d17a3d3b

+ 3 - 3
static/app/utils/discover/charts.tsx

@@ -5,7 +5,7 @@ import {LegendComponentOption} from 'echarts';
 import {t} from 'sentry/locale';
 import {Series} from 'sentry/types/echarts';
 import {defined, formatBytesBase2} from 'sentry/utils';
-import {AggregationOutputType} from 'sentry/utils/discover/fields';
+import {AggregationOutputType, RateUnits} from 'sentry/utils/discover/fields';
 import {
   DAY,
   formatAbbreviatedNumber,
@@ -66,7 +66,7 @@ export function axisLabelFormatter(
   outputType: AggregationOutputType,
   abbreviation: boolean = false,
   durationUnit?: number,
-  rateUnit?: 's' | 'min' | 'hr'
+  rateUnit?: RateUnits
 ): string {
   return axisLabelFormatterUsingAggregateOutputType(
     value,
@@ -85,7 +85,7 @@ export function axisLabelFormatterUsingAggregateOutputType(
   type: string,
   abbreviation: boolean = false,
   durationUnit?: number,
-  rateUnit?: 's' | 'min' | 'hr'
+  rateUnit?: RateUnits
 ): string {
   switch (type) {
     case 'integer':

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

@@ -31,11 +31,16 @@ import {
   getSpanOperationName,
   isEquation,
   isRelativeSpanOperationBreakdownField,
+  RATE_UNIT_LABELS,
   SPAN_OP_BREAKDOWN_FIELDS,
   SPAN_OP_RELATIVE_BREAKDOWN_FIELD,
 } from 'sentry/utils/discover/fields';
 import {getShortEventId} from 'sentry/utils/events';
-import {formatFloat, formatPercentage} from 'sentry/utils/formatters';
+import {
+  formatAbbreviatedNumber,
+  formatFloat,
+  formatPercentage,
+} from 'sentry/utils/formatters';
 import getDynamicText from 'sentry/utils/getDynamicText';
 import Projects from 'sentry/utils/projects';
 import toArray from 'sentry/utils/toArray';
@@ -168,12 +173,6 @@ export const DURATION_UNITS = {
   week: 1000 * 60 * 60 * 24 * 7,
 };
 
-const RATE_UNIT_LABELS = {
-  '1/second': '/s',
-  '1/minute': '/min',
-  '1/hour': '/hr',
-};
-
 export const PERCENTAGE_UNITS = ['ratio', 'percent'];
 
 /**
@@ -238,7 +237,7 @@ export const FIELD_FORMATTERS: FieldFormatters = {
 
       return (
         <NumberContainer>
-          {`${formatFloat(data[field], 2)}${unit ? RATE_UNIT_LABELS[unit] : ''}`}
+          {`${formatAbbreviatedNumber(data[field])}${unit ? RATE_UNIT_LABELS[unit] : ''}`}
         </NumberContainer>
       );
     },

+ 12 - 0
static/app/utils/discover/fields.tsx

@@ -118,6 +118,18 @@ export type Column = QueryFieldValue;
 
 export type Alignments = 'left' | 'right';
 
+export enum RateUnits {
+  PER_SECOND = '1/second',
+  PER_MINUTE = '1/minute',
+  PER_HOUR = '1/hour',
+}
+
+export const RATE_UNIT_LABELS = {
+  [RateUnits.PER_SECOND]: '/s',
+  [RateUnits.PER_MINUTE]: '/min',
+  [RateUnits.PER_HOUR]: '/h',
+};
+
 const CONDITIONS_ARGUMENTS: SelectValue<string>[] = [
   {
     label: 'is equal to',

+ 6 - 0
static/app/utils/formatters.spec.tsx

@@ -196,6 +196,12 @@ describe('formatAbbreviatedNumber()', function () {
     expect(formatAbbreviatedNumber('100000000000')).toBe('100b');
     expect(formatAbbreviatedNumber('1000000000000')).toBe('1000b');
   });
+
+  it('should round to 1 decimal place', function () {
+    expect(formatAbbreviatedNumber(100.12)).toBe('100.12');
+    expect(formatAbbreviatedNumber(1500)).toBe('1.5k');
+    expect(formatAbbreviatedNumber(1213122)).toBe('1.2m');
+  });
 });
 
 describe('formatFloat()', function () {

+ 3 - 2
static/app/utils/formatters.tsx

@@ -3,6 +3,7 @@ import round from 'lodash/round';
 
 import {t, tn} from 'sentry/locale';
 import {CommitAuthor, User} from 'sentry/types';
+import {RATE_UNIT_LABELS, RateUnits} from 'sentry/utils/discover/fields';
 
 export function userDisplayName(user: User | CommitAuthor, includeEmail = true): string {
   let displayName = String(user?.name ?? t('Unknown author')).trim();
@@ -308,6 +309,6 @@ export function formatAbbreviatedNumber(number: number | string) {
   return number.toLocaleString();
 }
 
-export function formatRate(value: number, rateUnit?: string) {
-  return `${value} /${rateUnit ?? 's'}`;
+export function formatRate(value: number, unit: RateUnits = RateUnits.PER_SECOND) {
+  return `${formatAbbreviatedNumber(value)}${RATE_UNIT_LABELS[unit]}`;
 }

+ 9 - 2
static/app/views/starfish/components/chart.tsx

@@ -43,7 +43,11 @@ import {
   getDurationUnit,
   tooltipFormatter,
 } from 'sentry/utils/discover/charts';
-import {aggregateOutputType, AggregationOutputType} from 'sentry/utils/discover/fields';
+import {
+  aggregateOutputType,
+  AggregationOutputType,
+  RateUnits,
+} from 'sentry/utils/discover/fields';
 import usePageFilters from 'sentry/utils/usePageFilters';
 import useRouter from 'sentry/utils/useRouter';
 import {SpanMetricsFields} from 'sentry/views/starfish/types';
@@ -94,6 +98,7 @@ type Props = {
   onMouseOut?: EChartMouseOutHandler;
   onMouseOver?: EChartMouseOverHandler;
   previousData?: Series[];
+  rateUnit?: RateUnits;
   scatterPlot?: Series[];
   showLegend?: boolean;
   stacked?: boolean;
@@ -152,6 +157,7 @@ function Chart({
   disableXAxis,
   definedAxisTicks,
   durationUnit,
+  rateUnit,
   chartColors,
   isBarChart,
   isLineChart,
@@ -254,7 +260,8 @@ function Chart({
             value,
             aggregateOutputFormat ?? aggregateOutputType(data[0].seriesName),
             undefined,
-            durationUnit ?? getDurationUnit(data)
+            durationUnit ?? getDurationUnit(data),
+            rateUnit
           );
         },
       },

+ 1 - 0
static/app/views/starfish/components/tableCells/renderHeadCell.tsx

@@ -23,6 +23,7 @@ export const SORTABLE_FIELDS = new Set([
   `avg(${SPAN_SELF_TIME})`,
   `p95(${SPAN_SELF_TIME})`,
   'sps()',
+  'spm()',
   'time_spent_percentage()',
   'http_error_count()',
 ]);

+ 6 - 9
static/app/views/starfish/components/tableCells/throughputCell.tsx

@@ -1,23 +1,20 @@
-import {t} from 'sentry/locale';
+import {RateUnits} from 'sentry/utils/discover/fields';
 import {NumberContainer} from 'sentry/utils/discover/styles';
-import {formatAbbreviatedNumber} from 'sentry/utils/formatters';
+import {formatRate} from 'sentry/utils/formatters';
 
 type Props = {
+  unit: RateUnits;
   containerProps?: React.DetailedHTMLProps<
     React.HTMLAttributes<HTMLDivElement>,
     HTMLDivElement
   >;
-  throughputPerSecond?: number;
+  rate?: number;
 };
 
-export default function ThroughputCell({throughputPerSecond, containerProps}: Props) {
-  const throughput = throughputPerSecond
-    ? formatAbbreviatedNumber(throughputPerSecond)
-    : '--';
-
+export default function ThroughputCell({rate, unit, containerProps}: Props) {
   return (
     <NumberContainer {...containerProps}>
-      {throughput}/{t('s')}
+      {rate ? formatRate(rate, unit) : '--'}
     </NumberContainer>
   );
 }

+ 2 - 2
static/app/views/starfish/queries/useSpanList.tsx

@@ -20,7 +20,7 @@ export type SpanMetrics = {
   'span.domain': string;
   'span.group': string;
   'span.op': string;
-  'sps()': number;
+  'spm()': number;
   'sum(span.self_time)': number;
   'time_spent_percentage()': number;
   'time_spent_percentage(local)': number;
@@ -82,7 +82,7 @@ function getEventView(
     SPAN_GROUP,
     SPAN_DESCRIPTION,
     SPAN_DOMAIN,
-    'sps()',
+    'spm()',
     `sum(${SPAN_SELF_TIME})`,
     `avg(${SPAN_SELF_TIME})`,
     'http_error_count()',

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

@@ -13,7 +13,7 @@ export type SpanMetrics = {
   'http_error_count()': number;
   'p95(span.self_time)': number;
   'span.op': string;
-  'sps()': number;
+  'spm()': number;
   'time_spent_percentage()': number;
 };
 

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