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.

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

Re-enable Sentry on Android, with enableNdk: false #5765

Merged
merged 4 commits into from
Oct 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 6 additions & 3 deletions docs/howto/release.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,12 @@ simple terminology for the process we follow with both.
tools/checkout-keystore
```

* Do *not* apply the Sentry client key from the `release-secrets`
branch; if you're already on that branch (from e.g. making the iOS
build), move off of it. See our issues #5757 and #5766.
* Apply the Sentry client key (using the local branch created for this
in initial setup):

```
git rebase @ release-secrets
```

* Build the app, as both good old-fashioned APKs and a fancy new AAB:

Expand Down
16 changes: 9 additions & 7 deletions ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -436,14 +436,14 @@ PODS:
- RNScreens (3.25.0):
- React-Core
- React-RCTImage
- RNSentry (3.4.3):
- RNSentry (5.10.0):
- React-Core
- Sentry (= 7.11.0)
- Sentry/HybridSDK (= 8.11.0)
- RNVectorIcons (9.2.0):
- React-Core
- Sentry (7.11.0):
- Sentry/Core (= 7.11.0)
- Sentry/Core (7.11.0)
- Sentry/HybridSDK (8.11.0):
- SentryPrivate (= 8.11.0)
- SentryPrivate (8.11.0)
- SocketRocket (0.6.0)
- Toast (4.0.0)
- Yoga (1.14.0)
Expand Down Expand Up @@ -554,6 +554,7 @@ SPEC REPOS:
- libevent
- OpenSSL-Universal
- Sentry
- SentryPrivate
- SocketRocket
- Toast
- YogaKit
Expand Down Expand Up @@ -757,9 +758,10 @@ SPEC CHECKSUMS:
RNGestureHandler: c0d04458598fcb26052494ae23dda8f8f5162b13
RNReanimated: e7d8afaf8fed4b3bf1a46e06adb2e04a2b248f1c
RNScreens: 85d3880b52d34db7b8eeebe2f1a0e807c05e69fa
RNSentry: 85f6525b5fe8d2ada065858026b338605b3c09da
RNSentry: 11917f7bf3e28806aca4c2791c6ba7522d8f09fe
RNVectorIcons: fcc2f6cb32f5735b586e66d14103a74ce6ad61f8
Sentry: 0c5cd63d714187b4a39c331c1f0eb04ba7868341
Sentry: 39d57e691e311bdb73bc1ab5bbebbd6bc890050d
SentryPrivate: 48712023cdfd523735c2edb6b06bedf26c4730a3
SocketRocket: fccef3f9c5cedea1353a9ef6ada904fde10d6608
Toast: 91b396c56ee72a5790816f40d3a94dd357abc196
Yoga: 0bc4b37c3b8a345336ff601e2cf7d9704bab7e93
Expand Down
4 changes: 2 additions & 2 deletions ios/ZulipMobile.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "if [[ $USE_SENTRY != \"YES\" ]]; then\necho [Bundle React Native code and images] Skipping sentry\n../node_modules/react-native/scripts/react-native-xcode.sh\nelse\nexport SENTRY_PROPERTIES=sentry.properties\nexport NODE_BINARY=node\n../node_modules/@sentry/cli/bin/sentry-cli react-native xcode ../node_modules/react-native/scripts/react-native-xcode.sh\nfi\n";
shellScript = "if [[ $USE_SENTRY != \"YES\" ]]; then\necho [Bundle React Native code and images] Skipping sentry\n../node_modules/react-native/scripts/react-native-xcode.sh\nelse\nexport SENTRY_PROPERTIES=sentry.properties\nexport EXTRA_PACKAGER_ARGS=\"--sourcemap-output $DERIVED_FILE_DIR/main.jsbundle.map\"\nexport NODE_BINARY=node\n../node_modules/@sentry/cli/bin/sentry-cli react-native xcode ../node_modules/react-native/scripts/react-native-xcode.sh\nfi\n";
};
52F8B8FD88489D79CA92AA16 /* [CP] Check Pods Manifest.lock */ = {
isa = PBXShellScriptBuildPhase;
Expand Down Expand Up @@ -298,7 +298,7 @@
);
runOnlyForDeploymentPostprocessing = 0;
shellPath = /bin/sh;
shellScript = "if [[ $USE_SENTRY != \"YES\" ]]; then\necho [Upload Debug Symbols to Sentry] Skipping sentry\nelse\nexport SENTRY_PROPERTIES=sentry.properties\n../node_modules/@sentry/cli/bin/sentry-cli upload-dsym\nfi\n";
shellScript = "if [[ $USE_SENTRY != \"YES\" ]]; then\necho [Upload Debug Symbols to Sentry] Skipping sentry\nelse\nexport SENTRY_PROPERTIES=sentry.properties\n\n[[ $SENTRY_INCLUDE_NATIVE_SOURCES == \"true\" ]] && INCLUDE_SOURCES_FLAG=\"--include-sources\" || INCLUDE_SOURCES_FLAG=\"\"\nSENTRY_CLI=\"../node_modules/@sentry/cli/bin/sentry-cli\"\n$SENTRY_CLI debug-files upload \"$INCLUDE_SOURCES_FLAG\" \"$DWARF_DSYM_FOLDER_PATH\"\nfi\n";
};
8BC51697242D4EF80019892C /* Start Metro */ = {
isa = PBXShellScriptBuildPhase;
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"@react-navigation/material-top-tabs": "^5.2.19",
"@react-navigation/native": "^5.7.6",
"@react-navigation/stack": "npm:@zulip/[email protected]",
"@sentry/react-native": "^3.1.1",
"@sentry/react-native": "^5.9.2",
"@zulip/shared": "0.0.18",
"base-64": "^1.0.0",
"blueimp-md5": "^2.10.0",
Expand Down
6 changes: 4 additions & 2 deletions src/__tests__/sentry-test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
// @flow
/*
* @flow strict-local
*/

import * as Sentry from '@sentry/react-native';
import { isSentryActive } from '../sentry';
Expand All @@ -13,7 +15,7 @@ describe('sentry', () => {
expect(isSentryActive()).toBeFalse();
Sentry.addBreadcrumb({
message: 'test message',
level: Sentry.Severity.Debug,
level: 'debug',
});
expect(isSentryActive()).toBeFalse();
});
Expand Down
20 changes: 17 additions & 3 deletions src/sentry.js
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
/* @flow strict-local */
import * as Sentry from '@sentry/react-native';
import type { Breadcrumb, BreadcrumbHint } from '@sentry/react-native';
import { nativeApplicationVersion } from 'expo-application';
// $FlowFixMe[untyped-import]
import md5 from 'blueimp-md5';

import { Platform } from 'react-native';
import type { Identity } from './types';
import isAppOwnDomain from './isAppOwnDomain';
import store from './boot/store';
import { getIdentities } from './account/accountsSelectors';
import { sentryKey } from './sentryConfig';
import { isUrlOnRealm } from './utils/url';

// TODO import from @sentry/react-native libdef
type Breadcrumb = $FlowFixMe;
type BreadcrumbHint = $FlowFixMe;

export const isSentryActive = (): boolean => {
// Hub#getClient() is documented as possibly returning undefined, but the
// significance of `undefined` is not. In practice, it appears to be
Expand Down Expand Up @@ -122,8 +126,6 @@ function scrubUrl(unscrubbedUrl: void | string): void | string {
function scrubBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): Breadcrumb {
switch (breadcrumb.type) {
case 'http': {
// $FlowIgnore[incompatible-indexer] | We assume it's an
// $FlowIgnore[incompatible-type] | HttpBreadcrumb; see jsdoc.
const httpBreadcrumb: HttpBreadcrumb = breadcrumb;
return {
...httpBreadcrumb,
Expand Down Expand Up @@ -152,10 +154,22 @@ export const initializeSentry = () => {

Sentry.init({
dsn: key,

ignoreErrors: [
// RN's fetch implementation can raise these; we sometimes mimic it
'Network request failed',
],

...(Platform.OS === 'android'
? {
// Disable Sentry's native code (libsentry.so), because it has
// memory-corruption bugs that cause the app to crash.
// https://github.com/zulip/zulip-mobile/issues/5766
// https://github.com/getsentry/sentry-java/issues/2955#issuecomment-1739030872
enableNdk: false,
Copy link
Member

Choose a reason for hiding this comment

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

Greg found in testing that this, plus re-enabling Sentry on Android
(coming up) seems adequate for #5766.

In this commit message let's include a link for that testing, since that's easy to do:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/re-enabling.20Sentry/near/1654140

}
: Object.freeze({})),

beforeBreadcrumb(breadcrumb: Breadcrumb, hint?: BreadcrumbHint): Breadcrumb | null {
try {
return scrubBreadcrumb(breadcrumb, hint);
Expand Down
10 changes: 6 additions & 4 deletions src/utils/logging.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
/* @flow strict-local */
import type { Scope, SeverityType } from '@sentry/react-native';
import {
captureException,
captureMessage,
configureScope,
Severity,
withScope as withScopeImpl,
} from '@sentry/react-native';

Expand All @@ -16,6 +14,10 @@ import config from '../config';
/** Type of "extras" intended for Sentry. */
export type Extras = {| +[key: string]: JSONable |};

// TODO import from @sentry/react-native libdef
type Scope = $FlowFixMe;
type SeverityType = $FlowFixMe;

/**
* `Error`, but subclass instances have the name of the subclass at `.name`
*
Expand Down Expand Up @@ -183,7 +185,7 @@ const makeLogFunction = ({ consoleMethod, severity }: LogParams): LogFunction =>
*/
export const error: (event: string | Error, extras?: Extras) => void = makeLogFunction({
consoleMethod: console.error,
severity: Severity.Error,
severity: 'error',
});

/**
Expand All @@ -208,7 +210,7 @@ export const error: (event: string | Error, extras?: Extras) => void = makeLogFu
*/
export const warn: (event: string | Error, extras?: Extras) => void = makeLogFunction({
consoleMethod: console.warn,
severity: Severity.Warning,
severity: 'warning',
});

/**
Expand Down
6 changes: 2 additions & 4 deletions tools/android
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,14 @@ do_apk() {
check_yarn_link
run_visibly yarn

# TODO(#5766): start passing -Psentry again
run_visibly tools/gradle :app:assembleRelease -Psigned
run_visibly tools/gradle :app:assembleRelease -Psigned -Psentry
}

do_aab() {
check_yarn_link
run_visibly yarn

# TODO(#5766): start passing -Psentry again
run_visibly tools/gradle :app:bundleRelease -Psigned
run_visibly tools/gradle :app:bundleRelease -Psigned -Psentry
}

case "${action}" in
Expand Down
6 changes: 6 additions & 0 deletions types/@sentry/react-native.js.flow
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ export type Options = {|
same effect. */
dsn: string,

/** Enable NDK on Android
 *
 * @default true
 */
enableNdk?: boolean,

ignoreErrors?: Array<string | RegExp>,

/**
Expand Down
Loading
Loading