Browse Source

feat(profiling): Support package/module over image on profiles (#46088)

`image` existed as an artifact of a legacy format. To be better
compatible with sentry errors, we're opting to use `package/module` as
needed.
Tony Xiao 2 years ago
parent
commit
5f0c6dfee5

+ 6 - 6
static/app/components/events/interfaces/spans/spanProfileDetails.tsx

@@ -295,15 +295,15 @@ function extractFrames(node: CallTreeNode | null, platform: PlatformType): Frame
       filename: node.frame.file ?? null,
       function: node.frame.name ?? null,
       inApp: node.frame.is_application,
-      instructionAddr: null,
+      instructionAddr: node.frame.instructionAddr ?? null,
       lineNo: node.frame.line ?? null,
-      // TODO: distinguish between module/package
-      module: node.frame.image ?? null,
-      package: null,
+      module: node.frame.module ?? null,
+      package: node.frame.package ?? null,
       platform,
       rawFunction: null,
-      symbol: null,
-      symbolAddr: null,
+      symbol: node.frame.symbol ?? null,
+      symbolAddr: node.frame.symbolAddr ?? null,
+      symbolicatorStatus: node.frame.symbolicatorStatus,
       trust: null,
       vars: null,
     };

+ 3 - 1
static/app/components/profiling/flamegraph/aggregateFlamegraph.tsx

@@ -64,7 +64,9 @@ function findLongestMatchingFrame(
     const frame = frames.pop()!;
     if (
       focusFrame.name === frame.frame.name &&
-      focusFrame.package === frame.frame.image &&
+      // the image name on a frame is optional treat it the same as the empty string
+      (focusFrame.package === (frame.frame.package || '') ||
+        focusFrame.package === (frame.frame.module || '')) &&
       (longestFrame?.node?.totalWeight || 0) < frame.node.totalWeight
     ) {
       longestFrame = frame;

+ 3 - 3
static/app/components/profiling/flamegraph/flamegraph.tsx

@@ -139,9 +139,9 @@ function findLongestMatchingFrame(
     const frame = frames.pop()!;
     if (
       focusFrame.name === frame.frame.name &&
-      // the image name on a frame is optional,
-      // treat it the same as the empty string
-      focusFrame.package === (frame.frame.image || '') &&
+      // the image name on a frame is optional treat it the same as the empty string
+      (focusFrame.package === (frame.frame.package || '') ||
+        focusFrame.package === (frame.frame.module || '')) &&
       (longestFrame?.node?.totalWeight || 0) < frame.node.totalWeight
     ) {
       longestFrame = frame;

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

@@ -138,7 +138,9 @@ export function FlamegraphTreeTable({
     canvasPoolManager.dispatch('highlight frame', [
       flamegraph.findAllMatchingFrames(
         clickedContextMenuNode.node.frame.name,
-        clickedContextMenuNode.node.frame.image
+        clickedContextMenuNode.node.frame.package ??
+          clickedContextMenuNode.node.frame.module ??
+          ''
       ),
       'selected',
     ]);

+ 10 - 6
static/app/components/profiling/flamegraph/flamegraphZoomView.tsx

@@ -605,18 +605,22 @@ function FlamegraphZoomView({
     }
 
     setHighlightingAllOccurences(true);
+
+    const frameName = hoveredNodeOnContextMenuOpen.current.frame.name;
+    const packageName =
+      hoveredNodeOnContextMenuOpen.current.frame.package ??
+      hoveredNodeOnContextMenuOpen.current.frame.module ??
+      '';
+
     dispatch({
       type: 'set highlight all frames',
       payload: {
-        name: hoveredNodeOnContextMenuOpen.current.frame.name,
-        package: hoveredNodeOnContextMenuOpen.current.frame.image ?? '',
+        name: frameName,
+        package: packageName,
       },
     });
 
-    const frames = flamegraph.findAllMatchingFrames(
-      hoveredNodeOnContextMenuOpen.current.frame.name,
-      hoveredNodeOnContextMenuOpen.current.frame.image
-    );
+    const frames = flamegraph.findAllMatchingFrames(frameName, packageName);
     const rectFrames = frames.map(f => new Rect(f.start, f.depth, f.end - f.start, 1));
     const newConfigView = computeMinZoomConfigViewForFrames(
       flamegraphView.configView,

+ 26 - 13
static/app/types/profiling.d.ts

@@ -1,5 +1,10 @@
 declare namespace Profiling {
   type Release = import('sentry/types').Release;
+
+  type Image = import('sentry/types/debugImage').Image;
+
+  type SymbolicatorStatus = import('sentry/components/events/interfaces/types').SymbolicatorStatus;
+
   type SentrySampledProfileSample = {
     stack_id: number;
     thread_id: string;
@@ -10,20 +15,18 @@ declare namespace Profiling {
   type SentrySampledProfileStack = number[];
 
   type SentrySampledProfileFrame = {
+    in_app: boolean;
+    colno?: number;
+    filename?: string;
     function?: string;
     instruction_addr?: string;
     lineno?: number;
-    colno?: number;
-    filename?: string;
-  };
-
-  type SentrySampledProfileDebugMetaImage = {
-    debug_id: string;
-    image_addr: string;
-    code_file: string;
-    type: string;
-    image_size: number;
-    image_vmaddr: string;
+    module?: string;
+    package?: string;
+    abs_path?: string;
+    status?: SymbolicatorStatus;
+    sym_addr?: string;
+    symbol?: string;
   };
 
   type SentrySampledProfileTransaction = {
@@ -57,7 +60,7 @@ declare namespace Profiling {
     platform: string;
     environment?: string;
     debug_meta?: {
-      images: SentryProfileDebugMetaImage[];
+      images: Image[];
     };
     profile: {
       samples: SentrySampledProfileSample[];
@@ -113,10 +116,20 @@ declare namespace Profiling {
     line?: number;
     column?: number;
     is_application?: boolean;
-    image?: string;
     resource?: string;
     threadId?: number;
     inline?: boolean;
+    instructionAddr?: string;
+    symbol?: string;
+    symbolAddr?: string;
+    symbolicatorStatus?: SymbolicatorStatus;
+
+    // This exists as an artifact of the legacy formats, the speedscope format still uses this
+    image?: string;
+    // This is used for native platforms to indicate the name of the assembly, path of the dylib, etc
+    package?: string;
+    // This is the import path for the module
+    module?: string;
 
     // nodejs only
     columnNumber?: number;

+ 27 - 24
static/app/utils/profiling/colors/utils.tsx

@@ -158,12 +158,16 @@ export function toRGBAString(r: number, g: number, b: number, alpha: number): st
   )}, ${alpha})`;
 }
 
-export function defaultFrameSortKey(frame: FlamegraphFrame): string {
-  return frame.frame.name + (frame.frame.image || '');
+function frameLibraryKey(frame: FlamegraphFrame): string {
+  return frame.frame.package ?? frame.frame.module ?? '';
+}
+
+export function defaultFrameKey(frame: FlamegraphFrame): string {
+  return `${frame.frame.name}:${frame.frame.package}:${frame.frame.module}`;
 }
 
 function defaultFrameSort(a: FlamegraphFrame, b: FlamegraphFrame): number {
-  return defaultFrameSortKey(a) > defaultFrameSortKey(b) ? 1 : -1;
+  return defaultFrameKey(a) > defaultFrameKey(b) ? 1 : -1;
 }
 
 export function makeColorBucketTheme(
@@ -189,12 +193,12 @@ export function makeColorMapBySymbolName(
   const colorCache: Map<string, ColorChannels> = new Map();
 
   const sortedFrames: FlamegraphFrame[] = [...frames].sort(defaultFrameSort);
-  const uniqueCount = uniqueCountBy(sortedFrames, t => t.frame.name + t.frame.image);
+  const uniqueCount = uniqueCountBy(sortedFrames, t => defaultFrameKey(t));
 
   for (let i = 0; i < sortedFrames.length; i++) {
     const frame = sortedFrames[i];
 
-    const key = frame.frame.name + frame.frame.image;
+    const key = defaultFrameKey(frame);
 
     if (!colorCache.has(key)) {
       const color = colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
@@ -222,7 +226,7 @@ export function makeColorMapByRecursion(
       continue;
     }
     const frame = sortedFrames[i]!;
-    const key = frame.frame.name + frame.frame.image;
+    const key = defaultFrameKey(frame);
 
     if (!colorCache.has(key)) {
       const color = colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
@@ -243,25 +247,24 @@ export function makeColorMapByLibrary(
   const colorCache: Map<string, ColorChannels> = new Map();
 
   const sortedFrames: FlamegraphFrame[] = [...frames].sort((a, b) => {
-    return (a.frame.image ?? '').localeCompare(b.frame.image ?? '');
+    return frameLibraryKey(a).localeCompare(frameLibraryKey(b));
   });
 
-  const uniqueCount = uniqueCountBy(sortedFrames, t =>
-    t.frame.image ? t.frame.image : false
-  );
+  const uniqueCount = uniqueCountBy(sortedFrames, t => frameLibraryKey(t));
 
   for (let i = 0; i < sortedFrames.length; i++) {
     const frame = sortedFrames[i];
 
-    if (!frame.frame.image) {
+    const key = frameLibraryKey(frame);
+
+    if (!key) {
       continue;
     }
 
     const color =
-      colorCache.get(frame.frame.image) ||
-      colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
+      colorCache.get(key) || colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
 
-    colorCache.set(frame.frame.image, color);
+    colorCache.set(key, color);
     colors.set(frame.key, color);
   }
 
@@ -276,16 +279,16 @@ export function makeColorMapBySystemFrame(
   const colorCache: Map<string, ColorChannels> = new Map();
 
   const sortedFrames: FlamegraphFrame[] = [...frames].sort((a, b) => {
-    return (a.frame.name + a.frame.image).localeCompare(b.frame.name + b.frame.image);
+    return defaultFrameKey(a).localeCompare(defaultFrameKey(b));
   });
 
-  const uniqueCount = uniqueCountBy(sortedFrames, t => t.frame.name + t.frame.image);
+  const uniqueCount = uniqueCountBy(sortedFrames, t => defaultFrameKey(t));
   for (let i = 0; i < sortedFrames.length; i++) {
     if (sortedFrames[i].frame.is_application) {
       continue;
     }
 
-    const key = sortedFrames[i].frame.name + sortedFrames[i].frame.image;
+    const key = defaultFrameKey(sortedFrames[i]);
     if (!colorCache.has(key)) {
       const color = colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
       colorCache.set(key, color);
@@ -306,11 +309,11 @@ export function makeColorMapBySystemVsApplicationFrame(
   const colorCache: Map<string, ColorChannels> = new Map();
 
   const sortedFrames: FlamegraphFrame[] = [...frames].sort((a, b) => {
-    return (a.frame.name + a.frame.image).localeCompare(b.frame.name + b.frame.image);
+    return defaultFrameKey(a).localeCompare(defaultFrameKey(b));
   });
 
   for (let i = 0; i < sortedFrames.length; i++) {
-    const key = sortedFrames[i].frame.name + sortedFrames[i].frame.image;
+    const key = defaultFrameKey(sortedFrames[i]);
 
     if (sortedFrames[i].frame.is_application) {
       colorCache.set(key, theme.COLORS.FRAME_APPLICATION_COLOR);
@@ -332,16 +335,16 @@ export function makeColorMapByApplicationFrame(
   const colorCache: Map<string, ColorChannels> = new Map();
 
   const sortedFrames: FlamegraphFrame[] = [...frames].sort((a, b) => {
-    return (a.frame.name + a.frame.image).localeCompare(b.frame.name + b.frame.image);
+    return defaultFrameKey(a).localeCompare(defaultFrameKey(b));
   });
 
-  const uniqueCount = uniqueCountBy(sortedFrames, t => t.frame.name + t.frame.image);
+  const uniqueCount = uniqueCountBy(sortedFrames, t => defaultFrameKey(t));
   for (let i = 0; i < sortedFrames.length; i++) {
     if (!sortedFrames[i].frame.is_application) {
       continue;
     }
 
-    const key = sortedFrames[i].frame.name + sortedFrames[i].frame.image;
+    const key = defaultFrameKey(sortedFrames[i]);
     if (!colorCache.has(key)) {
       const color = colorBucket(Math.floor((255 * i) / uniqueCount) / 256);
       colorCache.set(key, color);
@@ -364,7 +367,7 @@ export function makeColorMapByFrequency(
 
   for (let i = 0; i < frames.length; i++) {
     const frame = frames[i]!; // iterating over non empty array
-    const key = frame.frame.name + frame.frame.image;
+    const key = defaultFrameKey(frame);
 
     if (!countMap.has(key)) {
       countMap.set(key, 0);
@@ -378,7 +381,7 @@ export function makeColorMapByFrequency(
 
   for (let i = 0; i < frames.length; i++) {
     const frame = frames[i]!; // iterating over non empty array
-    const key = frame.frame.name + frame.frame.image;
+    const key = defaultFrameKey(frame);
     const count = countMap.get(key)!;
     const [r, g, b] = colorBucket(0.7);
     const color: ColorChannels = [r, g, b, Math.max(count / max, 0.1)];

+ 4 - 1
static/app/utils/profiling/flamegraph.ts

@@ -344,7 +344,10 @@ export class Flamegraph {
     for (let i = 0; i < this.frames.length; i++) {
       if (
         this.frames[i].frame.name === frameName &&
-        this.frames[i].frame.image === framePackage
+        // the framePackage can match either the package or the module
+        // this is an artifact of how we previously used image
+        (this.frames[i].frame.package === framePackage ||
+          this.frames[i].frame.module === framePackage)
       ) {
         matches.push(this.frames[i]);
       }

+ 2 - 2
static/app/utils/profiling/frame.spec.tsx

@@ -29,7 +29,7 @@ describe('Frame', () => {
             column: undefined,
           },
           'node'
-        ).image
+        ).module
       ).toBe(undefined);
     });
     it.each([
@@ -58,7 +58,7 @@ describe('Frame', () => {
             column: undefined,
           },
           'node'
-        ).image
+        ).module
       ).toBe(expected);
     });
   });

+ 17 - 6
static/app/utils/profiling/frame.tsx

@@ -1,3 +1,4 @@
+import type {SymbolicatorStatus} from 'sentry/components/events/interfaces/types';
 import {t} from 'sentry/locale';
 
 import {WeightedNode} from './weightedNode';
@@ -10,10 +11,15 @@ export class Frame extends WeightedNode {
   readonly column?: number;
   readonly is_application: boolean;
   readonly path?: string;
-  readonly image?: string;
+  readonly package?: string;
+  readonly module?: string;
   readonly resource?: string;
   readonly threadId?: number;
   readonly inline?: boolean;
+  readonly instructionAddr?: string;
+  readonly symbol?: string;
+  readonly symbolAddr?: string;
+  readonly symbolicatorStatus?: SymbolicatorStatus;
 
   static Root = new Frame(
     {
@@ -34,9 +40,14 @@ export class Frame extends WeightedNode {
     this.line = frameInfo.line;
     this.column = frameInfo.column;
     this.is_application = !!frameInfo.is_application;
-    this.image = frameInfo.image;
+    this.package = frameInfo.package;
+    this.module = frameInfo.module ?? frameInfo.image;
     this.threadId = frameInfo.threadId;
     this.path = frameInfo.path;
+    this.instructionAddr = frameInfo.instructionAddr;
+    this.symbol = frameInfo.symbol;
+    this.symbolAddr = frameInfo.symbolAddr;
+    this.symbolicatorStatus = frameInfo.symbolicatorStatus;
 
     // We are remapping some of the keys as they differ between platforms.
     // This is a temporary solution until we adopt a unified format.
@@ -75,7 +86,7 @@ export class Frame extends WeightedNode {
           this.is_application = false;
         }
 
-        if (this.image === undefined && pathOrFile) {
+        if (this.module === undefined && pathOrFile) {
           const match =
             /node_modules(\/|\\)(?<maybeScopeOrPackage>.*?)(\/|\\)((?<maybePackage>.*)((\/|\\)))?/.exec(
               pathOrFile
@@ -84,9 +95,9 @@ export class Frame extends WeightedNode {
             const {maybeScopeOrPackage, maybePackage} = match.groups;
 
             if (maybeScopeOrPackage.startsWith('@')) {
-              this.image = `${maybeScopeOrPackage}/${maybePackage}`;
+              this.module = `${maybeScopeOrPackage}/${maybePackage}`;
             } else {
-              this.image = match.groups.maybeScopeOrPackage;
+              this.module = match.groups.maybeScopeOrPackage;
             }
             this.is_application = false;
           }
@@ -112,7 +123,7 @@ export class Frame extends WeightedNode {
           }
 
           if (image) {
-            this.image = image;
+            this.module = image;
           }
         }
       }

Some files were not shown because too many files changed in this diff