Browse Source

feat(ddm): Improve chart series names (#64758)

### Problem
Our current series naming has multiple problems.

1. Dependent on both data and local component state leading to visual
glitches when updating groupings.
2. Long names with the names of grouping values being repetitive
elements
  e.g. `result_type:success, environment:prod`
3. if there is only a single group we simply showed the field name, not
exposing any information about the returned group

### Solutions

1. Only format based on the timeseries response. It includes all needed
information.
2. Remove grouping names from the series name -> `success, environment`.
Keep the grouping names in the tooltip when hovering in the summary
table. Adding groups is a conscious choice the user makes, expecting a
certain outcome. So my assumption is that in most cases they won't need
the name of the group value. If they need it, it is accessible via the
tooltip.
3. Always show the grouping values, even if there is only a single group

- relates to https://github.com/getsentry/sentry/issues/64021
ArthurKnaus 1 year ago
parent
commit
48768ae78d

+ 5 - 9
static/app/utils/metrics/index.tsx

@@ -256,13 +256,9 @@ export function useClearQuery() {
   }, [routerRef]);
 }
 
-// TODO(ddm): there has to be a nicer way to do this
-export function getSeriesName(
-  group: MetricsGroup,
-  isOnlyGroup = false,
-  groupBy: MetricsQuery['groupBy']
-) {
-  if (isOnlyGroup && !groupBy?.length) {
+export function getMetricsSeriesName(group: MetricsGroup) {
+  const groupByEntries = Object.entries(group.by ?? {});
+  if (!groupByEntries.length) {
     const field = Object.keys(group.series)?.[0];
     const {mri} = parseField(field) ?? {mri: field};
     const name = formatMRI(mri as MRI);
@@ -270,8 +266,8 @@ export function getSeriesName(
     return name ?? '(none)';
   }
 
-  return Object.entries(group.by)
-    .map(([key, value]) => `${key}:${String(value).length ? value : t('none')}`)
+  return groupByEntries
+    .map(([_key, value]) => `${String(value).length ? value : t('(none)')}`)
     .join(', ');
 }
 

+ 6 - 4
static/app/views/dashboards/datasetConfig/metrics.tsx

@@ -13,7 +13,11 @@ import type {CustomMeasurementCollection} from 'sentry/utils/customMeasurements/
 import type {TableData} from 'sentry/utils/discover/discoverQuery';
 import type {EventData} from 'sentry/utils/discover/eventView';
 import {NumberContainer} from 'sentry/utils/discover/styles';
-import {getMetricsApiRequestQuery, getSeriesName, groupByOp} from 'sentry/utils/metrics';
+import {
+  getMetricsApiRequestQuery,
+  getMetricsSeriesName,
+  groupByOp,
+} from 'sentry/utils/metrics';
 import {formatMetricUsingUnit} from 'sentry/utils/metrics/formatters';
 import {
   formatMRIField,
@@ -352,9 +356,7 @@ export function transformMetricsResponseToSeries(
   data.groups.forEach(group => {
     Object.keys(group.series).forEach(field => {
       results.push({
-        seriesName:
-          queryAlias ||
-          getSeriesName(group, data.groups.length === 1, widgetQuery.columns),
+        seriesName: queryAlias || getMetricsSeriesName(group),
         data: data.intervals.map((interval, index) => ({
           name: interval,
           value: group.series[field][index] ?? 0,

+ 1 - 2
static/app/views/dashboards/widgetCard/metricWidgetCard/index.tsx

@@ -242,10 +242,9 @@ export function MetricWidgetChartContainer({
       ? getChartTimeseries(timeseriesData, {
           getChartPalette: createChartPalette,
           mri,
-          groupBy,
         })
       : [];
-  }, [timeseriesData, mri, groupBy]);
+  }, [timeseriesData, mri]);
 
   if (isError) {
     const errorMessage =

+ 2 - 0
static/app/views/ddm/chart.tsx

@@ -160,6 +160,8 @@ export const MetricChart = forwardRef<ReactEchartsRef, ChartProps>(
             if (!isChartHovered(chartRef?.current)) {
               return '';
             }
+
+            // Hovering a single correlated sample datapoint
             if (params.seriesType === 'scatter') {
               return getFormatter(samples.formatters)(params, asyncTicket);
             }

+ 0 - 1
static/app/views/ddm/createAlertModal.tsx

@@ -154,7 +154,6 @@ export function CreateAlertModal({Header, Body, Footer, metricsQuery}: Props) {
       getChartTimeseries(data, {
         mri: metricsQuery.mri,
         focusedSeries: undefined,
-        groupBy: [],
         // We are limited to one series in this chart, so we can just use the first color
         getChartPalette: createChartPalette,
       }),

+ 32 - 7
static/app/views/ddm/summaryTable.tsx

@@ -116,7 +116,6 @@ export const SummaryTable = memo(function SummaryTable({
       return {
         ...s,
         ...getValues(s.data),
-        name: s.seriesName,
       };
     })
     .sort((a, b) => {
@@ -127,8 +126,8 @@ export const SummaryTable = memo(function SummaryTable({
 
       if (name === 'name') {
         return order === 'asc'
-          ? a.name.localeCompare(b.name)
-          : b.name.localeCompare(a.name);
+          ? a.seriesName.localeCompare(b.seriesName)
+          : b.seriesName.localeCompare(a.seriesName);
       }
       const aValue = a[name] ?? 0;
       const bValue = b[name] ?? 0;
@@ -168,7 +167,6 @@ export const SummaryTable = memo(function SummaryTable({
       >
         {rows.map(
           ({
-            name,
             seriesName,
             groupBy,
             color,
@@ -211,12 +209,11 @@ export const SummaryTable = memo(function SummaryTable({
                   </Cell>
                   <TextOverflowCell>
                     <Tooltip
-                      title={name}
-                      showOnlyOnOverflow
+                      title={<FullSeriesName seriesName={seriesName} groupBy={groupBy} />}
                       delay={500}
                       overlayStyle={{maxWidth: '80vw'}}
                     >
-                      <TextOverflow>{name}</TextOverflow>
+                      <TextOverflow>{seriesName}</TextOverflow>
                     </Tooltip>
                   </TextOverflowCell>
                   {/* TODO(ddm): Add a tooltip with the full value, don't add on click in case users want to copy the value */}
@@ -259,6 +256,34 @@ export const SummaryTable = memo(function SummaryTable({
   );
 });
 
+function FullSeriesName({
+  seriesName,
+  groupBy,
+}: {
+  seriesName: string;
+  groupBy?: Record<string, string>;
+}) {
+  if (!groupBy || Object.keys(groupBy).length === 0) {
+    return <Fragment>{seriesName}</Fragment>;
+  }
+
+  const goupByEntries = Object.entries(groupBy);
+  return (
+    <Fragment>
+      {goupByEntries.map(([key, value], index) => {
+        const formattedValue = value || t('(none)');
+        return (
+          <span key={key}>
+            <strong>{`${key}:`}</strong>
+            &nbsp;
+            {index === goupByEntries.length - 1 ? formattedValue : `${formattedValue}, `}
+          </span>
+        );
+      })}
+    </Fragment>
+  );
+}
+
 function SortableHeaderCell({
   sortState,
   name,

+ 3 - 12
static/app/views/ddm/widget.tsx

@@ -19,7 +19,7 @@ import type {MetricsApiResponse, MRI, PageFilters} from 'sentry/types';
 import type {ReactEchartsRef} from 'sentry/types/echarts';
 import {
   getDefaultMetricDisplayType,
-  getSeriesName,
+  getMetricsSeriesName,
   stringifyMetricWidget,
 } from 'sentry/utils/metrics';
 import {metricDisplayTypeOptions} from 'sentry/utils/metrics/constants';
@@ -289,16 +289,9 @@ const MetricWidgetBody = memo(
             getChartPalette,
             mri,
             focusedSeries: focusedSeries?.seriesName,
-            groupBy: metricsQuery.groupBy,
           })
         : [];
-    }, [
-      timeseriesData,
-      getChartPalette,
-      mri,
-      focusedSeries?.seriesName,
-      metricsQuery.groupBy,
-    ]);
+    }, [timeseriesData, getChartPalette, mri, focusedSeries?.seriesName]);
 
     const handleSortChange = useCallback(
       newSort => {
@@ -367,12 +360,10 @@ export function getChartTimeseries(
     getChartPalette,
     mri,
     focusedSeries,
-    groupBy,
   }: {
     getChartPalette: (seriesNames: string[]) => Record<string, string>;
     mri: MRI;
     focusedSeries?: string;
-    groupBy?: string[];
   }
 ) {
   // this assumes that all series have the same unit
@@ -382,7 +373,7 @@ export function getChartTimeseries(
   const series = data.groups.map(g => {
     return {
       values: Object.values(g.series)[0],
-      name: getSeriesName(g, data.groups.length === 1, groupBy),
+      name: getMetricsSeriesName(g),
       groupBy: g.by,
       transaction: g.by.transaction,
       release: g.by.release,