Browse Source

fix(ddm-alerts): Hide unit for count operation (#60961)

ArthurKnaus 1 year ago
parent
commit
60f0984d04

+ 11 - 0
static/app/utils/metrics/index.spec.tsx

@@ -156,6 +156,17 @@ describe('formatMetricUsingFixedUnit', () => {
     expect(formatMetricUsingFixedUnit(1234.56, 'randomunitname')).toBe('1,234.56');
     expect(formatMetricUsingFixedUnit(1234.56, 'none')).toBe('1,234.56');
   });
+
+  it.each(['sum', 'count_unique', 'avg', 'max', 'p50', 'p75', 'p95', 'p99'])(
+    'does append a unit for every operation (except count)',
+    op => {
+      expect(formatMetricUsingFixedUnit(1234.56, 'second', op)).toMatch(/1,234\.56s/);
+    }
+  );
+
+  it('does not append a unit for count operation', () => {
+    expect(formatMetricUsingFixedUnit(1234.56, 'second', 'count')).toBe('1,234.56');
+  });
 });
 
 describe('getDateTimeParams', () => {

+ 12 - 4
static/app/utils/metrics/index.tsx

@@ -313,10 +313,18 @@ const METRIC_UNIT_TO_SHORT: Record<FormattingSupportedMetricUnit, string> = {
 
 const getShortMetricUnit = (unit: string): string => METRIC_UNIT_TO_SHORT[unit] ?? '';
 
-export function formatMetricUsingFixedUnit(value: number | null, unit: string) {
-  return value !== null
-    ? `${round(value, 3).toLocaleString()}${getShortMetricUnit(unit)}`.trim()
-    : '\u2015';
+export function formatMetricUsingFixedUnit(
+  value: number | null,
+  unit: string,
+  op?: string
+) {
+  if (value === null) {
+    return '\u2014';
+  }
+
+  return op === 'count'
+    ? round(value, 3).toLocaleString()
+    : `${round(value, 3).toLocaleString()}${getShortMetricUnit(unit)}`.trim();
 }
 
 export function formatMetricsUsingUnitAndOp(

+ 1 - 1
static/app/views/alerts/rules/metric/constants.tsx

@@ -179,7 +179,7 @@ function getAlertTimeWindow(period: string | undefined): TimeWindow | undefined
 
   const timeWindows = Object.values(TimeWindow)
     .filter((value): value is TimeWindow => typeof value === 'number')
-    .sort();
+    .sort((a, b) => a - b);
 
   for (let index = 0; index < timeWindows.length; index++) {
     const timeWindow = timeWindows[index];

+ 4 - 4
static/app/views/alerts/utils/index.tsx

@@ -147,9 +147,9 @@ export function alertAxisFormatter(value: number, seriesName: string, aggregate:
   }
 
   if (isCustomMetricAlert(aggregate)) {
-    const {mri} = parseField(aggregate)!;
+    const {mri, op} = parseField(aggregate)!;
     const {unit} = parseMRI(mri)!;
-    return formatMetricUsingFixedUnit(value, unit);
+    return formatMetricUsingFixedUnit(value, unit, op);
   }
 
   return axisLabelFormatter(value, aggregateOutputType(seriesName));
@@ -165,9 +165,9 @@ export function alertTooltipValueFormatter(
   }
 
   if (isCustomMetricAlert(aggregate)) {
-    const {mri} = parseField(aggregate)!;
+    const {mri, op} = parseField(aggregate)!;
     const {unit} = parseMRI(mri)!;
-    return formatMetricUsingFixedUnit(value, unit);
+    return formatMetricUsingFixedUnit(value, unit, op);
   }
 
   return tooltipFormatter(value, aggregateOutputType(seriesName));

+ 7 - 7
static/app/views/ddm/createAlertModal.tsx

@@ -232,22 +232,22 @@ export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
   ]);
 
   const unit = parseMRI(metricsQuery.mri)?.unit ?? 'none';
-  const chartOptions = useMemo(
-    () => ({
+  const operation = metricsQuery.op;
+  const chartOptions = useMemo(() => {
+    return {
       isGroupedByDate: true,
       height: 200,
       grid: {top: 20, bottom: 20, left: 15, right: 25},
       tooltip: {
-        valueFormatter: value => formatMetricUsingFixedUnit(value, unit),
+        valueFormatter: value => formatMetricUsingFixedUnit(value, unit, operation),
       },
       yAxis: {
         axisLabel: {
-          formatter: value => formatMetricUsingFixedUnit(value, unit),
+          formatter: value => formatMetricUsingFixedUnit(value, unit, operation),
         },
       },
-    }),
-    [unit]
-  );
+    };
+  }, [operation, unit]);
 
   return (
     <Fragment>