Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(anr): Improve ANR root cause analysis #49673

Merged
merged 5 commits into from
Jun 2, 2023
Merged

feat(anr): Improve ANR root cause analysis #49673

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3057e78
feat(anr): Improve ANR root cause analysis
romtsn May 24, 2023
1fe04d8
Address PR review
romtsn Jun 1, 2023
22b9fa8
style(lint): Auto commit lint changes
getsantry[bot] Jun 1, 2023
4aa8dad
Only detect suspect frames for ANRs
romtsn Jun 2, 2023
f36861a
style(lint): Auto commit lint changes
getsantry[bot] Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 54 additions & 2 deletions static/app/components/events/interfaces/analyzeFrames.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ describe('analyzeAnrFrames', function () {
]);
const rootCause = analyzeFramesForRootCause(event);
expect(rootCause?.resources).toEqual(
'Switch to SharedPreferences.commit and move commit to a background thread.'
'SharedPreferences.apply will save data on background thread only if it happens before the activity/service finishes. Switch to SharedPreferences.commit and move commit to a background thread.'
);
expect(rootCause?.culprit).toEqual(
'/^android\\.app\\.SharedPreferencesImpl\\$EditorImpl\\$[0-9]/'
Expand Down Expand Up @@ -170,7 +170,59 @@ describe('analyzeAnrFrames', function () {
},
]);
const rootCause = analyzeFramesForRootCause(event);
expect(rootCause?.resources).toEqual('Move database operations off the main thread.');
expect(rootCause?.resources).toEqual(
'Database operations, such as querying, inserting, updating, or deleting data, can involve disk I/O, processing, and potentially long-running operations. Move database operations off the main thread to avoid this ANR.'
);
expect(rootCause?.culprit).toEqual('android.database.sqlite.SQLiteConnection');
});

it('picks anr root cause of the topmost frame', function () {
const event = makeEventWithFrames([
{
filename: 'Instrumentation.java',
absPath: 'Instrumentation.java',
module: 'android.app.Instrumentation',
package: null,
platform: null,
instructionAddr: null,
symbolAddr: null,
function: 'callApplicationOnCreate',
rawFunction: null,
symbol: null,
context: [],
lineNo: 1176,
colNo: null,
inApp: false,
trust: null,
errors: null,
vars: null,
},
{
filename: 'SharedPreferencesImpl.java',
absPath: 'SharedPreferencesImpl.java',
module: 'android.app.SharedPreferencesImpl$EditorImpl$1',
package: null,
platform: null,
instructionAddr: null,
symbolAddr: null,
function: 'run',
rawFunction: null,
symbol: null,
context: [],
lineNo: 366,
colNo: null,
inApp: false,
trust: null,
errors: null,
vars: null,
},
]);
const rootCause = analyzeFramesForRootCause(event);
expect(rootCause?.resources).toEqual(
'SharedPreferences.apply will save data on background thread only if it happens before the activity/service finishes. Switch to SharedPreferences.commit and move commit to a background thread.'
);
expect(rootCause?.culprit).toEqual(
'/^android\\.app\\.SharedPreferencesImpl\\$EditorImpl\\$[0-9]/'
);
});
});
115 changes: 81 additions & 34 deletions static/app/components/events/interfaces/analyzeFrames.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import {
getMappedThreadState,
ThreadStates,
} from 'sentry/components/events/interfaces/threads/threadSelector/threadStates';
import {getCurrentThread} from 'sentry/components/events/interfaces/utils';
import ExternalLink from 'sentry/components/links/externalLink';
import {t, tct} from 'sentry/locale';
import {EntryException, EntryThreads, EntryType, Event, Frame} from 'sentry/types';
import {EntryException, EntryType, Event, Frame, Thread} from 'sentry/types';

type SuspectFrame = {
module: string | RegExp;
Expand All @@ -19,9 +20,24 @@ type SuspectFrame = {
const CULPRIT_FRAMES: SuspectFrame[] = [
{
module: 'libcore.io.Linux',
functions: ['read', 'write', 'fstat', 'fsync', 'fdatasync', 'access', 'open'],
offendingThreadStates: [ThreadStates.WAITING, ThreadStates.TIMED_WAITING],
resources: t('Move File I/O off the main thread.'),
functions: [
'read',
'write',
'fstat',
'fsync',
'fdatasync',
'access',
'open',
'chmod',
],
offendingThreadStates: [
ThreadStates.WAITING,
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
resources: t(
'File I/O operations, such as reading from or writing to files on disk, can be time-consuming, especially if the file size is large or the storage medium is slow. Move File I/O off the main thread to avoid this ANR.'
),
},
{
module: 'android.database.sqlite.SQLiteConnection',
Expand All @@ -31,24 +47,39 @@ const CULPRIT_FRAMES: SuspectFrame[] = [
/nativeExecuteFor[a-zA-Z]+/,
/nativeBind[a-zA-Z]+/,
/nativeGet[a-zA-Z]+/,
'nativePrepareStatement',
],
offendingThreadStates: [
ThreadStates.WAITING,
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
offendingThreadStates: [ThreadStates.WAITING, ThreadStates.TIMED_WAITING],
resources: t('Move database operations off the main thread.'),
resources: t(
'Database operations, such as querying, inserting, updating, or deleting data, can involve disk I/O, processing, and potentially long-running operations. Move database operations off the main thread to avoid this ANR.'
),
},
{
module: 'android.app.SharedPreferencesImpl$EditorImpl',
functions: ['commit'],
offendingThreadStates: [ThreadStates.WAITING, ThreadStates.TIMED_WAITING],
offendingThreadStates: [
ThreadStates.WAITING,
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
resources: t(
'Switch to SharedPreferences.apply or move commit to a background thread.'
"If you have a particularly large or complex SharedPreferences file or if you're performing multiple simultaneous commits in quick succession, this can lead to ANR. Switch to SharedPreferences.apply or move commit to a background thread to avoid this ANR."
),
},
{
module: /^android\.app\.SharedPreferencesImpl\$EditorImpl\$[0-9]/,
functions: ['run'],
offendingThreadStates: [ThreadStates.WAITING, ThreadStates.TIMED_WAITING],
offendingThreadStates: [
ThreadStates.WAITING,
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
resources: t(
'Switch to SharedPreferences.commit and move commit to a background thread.'
'SharedPreferences.apply will save data on background thread only if it happens before the activity/service finishes. Switch to SharedPreferences.commit and move commit to a background thread.'
),
},
{
Expand All @@ -60,7 +91,7 @@ const CULPRIT_FRAMES: SuspectFrame[] = [
ThreadStates.RUNNABLE,
],
resources: tct(
'Optimize cold/warm app starts by offloading operations off the main thread and [link:lazily initializing] components.',
'The app is initializing too many things on the main thread during app launch. To avoid this ANR, optimize cold/warm app starts by offloading operations off the main thread and [link:lazily initializing] components.',
{
link: (
<ExternalLink href="https://developer.android.com/topic/performance/vitals/launch-time#heavy-app" />
Expand All @@ -81,7 +112,9 @@ const CULPRIT_FRAMES: SuspectFrame[] = [
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
resources: t('If possible, move loading heavy assets off the main thread.'),
resources: t(
'If the AssetManager operation involves reading or loading a large asset file on the main thread, this can lead to ANR. Move loading heavy assets off the main thread to avoid this ANR.'
),
},
{
module: 'android.content.res.AssetManager',
Expand All @@ -91,7 +124,9 @@ const CULPRIT_FRAMES: SuspectFrame[] = [
ThreadStates.TIMED_WAITING,
ThreadStates.RUNNABLE,
],
resources: t('Look for heavy resources in the /res or /res/raw folders.'),
resources: t(
"If you're reading a particularly large raw file (for example, a video file) on the main thread, this can lead to ANR. Look for heavy resources in the '/res' or '/res/raw; folders to avoid this ANR."
),
},
{
module: 'android.view.LayoutInflater',
Expand All @@ -102,7 +137,7 @@ const CULPRIT_FRAMES: SuspectFrame[] = [
ThreadStates.RUNNABLE,
],
resources: tct(
'The app is potentially inflating a heavy, deeply-nested layout. [link:Optimize view hierarchy], use view stubs, use include/merge tags for reusing inflated views.',
'The app is potentially inflating a heavy, deeply-nested layout. [link:Optimize view hierarchy], use view stubs, use include/merge tags for reusing inflated views to avoid this ANR.',
{
link: (
<ExternalLink href="https://developer.android.com/develop/ui/views/layout/improving-layouts" />
Expand Down Expand Up @@ -177,30 +212,42 @@ export function analyzeFramesForRootCause(event: Event): {
return null;
}

const threads = event.entries.find(entry => entry.type === EntryType.THREADS) as
| EntryThreads
| undefined;
const currentThread = threads?.data.values?.find(thread => thread.current);
const currentThread = getCurrentThread(event);

for (let index = 0; index < exceptionFrames.length; index++) {
// iterating the frames in reverse order, because the topmost frames most like the root cause
for (let index = exceptionFrames.length - 1; index > 0; index--) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the max number of frames here? If it's really low we don't really need an early exit by reverse walking this array. Up to you though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need the early exit here in case there are multiple offending frames. The UI is only capable of rendering a single root cause and we're picking the one that is the closest to the exception root cause, that's why we reverse it. Unless I misunderstood your comment :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah there are multiple, nevermind 👍

const frame = exceptionFrames[index];
for (let culpritIndex = 0; culpritIndex < CULPRIT_FRAMES.length; culpritIndex++) {
const possibleCulprit = CULPRIT_FRAMES[culpritIndex];
if (
satisfiesModuleCondition(frame, possibleCulprit) &&
satisfiesFunctionCondition(frame, possibleCulprit) &&
satisfiesOffendingThreadCondition(currentThread?.state, possibleCulprit)
) {
return {
culprit:
typeof possibleCulprit.module === 'string'
? possibleCulprit.module
: possibleCulprit.module.toString(),
resources: possibleCulprit.resources,
};
}
const rootCause = analyzeFrameForRootCause(frame, currentThread);
if (!isNil(rootCause)) {
romtsn marked this conversation as resolved.
Show resolved Hide resolved
return rootCause;
}
}

return null;
}

export function analyzeFrameForRootCause(
frame: Frame,
currentThread: Thread | undefined
): {
culprit: string;
resources: React.ReactNode;
} | null {
for (let culpritIndex = 0; culpritIndex < CULPRIT_FRAMES.length; culpritIndex++) {
const possibleCulprit = CULPRIT_FRAMES[culpritIndex];
romtsn marked this conversation as resolved.
Show resolved Hide resolved
if (
satisfiesModuleCondition(frame, possibleCulprit) &&
satisfiesFunctionCondition(frame, possibleCulprit) &&
satisfiesOffendingThreadCondition(currentThread?.state, possibleCulprit)
) {
return {
culprit:
typeof possibleCulprit.module === 'string'
? possibleCulprit.module
: possibleCulprit.module.toString(),
resources: possibleCulprit.resources,
};
}
}
return null;
}
60 changes: 59 additions & 1 deletion static/app/components/events/interfaces/frame/deprecatedLine.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {render, screen, within} from 'sentry-test/reactTestingLibrary';

import DeprecatedLine from 'sentry/components/events/interfaces/frame/deprecatedLine';
import {Frame} from 'sentry/types';
import {EntryType, Frame} from 'sentry/types';

describe('Frame - Line', function () {
const event = TestStubs.Event();
Expand Down Expand Up @@ -191,4 +191,62 @@ describe('Frame - Line', function () {
expect(container).toSnapshot();
});
});

describe('ANR suspect frame', () => {
it('should render suspect frame', () => {
const org = {...TestStubs.Organization(), features: ['anr-analyze-frames']};
const eventWithThreads = {
...event,
entries: [
{
data: {
values: [
{
id: 13920,
current: true,
crashed: true,
name: 'puma 002',
stacktrace: null,
rawStacktrace: null,
state: 'WAITING',
},
],
},
type: EntryType.THREADS,
},
],
};
const suspectFrame: Frame = {
filename: 'Instrumentation.java',
absPath: 'Instrumentation.java',
module: 'android.app.Instrumentation',
package: null,
platform: null,
instructionAddr: null,
symbolAddr: null,
function: 'callApplicationOnCreate',
rawFunction: null,
symbol: null,
context: [],
lineNo: 1176,
colNo: null,
inApp: false,
trust: null,
errors: null,
vars: null,
};

render(
<DeprecatedLine
data={suspectFrame}
registers={{}}
components={[]}
event={eventWithThreads}
isExpanded
/>,
{organization: org}
);
expect(screen.getByText('Suspect Frame')).toBeInTheDocument();
});
});
});
18 changes: 18 additions & 0 deletions static/app/components/events/interfaces/frame/deprecatedLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@ import classNames from 'classnames';
import scrollToElement from 'scroll-to-element';

import {Button} from 'sentry/components/button';
import {analyzeFrameForRootCause} from 'sentry/components/events/interfaces/analyzeFrames';
import {
StacktraceFilenameQuery,
useSourceMapDebug,
} from 'sentry/components/events/interfaces/crashContent/exception/useSourceMapDebug';
import LeadHint from 'sentry/components/events/interfaces/frame/line/leadHint';
import {getCurrentThread} from 'sentry/components/events/interfaces/utils';
import StrictClick from 'sentry/components/strictClick';
import Tag from 'sentry/components/tag';
import {Tooltip} from 'sentry/components/tooltip';
Expand Down Expand Up @@ -197,6 +199,11 @@ export class DeprecatedLine extends Component<Props, State> {
scrollToElement('#images-loaded');
};

scrollToSuspectRootCause = event => {
event.stopPropagation(); // to prevent collapsing if collapsible
scrollToElement('#suspect-root-cause');
};

preventCollapse = evt => {
evt.stopPropagation();
};
Expand Down Expand Up @@ -262,6 +269,8 @@ export class DeprecatedLine extends Component<Props, State> {

renderDefaultLine() {
const {isHoverPreviewed, debugFrames, data} = this.props;
const organization = this.props.organization;
const anrCulprit = analyzeFrameForRootCause(data, getCurrentThread(this.props.event));
romtsn marked this conversation as resolved.
Show resolved Hide resolved

return (
<StrictClick onClick={this.isExpandable() ? this.toggleContext : undefined}>
Expand All @@ -281,6 +290,11 @@ export class DeprecatedLine extends Component<Props, State> {
</LeftLineTitle>
{this.renderRepeats()}
</DefaultLineTitleWrapper>
{organization?.features.includes('anr-analyze-frames') && anrCulprit ? (
<SuspectFrameTag type="warning" to="" onClick={this.scrollToSuspectRootCause}>
{t('Suspect Frame')}
</SuspectFrameTag>
) : null}
{!data.inApp ? <Tag>{t('System')}</Tag> : <Tag type="info">{t('In App')}</Tag>}
{this.renderExpander()}
</DefaultLine>
Expand Down Expand Up @@ -500,3 +514,7 @@ const IconWrapper = styled('div')`
align-items: center;
margin-right: ${space(1)};
`;

const SuspectFrameTag = styled(Tag)`
margin-right: ${space(1)};
`;
Loading