Browse Source

fix(profiling): fix highlight frame initializer and package for node profiles (#43705)

qs.parse will initialize blank values to "" so when we perform our
search the equality check fails as frame properties are initialized to
undefined values. Additionally, for node, the links generated from
suspect functions do not contain a package (as inference is currently
done on the frontend), so in case we find no results we perform another
search only by name.
Jonas 2 years ago
parent
commit
989c896190

+ 17 - 2
static/app/components/profiling/flamegraph/flamegraph.tsx

@@ -353,12 +353,27 @@ function Flamegraph(): ReactElement {
             previousView.configView.withHeight(newView.configView.height)
           );
         }
-      } else if (defined(highlightFrames)) {
-        const frames = flamegraph.findAllMatchingFrames(
+      }
+
+      if (defined(highlightFrames)) {
+        let frames = flamegraph.findAllMatchingFrames(
           highlightFrames.name,
           highlightFrames.package
         );
 
+        if (
+          !frames.length &&
+          !highlightFrames.package &&
+          highlightFrames.name &&
+          profileGroup.metadata.platform === 'node'
+        ) {
+          // there is a chance that the reason we did not find any frames is because
+          // for node, we try to infer some package from the frontend code.
+          // If that happens, we'll try and just do a search by name. This logic
+          // is duplicated in flamegraphZoomView.tsx and should be kept in sync
+          frames = flamegraph.findAllMatchingFramesBy(highlightFrames.name, ['name']);
+        }
+
         if (frames.length > 0) {
           const rectFrames = frames.map(
             f => new Rect(f.start, f.depth, f.end - f.start, 1)

+ 4 - 1
static/app/components/profiling/flamegraph/flamegraphDrawer/flamegraphTreeTable.tsx

@@ -136,7 +136,10 @@ export function FlamegraphTreeTable({
     }
 
     canvasPoolManager.dispatch('highlight frame', [
-      flamegraph.findAllMatchingFrames(clickedContextMenuNode.node),
+      flamegraph.findAllMatchingFrames(
+        clickedContextMenuNode.node.frame.name,
+        clickedContextMenuNode.node.frame.image
+      ),
       'selected',
     ]);
   }, [canvasPoolManager, clickedContextMenuNode, flamegraph]);

+ 21 - 2
static/app/components/profiling/flamegraph/flamegraphZoomView.tsx

@@ -265,10 +265,26 @@ function FlamegraphZoomView({
 
   useEffect(() => {
     if (flamegraphState.profiles.highlightFrames) {
-      selectedFramesRef.current = flamegraph.findAllMatchingFrames(
+      let frames = flamegraph.findAllMatchingFrames(
         flamegraphState.profiles.highlightFrames.name,
         flamegraphState.profiles.highlightFrames.package
       );
+
+      // there is a chance that the reason we did not find any frames is because
+      // for node, we try to infer some package from the frontend code.
+      // If that happens, we'll try and just do a search by name. This logic
+      // is duplicated in flamegraph.tsx and should be kept in sync
+      if (
+        !frames.length &&
+        !flamegraphState.profiles.highlightFrames.package &&
+        flamegraphState.profiles.highlightFrames.name
+      ) {
+        frames = flamegraph.findAllMatchingFramesBy(
+          flamegraphState.profiles.highlightFrames.name,
+          ['name']
+        );
+      }
+      selectedFramesRef.current = frames;
     } else {
       selectedFramesRef.current = null;
     }
@@ -597,7 +613,10 @@ function FlamegraphZoomView({
       },
     });
 
-    const frames = flamegraph.findAllMatchingFrames(hoveredNodeOnContextMenuOpen.current);
+    const frames = flamegraph.findAllMatchingFrames(
+      hoveredNodeOnContextMenuOpen.current.frame.name,
+      hoveredNodeOnContextMenuOpen.current.frame.image
+    );
     const rectFrames = frames.map(f => new Rect(f.start, f.depth, f.end - f.start, 1));
     const newConfigView = computeMinZoomConfigViewForFrames(
       flamegraphView.configView,

+ 29 - 16
static/app/utils/profiling/flamegraph.ts

@@ -308,29 +308,27 @@ export class Flamegraph {
     return frames;
   }
 
-  findAllMatchingFrames(
-    frameOrName: FlamegraphFrame | string,
-    packageName?: string
+  findAllMatchingFramesBy(
+    query: string,
+    fields: (keyof FlamegraphFrame['frame'])[]
   ): FlamegraphFrame[] {
     const matches: FlamegraphFrame[] = [];
+    if (!fields.length) {
+      throw new Error('No fields provided');
+    }
 
-    if (typeof frameOrName === 'string') {
+    if (fields.length === 1) {
       for (let i = 0; i < this.frames.length; i++) {
-        if (
-          this.frames[i].frame.name === frameOrName &&
-          // the image name on a frame is optional,
-          // treat it the same as the empty string
-          (this.frames[i].frame.image || '') === packageName
-        ) {
+        if (this.frames[i].frame[fields[0]] === query) {
           matches.push(this.frames[i]);
         }
       }
-    } else {
-      for (let i = 0; i < this.frames.length; i++) {
-        if (
-          this.frames[i].frame.name === frameOrName.node.frame.name &&
-          this.frames[i].frame.image === frameOrName.node.frame.image
-        ) {
+      return matches;
+    }
+
+    for (let i = 0; i < this.frames.length; i++) {
+      for (let j = fields.length; j--; ) {
+        if (this.frames[i].frame[fields[j]] === query) {
           matches.push(this.frames[i]);
         }
       }
@@ -338,4 +336,19 @@ export class Flamegraph {
 
     return matches;
   }
+
+  findAllMatchingFrames(frameName?: string, framePackage?: string): FlamegraphFrame[] {
+    const matches: FlamegraphFrame[] = [];
+
+    for (let i = 0; i < this.frames.length; i++) {
+      if (
+        this.frames[i].frame.name === frameName &&
+        this.frames[i].frame.image === framePackage
+      ) {
+        matches.push(this.frames[i]);
+      }
+    }
+
+    return matches;
+  }
 }

+ 7 - 4
static/app/utils/profiling/flamegraph/flamegraphStateProvider/flamegraphContextProvider.tsx

@@ -16,8 +16,8 @@ import {
 
 function isValidHighlightFrame(
   frame: Partial<FlamegraphProfiles['highlightFrames']> | null | undefined
-): frame is NonNullable<FlamegraphProfiles['highlightFrames']> {
-  return !!frame && typeof frame.name === 'string';
+): frame is FlamegraphProfiles['highlightFrames'] {
+  return !!frame && (typeof frame.name === 'string' || typeof frame.package === 'string');
 }
 
 interface FlamegraphStateProviderProps {
@@ -29,8 +29,11 @@ function getDefaultState(initialState?: DeepPartial<FlamegraphState>): Flamegrap
   return {
     profiles: {
       highlightFrames: isValidHighlightFrame(initialState?.profiles?.highlightFrames)
-        ? (initialState?.profiles
-            ?.highlightFrames as FlamegraphProfiles['highlightFrames'])
+        ? {
+            name: undefined,
+            package: undefined,
+            ...initialState?.profiles?.highlightFrames,
+          }
         : isValidHighlightFrame(DEFAULT_FLAMEGRAPH_STATE.profiles.highlightFrames)
         ? DEFAULT_FLAMEGRAPH_STATE.profiles.highlightFrames
         : null,

+ 29 - 3
static/app/utils/profiling/flamegraph/flamegraphStateProvider/flamegraphQueryParamSync.tsx

@@ -66,12 +66,25 @@ export function decodeFlamegraphStateFromQueryParams(
 ): DeepPartial<FlamegraphState> {
   const decoded: DeepPartial<FlamegraphState> = {};
 
-  if (typeof query.frameName === 'string' && typeof query.framePackage === 'string') {
+  // Similarly to how we encode frame name and values, we want to
+  // omit the field entirely if it is not present in the query string or
+  // if it is an empty string.
+  if (typeof query.frameName === 'string') {
     decoded.profiles = {
       ...(decoded.profiles ?? {}),
       highlightFrames: {
-        name: query.frameName,
-        package: query.framePackage,
+        ...(decoded.profiles?.highlightFrames ?? {}),
+        name: query.frameName ? query.frameName : undefined,
+      },
+    };
+  }
+
+  if (typeof query.framePackage === 'string') {
+    decoded.profiles = {
+      ...(decoded.profiles ?? {}),
+      highlightFrames: {
+        ...(decoded.profiles?.highlightFrames ?? {}),
+        package: query.framePackage ? query.framePackage : undefined,
       },
     };
   }
@@ -112,6 +125,19 @@ export function decodeFlamegraphStateFromQueryParams(
 }
 
 export function encodeFlamegraphStateToQueryParams(state: FlamegraphState) {
+  const highlightFrameToEncode: Record<string, string> = {};
+
+  // For some frames we do not have a package (or name) if that happens we want to omit
+  // the field entirely from the query string. This is to avoid default values being used
+  // as qs.parse will initialize empty values to "" which can differ from the respective
+  // frame values which are undefined.
+  if (state.profiles.highlightFrames?.name) {
+    highlightFrameToEncode.frameName = state.profiles.highlightFrames.name;
+  }
+  if (state.profiles.highlightFrames?.package) {
+    highlightFrameToEncode.framePackage = state.profiles.highlightFrames.package;
+  }
+
   const highlightFrame = state.profiles.highlightFrames
     ? {
         frameName: state.profiles.highlightFrames?.name,

+ 1 - 1
static/app/utils/profiling/flamegraph/flamegraphStateProvider/reducers/flamegraphProfiles.tsx

@@ -21,7 +21,7 @@ type SetHighlightAllFrames = {
 type FlamegraphProfilesAction = SetHighlightAllFrames | SetProfilesThreadId | SetRootNode;
 
 export type FlamegraphProfiles = {
-  highlightFrames: {name: string; package: string} | null;
+  highlightFrames: {name: string | undefined; package: string | undefined} | null;
   selectedRoot: FlamegraphFrame | null;
   threadId: number | null;
 };

+ 3 - 1
static/app/utils/profiling/frame.tsx

@@ -111,7 +111,9 @@ export class Frame extends WeightedNode {
             image += pathOrFile.charAt(i);
           }
 
-          this.image = image;
+          if (image) {
+            this.image = image;
+          }
         }
       }
     }