Browse Source

fix(profiling): correctly resolve frame (#55275)

Correct "unknown" to <anonymous> for node/javascript profiles

Before
<img width="761" alt="CleanShot 2023-08-28 at 13 46 48@2x"
src="https://github.com/getsentry/sentry/assets/9317857/8fc0208e-c377-4870-bf5e-4ec6e5a27c23">

After
<img width="691" alt="CleanShot 2023-08-28 at 13 56 16@2x"
src="https://github.com/getsentry/sentry/assets/9317857/195e89f7-c7b8-4805-9efb-cbd4c53d2056">
Jonas 1 year ago
parent
commit
cd660ec075

+ 1 - 1
static/app/types/profiling.d.ts

@@ -80,7 +80,7 @@ declare namespace Profiling {
     received: string;
     timestamp: string;
     release: Release | null;
-    platform: string;
+    platform: 'node' | 'javascript' | string;
     environment?: string;
     debug_meta?: {
       images: Image[];

+ 18 - 15
static/app/utils/profiling/frame.spec.tsx

@@ -1,22 +1,25 @@
 import {Frame} from 'sentry/utils/profiling/frame';
 
 describe('Frame', () => {
-  describe('web', () => {
-    it('sets anonymouse name if frame has no name', () => {
-      expect(new Frame({key: 0, name: '', line: 0, column: 0}, 'web').name).toBe(
-        '<anonymous>'
-      );
-    });
+  describe.each([['javascript'], ['node']])(
+    'renames unknown frame to <anonymous> for platform %s',
+    platform => {
+      it('sets anonymouse name if frame has no name', () => {
+        expect(new Frame({key: 0, name: '', line: 0, column: 0}, platform).name).toBe(
+          '<anonymous>'
+        );
+      });
 
-    it('appends [native code] to name if frame belongs to native code', () => {
-      expect(
-        new Frame(
-          {key: 0, name: 'foo', line: undefined, column: undefined},
-          'web'
-        ).name.endsWith('[native code]')
-      ).toBe(true);
-    });
-  });
+      it('appends [native code] to name if frame belongs to native code', () => {
+        expect(
+          new Frame(
+            {key: 0, name: 'foo', line: undefined, column: undefined},
+            platform
+          ).name.endsWith('[native code]')
+        ).toBe(true);
+      });
+    }
+  );
   describe('pulls package from path for web|node platforms', () => {
     it('file in node modules', () => {
       expect(

+ 12 - 12
static/app/utils/profiling/frame.tsx

@@ -25,16 +25,16 @@ export class Frame {
   totalWeight: number = 0;
   selfWeight: number = 0;
 
-  static Root = new Frame(
-    {
-      key: ROOT_KEY,
-      name: ROOT_KEY,
-      is_application: false,
-    },
-    'mobile'
-  );
-
-  constructor(frameInfo: Profiling.FrameInfo, type?: 'mobile' | 'web' | 'node') {
+  static Root = new Frame({
+    key: ROOT_KEY,
+    name: ROOT_KEY,
+    is_application: false,
+  });
+
+  constructor(
+    frameInfo: Profiling.FrameInfo,
+    type?: 'mobile' | 'javascript' | 'node' | string
+  ) {
     this.key = frameInfo.key;
     this.file = frameInfo.file;
     this.name = frameInfo.name;
@@ -66,12 +66,12 @@ export class Frame {
 
     // If the frame is a web frame and there is no name associated to it, then it was likely invoked as an iife or anonymous callback as
     // most modern browser engines properly show anonymous functions when they are assigned to references (e.g. `let foo = function() {};`)
-    if (type === 'web' || type === 'node') {
+    if (type === 'javascript' || type === 'node') {
       if (this.name === '(garbage collector)' || this.name === '(root)') {
         this.is_application = false;
       }
 
-      if (!this.name || this.name.startsWith('unknown ')) {
+      if (!this.name || this.name === 'unknown') {
         this.name = t('<anonymous>');
       }
       // If the frame had no line or column, it was part of the native code, (e.g. calling String.fromCharCode)

+ 7 - 3
static/app/utils/profiling/jsSelfProfiling.spec.tsx

@@ -40,7 +40,11 @@ describe('jsSelfProfiling', () => {
       };
       expect(
         toStackNames(
-          resolveJSSelfProfilingStack(trace, 5, createFrameIndex('web', trace.frames))
+          resolveJSSelfProfilingStack(
+            trace,
+            5,
+            createFrameIndex('javascript', trace.frames)
+          )
         )
       ).toEqual(['baz', 'foobar', 'foobarbaz', 'foobarbazfoo']);
     });
@@ -75,7 +79,7 @@ describe('jsSelfProfiling', () => {
           resolveJSSelfProfilingStack(
             trace,
             5,
-            createFrameIndex('web', trace.frames),
+            createFrameIndex('javascript', trace.frames),
             'gc'
           )
         )
@@ -95,7 +99,7 @@ describe('jsSelfProfiling', () => {
           resolveJSSelfProfilingStack(
             trace,
             0,
-            createFrameIndex('web', trace.frames),
+            createFrameIndex('javascript', trace.frames),
             'paint'
           )
         )

+ 20 - 8
static/app/utils/profiling/profile/importProfile.tsx

@@ -95,7 +95,7 @@ function importJSSelfProfile(
   traceID: string,
   options: ImportOptions
 ): ProfileGroup {
-  const frameIndex = createFrameIndex('web', input.frames);
+  const frameIndex = createFrameIndex('javascript', input.frames);
   const profile = importSingleProfile(input, frameIndex, options);
 
   return {
@@ -203,7 +203,11 @@ export function importSchema(
   options: ImportOptions
 ): ProfileGroup {
   const frameIndex = createFrameIndex(
-    input.metadata.platform === 'node' ? 'node' : 'mobile',
+    input.metadata.platform === 'node'
+      ? 'node'
+      : input.metadata.platform === 'javascript'
+      ? 'javascript'
+      : 'mobile',
     input.shared.frames
   );
 
@@ -266,17 +270,25 @@ function importSingleProfile(
   if (isJSProfile(profile)) {
     // In some cases, the SDK may return transaction as undefined and we dont want to throw there.
     if (!transaction) {
-      return JSSelfProfile.FromProfile(profile, createFrameIndex('web', profile.frames), {
-        type,
-      });
+      return JSSelfProfile.FromProfile(
+        profile,
+        createFrameIndex('javascript', profile.frames),
+        {
+          type,
+        }
+      );
     }
 
     return wrapWithSpan(
       transaction,
       () =>
-        JSSelfProfile.FromProfile(profile, createFrameIndex('web', profile.frames), {
-          type,
-        }),
+        JSSelfProfile.FromProfile(
+          profile,
+          createFrameIndex('javascript', profile.frames),
+          {
+            type,
+          }
+        ),
       {
         op: 'profile.import',
         description: 'js-self-profile',

+ 11 - 11
static/app/utils/profiling/profile/jsSelfProfile.spec.tsx

@@ -27,7 +27,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -57,7 +57,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', [{name: 'f0'}]),
+      createFrameIndex('javascript', [{name: 'f0'}]),
       {type: 'flamechart'}
     );
     expect(profile.stats.discardedSamplesCount).toBe(1);
@@ -86,7 +86,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', [{name: 'f0'}]),
+      createFrameIndex('javascript', [{name: 'f0'}]),
       {type: 'flamechart'}
     );
     expect(profile.stats.negativeSamplesCount).toBe(1);
@@ -119,7 +119,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', [{name: 'f0'}]),
+      createFrameIndex('javascript', [{name: 'f0'}]),
       {type: 'flamechart'}
     );
     // For JsSelfProfile, first sample is appended with 0 weight because it
@@ -156,7 +156,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -213,7 +213,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -256,7 +256,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -289,7 +289,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -330,7 +330,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -363,7 +363,7 @@ describe('jsSelfProfile', () => {
 
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 
@@ -412,7 +412,7 @@ describe('jsSelfProfile', () => {
     };
     const profile = JSSelfProfile.FromProfile(
       trace,
-      createFrameIndex('web', trace.frames, trace),
+      createFrameIndex('javascript', trace.frames, trace),
       {type: 'flamechart'}
     );
 

+ 1 - 1
static/app/utils/profiling/profile/sentrySampledProfile.tsx

@@ -120,7 +120,7 @@ export class SentrySampledProfile extends Profile {
       }
     );
 
-    function resolveFrame(index) {
+    function resolveFrame(index): Frame {
       const resolvedFrame = frameIndex[index];
       if (!resolvedFrame) {
         throw new Error(`Could not resolve frame ${index} in frame index`);

+ 4 - 4
static/app/utils/profiling/profile/utils.tsx

@@ -54,16 +54,16 @@ export function createSentrySampleProfileFrameIndex(
 }
 
 export function createFrameIndex(
-  type: 'mobile' | 'node' | 'web',
+  type: 'mobile' | 'node' | 'javascript',
   frames: Readonly<Profiling.Schema['shared']['frames']>
 ): FrameIndex;
 export function createFrameIndex(
-  type: 'mobile' | 'node' | 'web',
+  type: 'mobile' | 'node' | 'javascript',
   frames: Readonly<JSSelfProfiling.Frame[]>,
   trace: Readonly<JSSelfProfiling.Trace>
 ): FrameIndex;
 export function createFrameIndex(
-  type: 'mobile' | 'node' | 'web',
+  type: 'mobile' | 'node' | 'javascript',
   frames: Readonly<Profiling.Schema['shared']['frames'] | JSSelfProfiling.Frame[]>,
   trace?: Readonly<JSSelfProfiling.Trace>
 ): FrameIndex {
@@ -78,7 +78,7 @@ export function createFrameIndex(
               : undefined,
           ...frame,
         },
-        'web'
+        'javascript'
       );
       return acc;
     }, {});