-
-
Notifications
You must be signed in to change notification settings - Fork 361
fix(android): drop inflated app start for background-spawned processes #6526
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,11 +76,14 @@ | |
| import java.net.URISyntaxException; | ||
| import java.nio.charset.Charset; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import java.util.Set; | ||
| import java.util.concurrent.CountDownLatch; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
|
|
@@ -108,6 +111,36 @@ public class RNSentryModuleImpl { | |
|
|
||
| @VisibleForTesting static long lastStartTimestampMs = -1; | ||
|
|
||
| // App start reasons (from ApplicationStartInfo#getReason(), API 35+) that indicate the process | ||
| // was spawned by the system in the background rather than by a user-initiated launch. When the | ||
| // app is later brought to the foreground, the native SDK keeps the app start anchored at | ||
| // background process creation, which inflates app_start_cold/warm (see | ||
| // https://github.com/getsentry/sentry-react-native/issues/6382). We drop these app starts, | ||
| // similar to how the Cocoa SDK drops pre-warmed starts. | ||
| private static final Set<String> BACKGROUND_APP_START_REASONS = | ||
| new HashSet<>( | ||
| Arrays.asList( | ||
| "alarm", | ||
| "backup", | ||
| "boot_complete", | ||
| "broadcast", | ||
| "content_provider", | ||
| "job", | ||
| "push", | ||
| "service")); | ||
|
|
||
| /** | ||
| * @param appStartReason the process start reason from {@link | ||
| * io.sentry.android.core.performance.AppStartMetrics#getAppStartReason()} (API 35+, {@code | ||
| * null} on lower API levels or for an unmapped reason) | ||
| * @return {@code true} if the process was spawned by the system in the background rather than by | ||
| * a user-initiated launch, in which case the app start data should be dropped | ||
| */ | ||
| @VisibleForTesting | ||
| static boolean isBackgroundAppStartReason(final @Nullable String appStartReason) { | ||
| return appStartReason != null && BACKGROUND_APP_START_REASONS.contains(appStartReason); | ||
| } | ||
|
|
||
| // 700ms to constitute frozen frames. | ||
| private static final int FROZEN_FRAME_THRESHOLD = 700; | ||
| // 16ms (slower than 60fps) to constitute slow frames. | ||
|
|
@@ -367,6 +400,23 @@ protected void fetchNativeAppStart( | |
| return; | ||
| } | ||
|
|
||
| // On API 35+ the process start reason tells us whether the process was spawned in the | ||
| // background (e.g. by an FCM push, a job or a service) rather than by the user opening the app. | ||
| // In that case the native app start remains anchored at background process creation, so the | ||
| // reported cold/warm start is the full idle gap until the user finally opens the app. We drop | ||
| // such app starts to avoid inflating app_start_cold/warm. Reason is null on API < 35, where we | ||
| // fall back to the existing foreground check above. | ||
| final @Nullable String appStartReason = metrics.getAppStartReason(); | ||
| if (isBackgroundAppStartReason(appStartReason)) { | ||
| logger.log( | ||
| SentryLevel.WARNING, | ||
| "Invalid app start data: app was launched in the background (reason: " | ||
| + appStartReason | ||
| + ")."); | ||
| promise.resolve(null); | ||
| return; | ||
| } | ||
|
Comment on lines
+414
to
+418
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The code assumes Suggested FixAdd an integration test to verify the actual return value and format of Prompt for AI AgentDid we get this right? 👍 / 👎 to inform future reviews. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warm starts wrongly droppedMedium Severity
Triggered by project rule: PR Review Guidelines for Cursor Bot Reviewed by Cursor Bugbot for commit 0925ea0. Configure here. |
||
|
|
||
| WritableMap mutableMeasurement = | ||
| (WritableMap) RNSentryMapConverter.convertToWritable(metricsDataBag); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package io.sentry.react; | ||
|
|
||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| import org.junit.Test; | ||
|
|
||
| public class RNSentryAppStartTest { | ||
|
|
||
| @Test | ||
| public void backgroundReasonsAreTreatedAsBackgroundAppStart() { | ||
| final String[] backgroundReasons = { | ||
| "alarm", "backup", "boot_complete", "broadcast", "content_provider", "job", "push", "service" | ||
| }; | ||
|
|
||
| for (final String reason : backgroundReasons) { | ||
| assertTrue( | ||
| "Expected reason '" + reason + "' to be treated as a background app start", | ||
| RNSentryModuleImpl.isBackgroundAppStartReason(reason)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void userLaunchReasonsAreNotTreatedAsBackgroundAppStart() { | ||
| final String[] userLaunchReasons = {"launcher", "launcher_recents", "start_activity", "other"}; | ||
|
|
||
| for (final String reason : userLaunchReasons) { | ||
| assertFalse( | ||
| "Expected reason '" + reason + "' to be treated as a user-initiated app start", | ||
| RNSentryModuleImpl.isBackgroundAppStartReason(reason)); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void nullReasonIsNotTreatedAsBackgroundAppStart() { | ||
| // getAppStartReason() returns null on API < 35, where we fall back to the foreground check | ||
| // only. | ||
| assertFalse(RNSentryModuleImpl.isBackgroundAppStartReason(null)); | ||
| } | ||
|
|
||
| @Test | ||
| public void unknownReasonIsNotTreatedAsBackgroundAppStart() { | ||
| assertFalse(RNSentryModuleImpl.isBackgroundAppStartReason("some_future_reason")); | ||
| } | ||
| } |


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The new logic to filter background app starts in
fetchNativeAppStart()lacks integration test coverage. A regression could go undetected, reintroducing inflated app start metrics.Severity: LOW
Suggested Fix
Add an integration test for
fetchNativeAppStart(). This test should mock anAppStartMetricsobject wheregetAppStartReason()returns a background reason (e.g., "push") andisAppLaunchedInForeground()is true. Verify that the promise is resolved withnull, confirming the app start metric is correctly dropped.Prompt for AI Agent
Also affects:
packages/core/android/src/test/java/io/sentry/react/RNSentryAppStartTest.java:1~45