Browse Source

fix(profiling): Sampled format to profile conversion (#46011)

We're looking to move away from the speedscope format and use the
sampled format directly. This means we need to support importing it
correctly. This change fixes up the import procedure to be up to date
with the following changes:
- deprecating `transactions` in favour of `transaction`
- correctly setting the active profile index
- correctly assigning weights in flamegraph mode
- updating the thread name to use the same convention
Tony Xiao 2 years ago
parent
commit
c7548b0ff4

+ 1 - 1
static/app/components/profiling/profileHeader.tsx

@@ -22,7 +22,7 @@ function getTransactionName(input: Profiling.ProfileInput): string {
     return input.metadata.transactionName;
   }
   if (isSentrySampledProfile(input)) {
-    return input.transactions?.[0]?.name || t('Unknown Transaction');
+    return input.transaction.name || t('Unknown Transaction');
   }
 
   return t('Unknown Transaction');

+ 3 - 5
static/app/types/profiling.d.ts

@@ -3,7 +3,7 @@ declare namespace Profiling {
   type SentrySampledProfileSample = {
     stack_id: number;
     thread_id: string;
-    elapsed_since_start_ns: string;
+    elapsed_since_start_ns: number;
     queue_address?: string;
   };
 
@@ -30,9 +30,7 @@ declare namespace Profiling {
     name: string;
     trace_id: string;
     id: string;
-    active_thread_id: string;
-    relative_start_ns: string;
-    relative_end_ns: string;
+    active_thread_id: number;
   };
 
   type SentrySampledProfile = {
@@ -68,7 +66,7 @@ declare namespace Profiling {
       thread_metadata?: Record<string, {name?: string; priority?: number}>;
       queue_metadata?: Record<string, {label: string}>;
     };
-    transactions?: SentrySampledProfileTransaction[];
+    transaction: SentrySampledProfileTransaction;
   };
 
   ////////////////

+ 19 - 14
static/app/utils/profiling/profile/importProfile.tsx

@@ -153,11 +153,12 @@ function importSentrySampledProfile(
 
   for (const key in samplesByThread) {
     samplesByThread[key].sort(
-      (a, b) =>
-        parseInt(a.elapsed_since_start_ns, 10) - parseInt(b.elapsed_since_start_ns, 10)
+      (a, b) => a.elapsed_since_start_ns - b.elapsed_since_start_ns
     );
   }
 
+  let activeProfileIndex = 0;
+
   const profiles: Profile[] = [];
 
   for (const key in samplesByThread) {
@@ -169,6 +170,10 @@ function importSentrySampledProfile(
       },
     };
 
+    if (key === String(input.transaction.active_thread_id)) {
+      activeProfileIndex = profiles.length;
+    }
+
     profiles.push(
       wrapWithSpan(
         options.transaction,
@@ -181,12 +186,15 @@ function importSentrySampledProfile(
     );
   }
 
-  const firstTransaction = input.transactions?.[0];
+  const firstSample = input.profile.samples[0];
+  const lastSample = input.profile.samples[input.profile.samples.length - 1];
+  const duration = lastSample.elapsed_since_start_ns - firstSample.elapsed_since_start_ns;
+
   return {
-    transactionID: firstTransaction?.id ?? null,
-    traceID: firstTransaction?.trace_id ?? '',
-    name: firstTransaction?.name ?? '',
-    activeProfileIndex: 0,
+    transactionID: input.transaction.id,
+    traceID: input.transaction.trace_id,
+    name: input.transaction.name,
+    activeProfileIndex,
     measurements: {},
     metadata: {
       deviceLocale: input.device.locale,
@@ -194,18 +202,15 @@ function importSentrySampledProfile(
       deviceModel: input.device.model,
       deviceOSName: input.os.name,
       deviceOSVersion: input.os.version,
-      durationNS: parseInt(
-        input.profile.samples[input.profile.samples.length - 1].elapsed_since_start_ns,
-        10
-      ),
+      durationNS: duration,
       environment: input.environment,
       platform: input.platform,
       profileID: input.event_id,
 
       // these don't really work for multiple transactions
-      transactionID: firstTransaction?.id,
-      transactionName: firstTransaction?.name,
-      traceID: firstTransaction?.trace_id,
+      transactionID: input.transaction.id,
+      transactionName: input.transaction.name,
+      traceID: input.transaction.trace_id,
     },
     profiles,
   };

+ 67 - 105
static/app/utils/profiling/profile/sentrySampledProfile.spec.tsx

@@ -33,31 +33,37 @@ export const makeSentrySampledProfile = (
           {
             stack_id: 0,
             thread_id: '0',
-            elapsed_since_start_ns: '0',
+            elapsed_since_start_ns: 0,
           },
           {
             stack_id: 1,
             thread_id: '0',
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
           },
         ],
         frames: [
           {
-            function: 'foo',
+            function: 'main',
             instruction_addr: '',
-            lineno: 2,
-            colno: 2,
+            lineno: 1,
+            colno: 1,
             file: 'main.c',
           },
           {
-            function: 'main',
+            function: 'foo',
             instruction_addr: '',
-            lineno: 1,
-            colno: 1,
+            lineno: 2,
+            colno: 2,
             file: 'main.c',
           },
         ],
-        stacks: [[0], [0, 1]],
+        stacks: [[1, 0], [0]],
+      },
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 0,
+        trace_id: '1',
       },
     },
     profile
@@ -90,26 +96,22 @@ describe('SentrySampledProfile', () => {
 
   it('tracks discarded samples', () => {
     const sampledProfile = makeSentrySampledProfile({
-      transactions: [
-        {
-          id: '',
-          name: 'foo',
-          active_thread_id: '1',
-          relative_start_ns: '0',
-          relative_end_ns: '1000000',
-          trace_id: '1',
-        },
-      ],
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 1,
+        trace_id: '1',
+      },
       profile: {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
         ],
@@ -127,31 +129,27 @@ describe('SentrySampledProfile', () => {
       {type: 'flamechart'}
     );
 
-    expect(profile.stats.discardedSamplesCount).toBe(1);
+    expect(profile.stats.discardedSamplesCount).toBe(2);
   });
 
   it('tracks negative samples', () => {
     const sampledProfile = makeSentrySampledProfile({
-      transactions: [
-        {
-          id: '',
-          name: 'foo',
-          active_thread_id: '1',
-          relative_start_ns: '0',
-          relative_end_ns: '1000000',
-          trace_id: '1',
-        },
-      ],
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 1,
+        trace_id: '1',
+      },
       profile: {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
           {
             stack_id: 0,
-            elapsed_since_start_ns: '-1000',
+            elapsed_since_start_ns: -1000,
             thread_id: '0',
           },
         ],
@@ -174,31 +172,27 @@ describe('SentrySampledProfile', () => {
 
   it('tracks raw weights', () => {
     const sampledProfile = makeSentrySampledProfile({
-      transactions: [
-        {
-          id: '',
-          name: 'foo',
-          active_thread_id: '1',
-          relative_start_ns: '0',
-          relative_end_ns: '1000000',
-          trace_id: '1',
-        },
-      ],
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 1,
+        trace_id: '1',
+      },
       profile: {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
           {
             stack_id: 0,
-            elapsed_since_start_ns: '2000',
+            elapsed_since_start_ns: 2000,
             thread_id: '0',
           },
           {
             stack_id: 0,
-            elapsed_since_start_ns: '3000',
+            elapsed_since_start_ns: 3000,
             thread_id: '0',
           },
         ],
@@ -216,26 +210,22 @@ describe('SentrySampledProfile', () => {
       {type: 'flamechart'}
     );
 
-    expect(profile.rawWeights.length).toBe(3);
+    expect(profile.rawWeights.length).toBe(2);
   });
 
   it('derives a profile name from the transaction.name and thread_id', () => {
     const sampledProfile = makeSentrySampledProfile({
-      transactions: [
-        {
-          id: '',
-          name: 'foo',
-          active_thread_id: '1',
-          relative_start_ns: '0',
-          relative_end_ns: '1000000',
-          trace_id: '1',
-        },
-      ],
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 1,
+        trace_id: '1',
+      },
       profile: {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
         ],
@@ -253,7 +243,8 @@ describe('SentrySampledProfile', () => {
       {type: 'flamechart'}
     );
 
-    expect(profile.name).toBe('foo (thread: bar)');
+    expect(profile.name).toBe('bar');
+    expect(profile.threadId).toBe(0);
   });
 
   it('derives a profile name from just thread_id', () => {
@@ -262,7 +253,7 @@ describe('SentrySampledProfile', () => {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
         ],
@@ -276,7 +267,8 @@ describe('SentrySampledProfile', () => {
       {type: 'flamechart'}
     );
 
-    expect(profile.name).toBe('thread: 0');
+    expect(profile.name).toBe('');
+    expect(profile.threadId).toBe(0);
   });
 
   it('derives a profile name from just thread name', () => {
@@ -285,7 +277,7 @@ describe('SentrySampledProfile', () => {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
         ],
@@ -303,63 +295,33 @@ describe('SentrySampledProfile', () => {
       {type: 'flamechart'}
     );
 
-    expect(profile.name).toBe('thread: foo');
-  });
-
-  it('throws a TypeError when it cannot parse startedAt or endedAt', () => {
-    const sampledProfile = makeSentrySampledProfile({
-      profile: {
-        samples: [
-          {
-            stack_id: 0,
-            elapsed_since_start_ns: 'a1000',
-            thread_id: '0',
-          },
-        ],
-        thread_metadata: {
-          '0': {
-            name: 'foo',
-          },
-        },
-      },
-    });
-
-    expect(() =>
-      SentrySampledProfile.FromProfile(
-        sampledProfile,
-        createSentrySampleProfileFrameIndex(sampledProfile.profile.frames),
-        {type: 'flamechart'}
-      )
-    ).toThrow(new TypeError('startedAt or endedAt is NaN'));
+    expect(profile.name).toBe('foo');
+    expect(profile.threadId).toBe(0);
   });
 
   it('flamegraph tracks node occurences', () => {
     const sampledProfile = makeSentrySampledProfile({
-      transactions: [
-        {
-          id: '',
-          name: 'foo',
-          active_thread_id: '1',
-          relative_start_ns: '0',
-          relative_end_ns: '1000000',
-          trace_id: '1',
-        },
-      ],
+      transaction: {
+        id: '',
+        name: 'foo',
+        active_thread_id: 1,
+        trace_id: '1',
+      },
       profile: {
         samples: [
           {
             stack_id: 0,
-            elapsed_since_start_ns: '1000',
+            elapsed_since_start_ns: 1000,
             thread_id: '0',
           },
           {
             stack_id: 1,
-            elapsed_since_start_ns: '2000',
+            elapsed_since_start_ns: 2000,
             thread_id: '0',
           },
           {
             stack_id: 0,
-            elapsed_since_start_ns: '3000',
+            elapsed_since_start_ns: 3000,
             thread_id: '0',
           },
         ],
@@ -380,7 +342,7 @@ describe('SentrySampledProfile', () => {
       {type: 'flamegraph'}
     );
 
-    expect(profile.callTree.children[0].count).toBe(3);
+    expect(profile.callTree.children[0].count).toBe(2);
     expect(profile.callTree.children[0].children[0].count).toBe(1);
   });
 });

+ 24 - 20
static/app/utils/profiling/profile/sentrySampledProfile.tsx

@@ -5,9 +5,11 @@ import {Frame} from './../frame';
 import {Profile} from './profile';
 import {createSentrySampleProfileFrameIndex} from './utils';
 
-function sortSentrySampledProfileSamples(
-  samples: Readonly<Profiling.SentrySampledProfile['profile']['samples']>
-) {
+type WeightedSample = Profiling.SentrySampledProfile['profile']['samples'][0] & {
+  weight: number;
+};
+
+function sortSentrySampledProfileSamples(samples: Readonly<WeightedSample[]>) {
   return [...samples].sort((a, b) => {
     return a.stack_id - b.stack_id;
   });
@@ -23,40 +25,44 @@ export class SentrySampledProfile extends Profile {
     frameIndex: ReturnType<typeof createSentrySampleProfileFrameIndex>,
     options: {type: 'flamechart' | 'flamegraph'}
   ): Profile {
+    const weightedSamples: WeightedSample[] = sampledProfile.profile.samples.map(
+      (sample, i) => {
+        // falling back to the current sample timestamp has the effect
+        // of giving the last sample a weight of 0
+        const nextSample = sampledProfile.profile.samples[i + 1] ?? sample;
+        return {
+          ...sample,
+          weight: nextSample.elapsed_since_start_ns - sample.elapsed_since_start_ns,
+        };
+      }
+    );
+
     const {stacks, thread_metadata = {}} = sampledProfile.profile;
     const samples =
       options.type === 'flamegraph'
-        ? sortSentrySampledProfileSamples(sampledProfile.profile.samples)
-        : sampledProfile.profile.samples;
+        ? sortSentrySampledProfileSamples(weightedSamples)
+        : weightedSamples;
 
-    const startedAt = parseInt(samples[0].elapsed_since_start_ns, 10);
-    const endedAt = parseInt(samples[samples.length - 1].elapsed_since_start_ns, 10);
+    const startedAt = samples[0].elapsed_since_start_ns;
+    const endedAt = samples[samples.length - 1].elapsed_since_start_ns;
     if (Number.isNaN(startedAt) || Number.isNaN(endedAt)) {
       throw TypeError('startedAt or endedAt is NaN');
     }
+
     const threadId = parseInt(samples[0].thread_id, 10);
-    const threadName = `thread: ${
-      thread_metadata[samples[0].thread_id]?.name || threadId
-    }`;
-    const profileTransactionName = sampledProfile.transactions?.[0]?.name;
     const profile = new SentrySampledProfile({
       duration: endedAt - startedAt,
       startedAt,
       endedAt,
       unit: 'nanoseconds',
-      name: profileTransactionName
-        ? `${profileTransactionName} (${threadName})`
-        : threadName,
+      name: thread_metadata[samples[0].thread_id]?.name || '',
       threadId,
       type: options.type,
     });
 
-    let previousSampleWeight = 0;
-
     for (let i = 0; i < samples.length; i++) {
       const sample = samples[i];
       const stack = stacks[sample.stack_id];
-      const sampleWeight = parseInt(sample.elapsed_since_start_ns, 10);
 
       profile.appendSampleWithWeight(
         stack.map(n => {
@@ -66,10 +72,8 @@ export class SentrySampledProfile extends Profile {
 
           return frameIndex[n];
         }),
-        sampleWeight - previousSampleWeight
+        sample.weight
       );
-
-      previousSampleWeight = sampleWeight;
     }
 
     return profile.build();

+ 4 - 1
static/app/views/profiling/profilesProvider.tsx

@@ -6,7 +6,7 @@ import {ProfileHeader} from 'sentry/components/profiling/profileHeader';
 import {t} from 'sentry/locale';
 import type {EventTransaction, Organization, Project} from 'sentry/types';
 import {RequestState} from 'sentry/types/core';
-import {isSchema} from 'sentry/utils/profiling/guards/profile';
+import {isSchema, isSentrySampledProfile} from 'sentry/utils/profiling/guards/profile';
 import {useSentryEvent} from 'sentry/utils/profiling/hooks/useSentryEvent';
 import useApi from 'sentry/utils/useApi';
 import useOrganization from 'sentry/utils/useOrganization';
@@ -33,6 +33,9 @@ function getTransactionId(input: Profiling.ProfileInput): string | null {
   if (isSchema(input)) {
     return input.metadata.transactionID;
   }
+  if (isSentrySampledProfile(input)) {
+    return input.transaction.id;
+  }
   return null;
 }