Skip to content

Commit 1653532

Browse files
authored
feat(flamegraph): Use average frame duration in call tree table view (#99553)
The sum frame duration is not a useful metric without an idea of how frequently it's called. This swaps it out for avg frame duration. Would be better to have the number of calls as well but that's out of scope of this PR. Closes PRO-25
1 parent 0bf686f commit 1653532

File tree

13 files changed

+111
-100
lines changed

13 files changed

+111
-100
lines changed

static/app/components/profiling/flamegraph/aggregateFlamegraphTreeTable.tsx

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,17 @@ function makeSortFunction(
7474
a: VirtualizedTreeNode<FlamegraphFrame>,
7575
b: VirtualizedTreeNode<FlamegraphFrame>
7676
) => {
77-
return b.node.node.aggregate_duration_ns - a.node.node.aggregate_duration_ns;
77+
const avgA = a.node.frame.averageCallDuration || 0;
78+
const avgB = b.node.frame.averageCallDuration || 0;
79+
return avgB - avgA;
7880
}
7981
: (
8082
a: VirtualizedTreeNode<FlamegraphFrame>,
8183
b: VirtualizedTreeNode<FlamegraphFrame>
8284
) => {
83-
return a.node.node.aggregate_duration_ns - b.node.node.aggregate_duration_ns;
85+
const avgA = a.node.frame.averageCallDuration || 0;
86+
const avgB = b.node.frame.averageCallDuration || 0;
87+
return avgA - avgB;
8488
};
8589
}
8690

@@ -249,6 +253,14 @@ export function AggregateFlamegraphTreeTable({
249253
abbreviation
250254
/>
251255
}
256+
avgWeight={
257+
defined(r.item.node.frame.averageCallDuration) ? (
258+
<PerformanceDuration
259+
nanoseconds={r.item.node.frame.averageCallDuration}
260+
abbreviation
261+
/>
262+
) : undefined
263+
}
252264
selfWeight={r.item.node.node.totalWeight.toFixed(0)}
253265
relativeSelfWeight={relativeWeight(
254266
referenceNode.node.totalWeight,
@@ -424,9 +436,9 @@ export function AggregateFlamegraphTreeTable({
424436
<CallTreeTableHeaderButton onClick={onSortByDuration}>
425437
<InteractionStateLayer />
426438
<span>
427-
{t('Duration')}{' '}
439+
{t('Average Duration')}{' '}
428440
<QuestionTooltip
429-
title={t('Aggregated duration of this frame across different samples.')}
441+
title={t('Average duration of this frame across different samples.')}
430442
size="sm"
431443
position="top"
432444
/>

static/app/components/profiling/flamegraph/callTreeTable.tsx

Lines changed: 37 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ import styled from '@emotion/styled';
33

44
import {IconSettings} from 'sentry/icons/iconSettings';
55
import {IconUser} from 'sentry/icons/iconUser';
6+
import {t} from 'sentry/locale';
67
import {space} from 'sentry/styles/space';
8+
import {defined} from 'sentry/utils';
79
import type {FlamegraphFrame} from 'sentry/utils/profiling/flamegraphFrame';
810
import type {VirtualizedTreeNode} from 'sentry/utils/profiling/hooks/useVirtualizedTree/VirtualizedTreeNode';
911
import type {VirtualizedTreeRenderedRow} from 'sentry/utils/profiling/hooks/useVirtualizedTree/virtualizedTreeUtils';
@@ -403,7 +405,7 @@ export function CallTreeTableRow({ref, ...props}: CallTreeTableRowProps) {
403405
);
404406
}
405407

406-
interface CallTreeTableColumns {
408+
interface BaseCallTreeTableColumns {
407409
formatDuration: (value: number) => string;
408410
frameColor: string;
409411
node: VirtualizedTreeNode<FlamegraphFrame>;
@@ -413,12 +415,17 @@ interface CallTreeTableColumns {
413415
opts?: {expandChildren: boolean}
414416
) => void;
415417
referenceNode: FlamegraphFrame;
418+
tabIndex: number;
419+
type: 'count' | 'time';
420+
}
421+
422+
interface CallTreeTableColumns extends BaseCallTreeTableColumns {
416423
relativeSelfWeight: number;
417424
relativeTotalWeight: number;
418425
selfWeight: number | React.ReactNode;
419-
tabIndex: number;
420426
totalWeight: number | React.ReactNode;
421-
type: 'count' | 'time';
427+
avgWeight?: React.ReactNode;
428+
showAvg?: boolean;
422429
}
423430

424431
export function CallTreeTableFixedColumns(props: CallTreeTableColumns) {
@@ -437,34 +444,39 @@ export function CallTreeTableFixedColumns(props: CallTreeTableColumns) {
437444
</div>
438445
</div>
439446
<div className={CallTreeTableClassNames.CELL} style={TEXT_ALIGN_RIGHT}>
440-
{typeof props.totalWeight === 'number'
441-
? props.formatDuration(props.totalWeight)
442-
: props.totalWeight}
443-
<div className={CallTreeTableClassNames.WEIGHT}>
444-
{props.relativeTotalWeight.toFixed(1)}%
445-
<div
446-
className={CallTreeTableClassNames.BACKGROUND_WEIGHT}
447-
style={{transform: `scaleX(${props.relativeTotalWeight / 100})`}}
448-
/>
449-
</div>
450-
<div className={CallTreeTableClassNames.FRAME_TYPE}>
451-
{props.node.node.node.frame.is_application ? (
452-
<IconUser size="xs" />
447+
{props.showAvg ? (
448+
defined(props.avgWeight) ? (
449+
props.avgWeight
453450
) : (
454-
<IconSettings size="xs" />
455-
)}
456-
</div>
451+
<span>{t('Unknown')}</span>
452+
)
453+
) : (
454+
<Fragment>
455+
{typeof props.totalWeight === 'number'
456+
? props.formatDuration(props.totalWeight)
457+
: props.totalWeight}
458+
<div className={CallTreeTableClassNames.WEIGHT}>
459+
{props.relativeTotalWeight.toFixed(1)}%
460+
<div
461+
className={CallTreeTableClassNames.BACKGROUND_WEIGHT}
462+
style={{transform: `scaleX(${props.relativeTotalWeight / 100})`}}
463+
/>
464+
</div>
465+
<div className={CallTreeTableClassNames.FRAME_TYPE}>
466+
{props.node.node.node.frame.is_application ? (
467+
<IconUser size="xs" />
468+
) : (
469+
<IconSettings size="xs" />
470+
)}
471+
</div>
472+
</Fragment>
473+
)}
457474
</div>
458475
</Fragment>
459476
);
460477
}
461478

462-
export function CallTreeTableDynamicColumns(
463-
props: Omit<
464-
CallTreeTableColumns,
465-
'relativeTotalWeight' | 'relativeSelfWeight' | 'selfWeight' | 'totalWeight'
466-
>
467-
) {
479+
export function CallTreeTableDynamicColumns(props: BaseCallTreeTableColumns) {
468480
const handleExpanding = (evt: React.MouseEvent) => {
469481
evt.stopPropagation();
470482
props.onExpandClick(props.node, !props.node.expanded, {

static/app/types/profiling.d.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ declare namespace Profiling {
192192
children?: Span[];
193193
};
194194

195-
type FrameInfo = {
195+
type Frame = {
196196
col?: number;
197197
colno?: number;
198198
column?: number;
@@ -219,8 +219,18 @@ declare namespace Profiling {
219219
// nodejs only
220220
columnNumber?: number;
221221
lineNumber?: number;
222+
resourceId?: number;
222223
scriptName?: string;
223224
scriptId?: number;
225+
226+
// metadata about the frame's occurrences and more
227+
count?: number;
228+
weight?: number;
229+
};
230+
231+
type FrameInfo = {
232+
count: number;
233+
weight: number;
224234
};
225235

226236
type FunctionMetric = {
@@ -313,7 +323,8 @@ declare namespace Profiling {
313323
>
314324
>;
315325
shared: {
316-
frames: ReadonlyArray<Omit<Profiling.FrameInfo, 'key'>>;
326+
frames: ReadonlyArray<Omit<Profiling.Frame, 'key'>>;
327+
frame_infos?: ReadonlyArray<Profiling.FrameInfo, 'key'>;
317328
profile_ids?: ReadonlyArray<string>[];
318329
profiles?: ReadonlyArray<ProfileReference>;
319330
};

static/app/utils/profiling/flamegraph.spec.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -271,12 +271,10 @@ describe('flamegraph', () => {
271271
);
272272

273273
expect(flamegraph.frames[1]!.frame.name).toBe('f0');
274-
expect(flamegraph.frames[1]!.frame.totalWeight).toBe(1);
275274
expect(flamegraph.frames[1]!.start).toBe(2);
276275
expect(flamegraph.frames[1]!.end).toBe(3);
277276

278277
expect(flamegraph.frames[0]!.frame.name).toBe('f1');
279-
expect(flamegraph.frames[0]!.frame.totalWeight).toBe(2);
280278
expect(flamegraph.frames[0]!.start).toBe(0);
281279
expect(flamegraph.frames[0]!.end).toBe(2);
282280
});

static/app/utils/profiling/flamegraph.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ export class Flamegraph {
145145
0
146146
);
147147
this.root.end = this.root.start + weight;
148-
this.root.frame.totalWeight += weight;
149148

150149
let width = 0;
151150

static/app/utils/profiling/frame.tsx

Lines changed: 36 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {trimPackage} from 'sentry/components/events/interfaces/frame/utils';
22
import type {SymbolicatorStatus} from 'sentry/components/events/interfaces/types';
33
import {t} from 'sentry/locale';
4+
import {defined} from 'sentry/utils';
45

56
const ROOT_KEY = 'sentry root';
67
const BROWSER_EXTENSION_REGEXP = /^(\@moz-extension\:\/\/|chrome-extension\:\/\/)/;
@@ -27,9 +28,9 @@ export class Frame {
2728

2829
readonly isRoot: boolean;
2930

30-
totalWeight = 0;
31-
selfWeight = 0;
32-
aggregateDuration = 0;
31+
readonly totalCallCount?: number;
32+
readonly totalCallDuration?: number;
33+
readonly averageCallDuration?: number;
3334

3435
static Root = new Frame({
3536
key: ROOT_KEY,
@@ -38,40 +39,49 @@ export class Frame {
3839
});
3940

4041
constructor(
41-
frameInfo: Profiling.FrameInfo,
42+
frame: Profiling.Frame,
4243
type?: 'mobile' | 'javascript' | 'node' | string,
4344
// In aggregate mode, we miss certain info like lineno/col and so
4445
// we need to make sure we don't try to use it or infer data based on it
4546
mode?: 'detailed' | 'aggregate'
4647
) {
47-
this.key = frameInfo.key;
48-
this.file = frameInfo.file;
49-
this.name = frameInfo.name;
50-
this.resource = frameInfo.resource;
51-
this.line = frameInfo.line;
52-
this.column = frameInfo.column;
53-
this.is_application = !!frameInfo.is_application;
54-
this.package = frameInfo.package;
55-
this.module = frameInfo.module ?? frameInfo.image;
56-
this.threadId = frameInfo.threadId;
57-
this.path = frameInfo.path;
58-
this.platform = frameInfo.platform;
59-
this.instructionAddr = frameInfo.instructionAddr;
60-
this.symbol = frameInfo.symbol;
61-
this.symbolAddr = frameInfo.symbolAddr;
62-
this.symbolicatorStatus = frameInfo.symbolicatorStatus;
48+
this.key = frame.key;
49+
this.file = frame.file;
50+
this.name = frame.name;
51+
this.resource = frame.resource;
52+
this.line = frame.line;
53+
this.column = frame.column;
54+
this.is_application = !!frame.is_application;
55+
this.package = frame.package;
56+
this.module = frame.module ?? frame.image;
57+
this.threadId = frame.threadId;
58+
this.path = frame.path;
59+
this.platform = frame.platform;
60+
this.instructionAddr = frame.instructionAddr;
61+
this.symbol = frame.symbol;
62+
this.symbolAddr = frame.symbolAddr;
63+
this.symbolicatorStatus = frame.symbolicatorStatus;
6364
this.isRoot = this.key === ROOT_KEY;
6465

66+
this.totalCallCount = frame.count;
67+
this.totalCallDuration = frame.weight;
68+
this.averageCallDuration =
69+
defined(frame.weight) && defined(frame.count)
70+
? frame.count
71+
? frame.weight / frame.count
72+
: 0
73+
: undefined;
74+
6575
// We are remapping some of the keys as they differ between platforms.
6676
// This is a temporary solution until we adopt a unified format.
67-
if (frameInfo.columnNumber && this.column === undefined) {
68-
this.column = frameInfo.columnNumber;
77+
if (frame.columnNumber && this.column === undefined) {
78+
this.column = frame.columnNumber;
6979
}
70-
if (frameInfo.lineNumber && this.column === undefined) {
71-
this.line = frameInfo.lineNumber;
80+
if (frame.lineNumber && this.column === undefined) {
81+
this.line = frame.lineNumber;
7282
}
73-
if (frameInfo.scriptName && this.column === undefined) {
74-
this.resource = frameInfo.scriptName;
83+
if (frame.scriptName && this.column === undefined) {
84+
this.resource = frame.scriptName;
7585
}
7686

7787
// 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

static/app/utils/profiling/profile/continuousProfile.tsx

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,7 @@ export class ContinuousProfile extends Profile {
138138
child.lock();
139139
}
140140

141-
node.frame.selfWeight += duration;
142-
143141
for (const stackNode of framesInStack) {
144-
stackNode.frame.totalWeight += duration;
145142
stackNode.count++;
146143
}
147144

static/app/utils/profiling/profile/eventedProfile.tsx

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -97,19 +97,6 @@ export class EventedProfile extends Profile {
9797
return built;
9898
}
9999

100-
addWeightToFrames(weight: number): void {
101-
const weightDelta = weight - this.lastValue;
102-
103-
for (const frame of this.stack) {
104-
frame.totalWeight += weightDelta;
105-
}
106-
107-
const top = this.stack[this.stack.length - 1];
108-
if (top) {
109-
top.selfWeight += weight;
110-
}
111-
}
112-
113100
addWeightsToNodes(value: number) {
114101
const delta = value - this.lastValue;
115102

@@ -124,7 +111,6 @@ export class EventedProfile extends Profile {
124111
}
125112

126113
enterFrame(frame: Frame, at: number): void {
127-
this.addWeightToFrames(at);
128114
this.addWeightsToNodes(at);
129115

130116
const lastTop = this.calltree[this.calltree.length - 1];
@@ -192,8 +178,8 @@ export class EventedProfile extends Profile {
192178
}
193179

194180
leaveFrame(_event: Frame, at: number): void {
195-
this.addWeightToFrames(at);
196181
this.addWeightsToNodes(at);
182+
197183
this.trackSampleStats(at);
198184

199185
const leavingStackTop = this.calltree.pop();

static/app/utils/profiling/profile/importProfile.tsx

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,14 @@ function importSchema(
231231
: input.metadata.platform === 'javascript'
232232
? 'javascript'
233233
: 'mobile',
234-
input.shared.frames
234+
input.shared.frames.map((frame, i) => {
235+
const frameInfo = input.shared.frame_infos?.[i];
236+
return {
237+
...frame,
238+
count: frameInfo?.count,
239+
weight: frameInfo?.weight,
240+
};
241+
})
235242
);
236243

237244
return {

static/app/utils/profiling/profile/jsSelfProfile.tsx

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,10 +157,7 @@ export class JSSelfProfile extends Profile {
157157
child.lock();
158158
}
159159

160-
node.frame.selfWeight += weight;
161-
162160
for (const stackNode of framesInStack) {
163-
stackNode.frame.totalWeight += weight;
164161
stackNode.count++;
165162
}
166163

0 commit comments

Comments
 (0)