Browse Source

fix(flamechart): improve messaging and throw if colors are out of bounds (#65253)

Jonas 1 year ago
parent
commit
d1dd7a1a22

+ 19 - 18
static/app/components/profiling/flamegraph/flamegraphChart.tsx

@@ -9,7 +9,7 @@ import {useCanvasScheduler} from 'sentry/utils/profiling/canvasScheduler';
 import type {CanvasView} from 'sentry/utils/profiling/canvasView';
 import {useFlamegraphTheme} from 'sentry/utils/profiling/flamegraph/useFlamegraphTheme';
 import type {FlamegraphCanvas} from 'sentry/utils/profiling/flamegraphCanvas';
-import {FlamegraphChart as FlamegraphChartModel} from 'sentry/utils/profiling/flamegraphChart';
+import type {FlamegraphChart as FlamegraphChartModel} from 'sentry/utils/profiling/flamegraphChart';
 import {
   getConfigViewTranslationBetweenVectors,
   getPhysicalSpacePositionFromOffset,
@@ -261,23 +261,24 @@ export function FlamegraphChart({
     [configSpaceCursor, chartView]
   );
 
-  let message = t('Profile has no measurements');
-  const canRenderChart = chart?.series.every(
-    s => s.points.length >= FlamegraphChartModel.MIN_RENDERABLE_POINTS
-  );
-
-  if (chart && !canRenderChart) {
-    const profileIsTooShortToDisplayMeasurements = chart.configSpace.width < 200 * 1e6;
-    const didNotCollectAnyMeasurements = chart.series.every(s => s.points.length === 0);
-
-    if (profileIsTooShortToDisplayMeasurements) {
-      message = t(
-        'Profile is too short to display measurements, minimum duration is at least 200ms'
-      );
-    }
-
-    if (didNotCollectAnyMeasurements) {
+  let message: string | undefined;
+
+  if (chart) {
+    if (
+      chart.configSpace.width < 200 * 1e6 &&
+      (chart.status === 'insufficient data' || chart.status === 'empty metrics')
+    ) {
+      message = t('Profile duration was too short to collect enough metrics');
+    } else if (chart.configSpace.width >= 200 && chart.status === 'insufficient data') {
+      message =
+        noMeasurementMessage ||
+        t(
+          'Profile failed to collect a sufficient amount of measurements to render a chart'
+        );
+    } else if (chart.status === 'no metrics') {
       message = noMeasurementMessage || t('Profile has no measurements');
+    } else if (chart.status === 'empty metrics') {
+      message = t('Profile has empty measurements');
     }
   }
 
@@ -304,7 +305,7 @@ export function FlamegraphChart({
       {/* transaction loads after profile, so we want to show loading even if it's in initial state */}
       {profiles.type === 'loading' || profiles.type === 'initial' ? (
         <CollapsibleTimelineLoadingIndicator />
-      ) : profiles.type === 'resolved' && !canRenderChart ? (
+      ) : profiles.type === 'resolved' && chart?.status !== 'ok' ? (
         <CollapsibleTimelineMessage>{message}</CollapsibleTimelineMessage>
       ) : null}
     </Fragment>

+ 11 - 0
static/app/utils/profiling/flamegraphChart.tsx

@@ -39,6 +39,7 @@ export class FlamegraphChart {
   tooltipFormatter: ReturnType<typeof makeFormatter>;
   timelineFormatter: (value: number) => string;
   series: Series[];
+  status: 'no metrics' | 'empty metrics' | 'insufficient data' | 'ok' = 'no metrics';
   domains: {
     x: [number, number];
     y: [number, number];
@@ -63,13 +64,22 @@ export class FlamegraphChart {
       this.formatter = makeFormatter('percent');
       this.tooltipFormatter = makeFormatter('percent');
       this.configSpace = configSpace.clone();
+      this.status = !measurements ? 'no metrics' : 'empty metrics';
       return;
     }
 
+    this.status = 'insufficient data';
     const type = options.type ? options.type : measurements.length > 1 ? 'line' : 'area';
 
     for (let j = 0; j < measurements.length; j++) {
       const measurement = measurements[j];
+
+      if (!colors[j]) {
+        throw new Error(
+          `No color provided for measurement, got ${colors.length} colors for ${measurements.length} measurements.`
+        );
+      }
+
       this.series[j] = {
         type,
         name: measurement.name,
@@ -85,6 +95,7 @@ export class FlamegraphChart {
         continue;
       }
 
+      this.status = 'ok';
       for (let i = 0; i < measurement.values.length; i++) {
         const m = measurement.values[i];