Skip to content
Draft
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
- **IMPORTANT:** This disables collecting external storage size (total/free) by default, to enable it back
use `options.isCollectExternalStorageContext = true` or `<meta-data android:name="io.sentry.external-storage-context" android:value="true" />`
- Fix `NullPointerException` when reading ANR marker ([#4979](https://github.com/getsentry/sentry-java/pull/4979))
- Improve app start type detection with main thread timing ([#4999](https://github.com/getsentry/sentry-java/pull/4999))

### Improvements

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public enum AppStartType {

private @NotNull AppStartType appStartType = AppStartType.UNKNOWN;
private boolean appLaunchedInForeground;
private volatile long firstPostUptimeMillis = -1;

private final @NotNull TimeSpan appStartSpan;
private final @NotNull TimeSpan sdkInitTimeSpan;
Expand Down Expand Up @@ -234,6 +235,7 @@ public void clear() {
shouldSendStartMeasurements = true;
firstDrawDone.set(false);
activeActivitiesCounter.set(0);
firstPostUptimeMillis = -1;
}

public @Nullable ITransactionProfiler getAppStartProfiler() {
Expand Down Expand Up @@ -316,7 +318,15 @@ public void registerLifecycleCallbacks(final @NotNull Application application) {
// (possibly others) the first task posted on the main thread is called before the
// Activity.onCreate callback. This is a workaround for that, so that the Activity.onCreate
// callback is called before the application one.
new Handler(Looper.getMainLooper()).post(() -> checkCreateTimeOnMain());
new Handler(Looper.getMainLooper())
.post(
new Runnable() {
@Override
public void run() {
firstPostUptimeMillis = SystemClock.uptimeMillis();
checkCreateTimeOnMain();
}
});
}

private void checkCreateTimeOnMain() {
Expand Down Expand Up @@ -348,7 +358,7 @@ public void onActivityCreated(@NonNull Activity activity, @Nullable Bundle saved
if (activeActivitiesCounter.incrementAndGet() == 1 && !firstDrawDone.get()) {
final long nowUptimeMs = SystemClock.uptimeMillis();

// If the app (process) was launched more than 1 minute ago, it's likely wrong
// If the app (process) was launched more than 1 minute ago, consider it a warm start
final long durationSinceAppStartMillis = nowUptimeMs - appStartSpan.getStartUptimeMs();
if (!appLaunchedInForeground || durationSinceAppStartMillis > TimeUnit.MINUTES.toMillis(1)) {
appStartType = AppStartType.WARM;
Comment on lines 358 to 364
Copy link

Choose a reason for hiding this comment

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

Bug: The appStartSpan is used before it is started in onActivityCreated(), causing cold starts to be incorrectly classified as warm starts.
Severity: HIGH | Confidence: High

🔍 Detailed Analysis

In the onActivityCreated method, the duration since app start is calculated using appStartSpan.getStartUptimeMs(). However, the appStartSpan is only started later within the same method. Because appStartSpan is not yet started, getStartUptimeMs() returns its default value of 0. This causes the durationSinceAppStartMillis to be incorrectly calculated as nowUptimeMs - 0, a large value that will almost always exceed one minute. As a result, the logic incorrectly classifies cold app starts as warm starts, skewing performance metrics.

💡 Suggested Fix

The logic should be changed to ensure appStartSpan is started before its start time is read for the duration calculation. Alternatively, add a check to see if the span has been started before using its value.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
sentry-android-core/src/main/java/io/sentry/android/core/performance/AppStartMetrics.java#L358-L364

Potential issue: In the `onActivityCreated` method, the duration since app start is
calculated using `appStartSpan.getStartUptimeMs()`. However, the `appStartSpan` is only
started later within the same method. Because `appStartSpan` is not yet started,
`getStartUptimeMs()` returns its default value of 0. This causes the
`durationSinceAppStartMillis` to be incorrectly calculated as `nowUptimeMs - 0`, a large
value that will almost always exceed one minute. As a result, the logic incorrectly
classifies cold app starts as warm starts, skewing performance metrics.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7993492

Expand All @@ -360,8 +370,12 @@ public void onActivityCreated(@NonNull Activity activity, @Nullable Bundle saved
CLASS_LOADED_UPTIME_MS = nowUptimeMs;
contentProviderOnCreates.clear();
applicationOnCreate.reset();
} else if (savedInstanceState != null) {
appStartType = AppStartType.WARM;
} else if (firstPostUptimeMillis > 0 && nowUptimeMs > firstPostUptimeMillis) {
appStartType = AppStartType.WARM;
} else {
appStartType = savedInstanceState == null ? AppStartType.COLD : AppStartType.WARM;
appStartType = AppStartType.COLD;
}
}
appLaunchedInForeground = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -537,4 +537,127 @@ class AppStartMetricsTest {

assertEquals(secondActivity, CurrentActivityHolder.getInstance().activity)
}

@Test
fun `firstPostUptimeMillis is properly cleared`() {
val metrics = AppStartMetrics.getInstance()
metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

val reflectionField = AppStartMetrics::class.java.getDeclaredField("firstPostUptimeMillis")
reflectionField.isAccessible = true
val firstPostValue = reflectionField.getLong(metrics)
assertTrue(firstPostValue > 0)

metrics.clear()

val clearedValue = reflectionField.getLong(metrics)
assertEquals(-1, clearedValue)
}

@Test
fun `firstPostUptimeMillis is set when registerLifecycleCallbacks is called`() {
val metrics = AppStartMetrics.getInstance()
val beforeRegister = SystemClock.uptimeMillis()

metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

val afterIdle = SystemClock.uptimeMillis()

val reflectionField = AppStartMetrics::class.java.getDeclaredField("firstPostUptimeMillis")
reflectionField.isAccessible = true
val firstPostValue = reflectionField.getLong(metrics)

assertTrue(firstPostValue >= beforeRegister)
assertTrue(firstPostValue <= afterIdle)
}

@Test
fun `Sets app launch type to WARM when activity created after firstPost`() {
val metrics = AppStartMetrics.getInstance()
assertEquals(AppStartMetrics.AppStartType.UNKNOWN, metrics.appStartType)

metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

SystemClock.setCurrentTimeMillis(SystemClock.uptimeMillis() + 100)
metrics.onActivityCreated(mock<Activity>(), null)

assertEquals(AppStartMetrics.AppStartType.WARM, metrics.appStartType)
}

@Test
fun `Sets app launch type to COLD when activity created before firstPost executes`() {
val metrics = AppStartMetrics.getInstance()
assertEquals(AppStartMetrics.AppStartType.UNKNOWN, metrics.appStartType)

metrics.registerLifecycleCallbacks(mock<Application>())
metrics.onActivityCreated(mock<Activity>(), null)

assertEquals(AppStartMetrics.AppStartType.COLD, metrics.appStartType)

Shadows.shadowOf(Looper.getMainLooper()).idle()

assertEquals(AppStartMetrics.AppStartType.COLD, metrics.appStartType)
}

@Test
fun `Sets app launch type to COLD when activity created at same time as firstPost`() {
val metrics = AppStartMetrics.getInstance()

val now = SystemClock.uptimeMillis()
val reflectionField = AppStartMetrics::class.java.getDeclaredField("firstPostUptimeMillis")
reflectionField.isAccessible = true
reflectionField.setLong(metrics, now)

SystemClock.setCurrentTimeMillis(now)
metrics.onActivityCreated(mock<Activity>(), null)

assertEquals(AppStartMetrics.AppStartType.COLD, metrics.appStartType)
}

@Test
fun `savedInstanceState check takes precedence over firstPost timing`() {
val metrics = AppStartMetrics.getInstance()

metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

SystemClock.setCurrentTimeMillis(SystemClock.uptimeMillis() + 100)
metrics.onActivityCreated(mock<Activity>(), mock<Bundle>())

assertEquals(AppStartMetrics.AppStartType.WARM, metrics.appStartType)
}

@Test
fun `timeout check takes precedence over firstPost timing`() {
val metrics = AppStartMetrics.getInstance()

metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

val futureTime = SystemClock.uptimeMillis() + TimeUnit.MINUTES.toMillis(2)
SystemClock.setCurrentTimeMillis(futureTime)
metrics.onActivityCreated(mock<Activity>(), null)

assertEquals(AppStartMetrics.AppStartType.WARM, metrics.appStartType)
assertTrue(metrics.appStartTimeSpan.hasStarted())
assertEquals(futureTime, metrics.appStartTimeSpan.startUptimeMs)
}

@Test
fun `firstPost timing does not affect subsequent activity creations`() {
val metrics = AppStartMetrics.getInstance()

metrics.registerLifecycleCallbacks(mock<Application>())
Shadows.shadowOf(Looper.getMainLooper()).idle()

SystemClock.setCurrentTimeMillis(SystemClock.uptimeMillis() + 100)
metrics.onActivityCreated(mock<Activity>(), null)
assertEquals(AppStartMetrics.AppStartType.WARM, metrics.appStartType)

metrics.onActivityCreated(mock<Activity>(), mock<Bundle>())
assertEquals(AppStartMetrics.AppStartType.WARM, metrics.appStartType)
}
}
Loading