Flamegraph: Update threshold for collapsing and fix flickering (#78206)

packages/grafana-flamegraph/src/FlameGraph/FlameGraph.tsx

@@ -17,7 +17,7 @@
 // TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
 // THIS SOFTWARE.
 import { css, cx } from '@emotion/css';
-import React, { useEffect, useMemo, useState } from 'react';
+import React, { useEffect, useState } from 'react';
 
 import { Icon } from '@grafana/ui';
 
@@ -26,7 +26,7 @@ import { ClickedItemData, ColorScheme, ColorSchemeDiff, TextAlign } from '../typ
 
 import FlameGraphCanvas from './FlameGraphCanvas';
 import FlameGraphMetadata from './FlameGraphMetadata';
-import { CollapsedMap, FlameGraphDataContainer } from './dataTransform';
+import { CollapsedMap, FlameGraphDataContainer, LevelItem } from './dataTransform';
 
 type Props = {
   data: FlameGraphDataContainer;
@@ -66,30 +66,42 @@ const FlameGraph = ({
 }: Props) => {
   const styles = getStyles();
 
-  const [collapsedMap, setCollapsedMap] = useState<CollapsedMap>(data ? data.getCollapsedMap() : new Map());
+  const [collapsedMap, setCollapsedMap] = useState<CollapsedMap>(new Map());
+  const [levels, setLevels] = useState<LevelItem[][]>();
+  const [levelsCallers, setLevelsCallers] = useState<LevelItem[][]>();
+  const [totalProfileTicks, setTotalProfileTicks] = useState<number>(0);
+  const [totalProfileTicksRight, setTotalProfileTicksRight] = useState<number>();
+  const [totalViewTicks, setTotalViewTicks] = useState<number>(0);
+
   useEffect(() => {
     if (data) {
       setCollapsedMap(data.getCollapsedMap());
+
+      let levels = data.getLevels();
+      let totalProfileTicks = levels.length ? levels[0][0].value : 0;
+      let totalProfileTicksRight = levels.length ? levels[0][0].valueRight : undefined;
+      let totalViewTicks = totalProfileTicks;
+      let levelsCallers = undefined;
+
+      if (sandwichItem) {
+        const [callers, callees] = data.getSandwichLevels(sandwichItem);
+        levels = callees;
+        levelsCallers = callers;
+        // We need this separate as in case of diff profile we want to compute diff colors based on the original ticks.
+        totalViewTicks = callees[0]?.[0]?.value ?? 0;
+      }
+      setLevels(levels);
+      setLevelsCallers(levelsCallers);
+      setTotalProfileTicks(totalProfileTicks);
+      setTotalProfileTicksRight(totalProfileTicksRight);
+      setTotalViewTicks(totalViewTicks);
     }
-  }, [data]);
-
-  const [levels, levelsCallers, totalProfileTicks, totalProfileTicksRight, totalViewTicks] = useMemo(() => {
-    let levels = data.getLevels();
-    let totalProfileTicks = levels.length ? levels[0][0].value : 0;
-    let totalProfileTicksRight = levels.length ? levels[0][0].valueRight : undefined;
-    let totalViewTicks = totalProfileTicks;
-    let levelsCallers = undefined;
-
-    if (sandwichItem) {
-      const [callers, callees] = data.getSandwichLevels(sandwichItem);
-      levels = callees;
-      levelsCallers = callers;
-      // We need this separate as in case of diff profile we want to compute diff colors based on the original ticks.
-      totalViewTicks = callees[0]?.[0]?.value ?? 0;
-    }
-    return [levels, levelsCallers, totalProfileTicks, totalProfileTicksRight, totalViewTicks];
   }, [data, sandwichItem]);
 
+  if (!levels) {
+    return null;
+  }
+
   const commonCanvasProps = {
     data,
     rangeMin,

packages/grafana-flamegraph/src/FlameGraph/dataTransform.test.ts

@@ -1,6 +1,6 @@
 import { createDataFrame, FieldType } from '@grafana/data';
 
-import { FlameGraphDataContainer, LevelItem, nestedSetToLevels } from './dataTransform';
+import { CollapsedMapContainer, FlameGraphDataContainer, LevelItem, nestedSetToLevels } from './dataTransform';
 import { textToDataContainer } from './testHelpers';
 
 describe('nestedSetToLevels', () => {
@@ -90,6 +90,20 @@ describe('FlameGraphDataContainer', () => {
     });
   });
 
+  it('creates correct collapse map 2', () => {
+    // Should not create any groups because even though the 1 is within threshold it has a sibling
+    const container = textToDataContainer(
+      `
+      [0////////////////////////////////]
+      [1/////////////////////////////][2]
+    `,
+      { collapsing: true, collapsingThreshold: 0.5 }
+    )!;
+
+    const collapsedMap = container.getCollapsedMap();
+    expect(Array.from(collapsedMap.keys()).length).toEqual(0);
+  });
+
   it('creates empty collapse map if no items are similar', () => {
     const container = textToDataContainer(`
       [0//////////////]
@@ -101,3 +115,81 @@ describe('FlameGraphDataContainer', () => {
     expect(Array.from(collapsedMap.keys()).length).toEqual(0);
   });
 });
+
+describe('CollapsedMapContainer', () => {
+  const defaultItem: LevelItem = {
+    itemIndexes: [0],
+    value: 100,
+    level: 0,
+    children: [],
+    start: 0,
+  };
+
+  it('groups items if they are within value threshold', () => {
+    const container = new CollapsedMapContainer();
+
+    const child2: LevelItem = {
+      ...defaultItem,
+      itemIndexes: [2],
+      value: 99.1,
+    };
+
+    const child1: LevelItem = {
+      ...defaultItem,
+      itemIndexes: [1],
+      children: [child2],
+    };
+
+    const parent: LevelItem = {
+      ...defaultItem,
+      children: [child1],
+    };
+
+    container.addItem(child1, parent);
+    container.addItem(child2, child1);
+    expect(container.getMap().get(child1)).toMatchObject({ collapsed: true, items: [parent, child1, child2] });
+    expect(container.getMap().get(child2)).toMatchObject({ collapsed: true, items: [parent, child1, child2] });
+    expect(container.getMap().get(parent)).toMatchObject({ collapsed: true, items: [parent, child1, child2] });
+  });
+
+  it("doesn't group items if they are outside value threshold", () => {
+    const container = new CollapsedMapContainer();
+
+    const parent: LevelItem = {
+      ...defaultItem,
+    };
+
+    const child: LevelItem = {
+      ...defaultItem,
+      itemIndexes: [1],
+      value: 98,
+    };
+
+    container.addItem(child, parent);
+    expect(container.getMap().size).toBe(0);
+  });
+
+  it("doesn't group items if parent has multiple children", () => {
+    const container = new CollapsedMapContainer();
+
+    const child1: LevelItem = {
+      ...defaultItem,
+      itemIndexes: [1],
+      value: 99.1,
+    };
+
+    const child2: LevelItem = {
+      ...defaultItem,
+      itemIndexes: [2],
+      value: 0.09,
+    };
+
+    const parent: LevelItem = {
+      ...defaultItem,
+      children: [child1, child2],
+    };
+
+    container.addItem(child1, parent);
+    expect(container.getMap().size).toBe(0);
+  });
+});

packages/grafana-flamegraph/src/FlameGraph/dataTransform.ts

@@ -34,24 +34,19 @@ export type CollapseConfig = {
   collapsed: boolean;
 };
 
-export type CollapsedMap = Map<LevelItem, CollapseConfig>;
-
 /**
  * Convert data frame with nested set format into array of level. This is mainly done for compatibility with current
  * rendering code.
  */
 export function nestedSetToLevels(
   container: FlameGraphDataContainer,
-  options?: {
-    collapsing: boolean;
-  }
+  options?: Options
 ): [LevelItem[][], Record<string, LevelItem[]>, CollapsedMap] {
   const levels: LevelItem[][] = [];
   let offset = 0;
 
   let parent: LevelItem | undefined = undefined;
   const uniqueLabels: Record<string, LevelItem[]> = {};
-  const collapsedMap: CollapsedMap = new Map();
 
   for (let i = 0; i < container.data.length; i++) {
     const currentLevel = container.getLevel(i);
@@ -82,22 +77,6 @@ export function nestedSetToLevels(
       level: currentLevel,
     };
 
-    if (options?.collapsing) {
-      // We collapse similar items here, where it seems like parent and child are the same thing and so the distinction
-      // isn't that important. We create a map of items that should be collapsed together.
-      if (parent && newItem.value === parent.value) {
-        if (collapsedMap.has(parent)) {
-          const config = collapsedMap.get(parent)!;
-          collapsedMap.set(newItem, config);
-          config.items.push(newItem);
-        } else {
-          const config = { items: [parent, newItem], collapsed: true };
-          collapsedMap.set(parent, config);
-          collapsedMap.set(newItem, config);
-        }
-      }
-    }
-
     if (uniqueLabels[container.getLabel(i)]) {
       uniqueLabels[container.getLabel(i)].push(newItem);
     } else {
@@ -107,12 +86,68 @@ export function nestedSetToLevels(
     if (parent) {
       parent.children.push(newItem);
     }
-    parent = newItem;
 
+    parent = newItem;
     levels[currentLevel].push(newItem);
   }
 
-  return [levels, uniqueLabels, collapsedMap];
+  const collapsedMapContainer = new CollapsedMapContainer(options?.collapsingThreshold);
+  if (options?.collapsing) {
+    // We collapse similar items here, where it seems like parent and child are the same thing and so the distinction
+    // isn't that important. We create a map of items that should be collapsed together. We need to do it with complete
+    // tree as we need to know how many children an item has to know if we can collapse it.
+    collapsedMapContainer.addTree(levels[0][0]);
+  }
+
+  return [levels, uniqueLabels, collapsedMapContainer.getMap()];
+}
+
+export type CollapsedMap = Map<LevelItem, CollapseConfig>;
+export class CollapsedMapContainer {
+  private map = new Map();
+  private threshold = 0.99;
+
+  constructor(threshold?: number) {
+    if (threshold !== undefined) {
+      this.threshold = threshold;
+    }
+  }
+
+  addTree(root: LevelItem) {
+    const stack = [root];
+    while (stack.length) {
+      const current = stack.shift()!;
+
+      if (current.parents?.length) {
+        this.addItem(current, current.parents[0]);
+      }
+
+      if (current.children.length) {
+        stack.unshift(...current.children);
+      }
+    }
+  }
+
+  addItem(item: LevelItem, parent?: LevelItem) {
+    // The heuristics here is pretty simple right now. Just check if it's single child and if we are within threshold.
+    // We assume items with small self just aren't too important while we cannot really collapse items with siblings
+    // as it's not clear what to do with said sibling.
+    if (parent && item.value > parent.value * this.threshold && parent.children.length === 1) {
+      if (this.map.has(parent)) {
+        const config = this.map.get(parent)!;
+        this.map.set(item, config);
+        config.items.push(item);
+      } else {
+        const config = { items: [parent, item], collapsed: true };
+        this.map.set(parent, config);
+        this.map.set(item, config);
+      }
+    }
+  }
+
+  getMap() {
+    return new Map(this.map);
+  }
 }
 
 export function getMessageCheckFieldsResult(wrongFields: CheckFieldsResult) {
@@ -166,9 +201,14 @@ export function checkFields(data: DataFrame): CheckFieldsResult | undefined {
   return undefined;
 }
 
+export type Options = {
+  collapsing: boolean;
+  collapsingThreshold?: number;
+};
+
 export class FlameGraphDataContainer {
   data: DataFrame;
-  options: { collapsing: boolean };
+  options: Options;
 
   labelField: Field;
   levelField: Field;
@@ -187,7 +227,7 @@ export class FlameGraphDataContainer {
   private uniqueLabelsMap: Record<string, LevelItem[]> | undefined;
   private collapsedMap: Map<LevelItem, CollapseConfig> | undefined;
 
-  constructor(data: DataFrame, options: { collapsing: boolean }, theme: GrafanaTheme2 = createTheme()) {
+  constructor(data: DataFrame, options: Options, theme: GrafanaTheme2 = createTheme()) {
     this.data = data;
     this.options = options;
 

packages/grafana-flamegraph/src/FlameGraph/rendering.test.ts

@@ -28,7 +28,7 @@ describe('walkTree', () => {
   it('correctly compute sizes for a single item', () => {
     const root: LevelItem = { start: 0, itemIndexes: [0], children: [], value: 100, level: 0 };
     const container = new FlameGraphDataContainer(
-      makeDataFrame({ value: [100], level: [1], label: ['1'], self: [0] }),
+      makeDataFrame({ value: [100], level: [0], label: ['1'], self: [0] }),
       { collapsing: true }
     );
 

packages/grafana-flamegraph/src/FlameGraph/testHelpers.ts

@@ -1,6 +1,6 @@
 import { arrayToDataFrame, FieldType } from '@grafana/data';
 
-import { FlameGraphDataContainer, LevelItem } from './dataTransform';
+import { FlameGraphDataContainer, LevelItem, Options } from './dataTransform';
 
 // Convert text to a FlameGraphDataContainer for testing. The format representing the flamegraph for example:
 // [0///////]
@@ -9,7 +9,7 @@ import { FlameGraphDataContainer, LevelItem } from './dataTransform';
 // [3]  [6]
 //      [7]
 // Each node starts with [ ends with ], single digit is used for label and the length of a node is it's value.
-export function textToDataContainer(text: string) {
+export function textToDataContainer(text: string, options?: Options) {
   const levels = text.split('\n');
 
   if (levels.length === 0) {
@@ -80,7 +80,7 @@ export function textToDataContainer(text: string) {
   const df = arrayToDataFrame(dfSorted);
   const labelField = df.fields.find((f) => f.name === 'label')!;
   labelField.type = FieldType.string;
-  return new FlameGraphDataContainer(df, { collapsing: true });
+  return new FlameGraphDataContainer(df, options || { collapsing: true });
 }
 
 export function trimLevelsString(s: string) {