Skip to content
Open
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
6 changes: 6 additions & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,12 @@ public abstract interface class io/sentry/android/core/SentryAndroidOptions$Befo
public abstract fun execute (Lio/sentry/SentryEvent;Lio/sentry/Hint;Z)Z
}

public final class io/sentry/android/core/SentryFramesDelayResult {
public fun <init> (DI)V
public fun getDelaySeconds ()D
public fun getFramesContributingToDelayCount ()I
}

public final class io/sentry/android/core/SentryInitProvider {
public fun <init> ()V
public fun attachInfo (Landroid/content/Context;Landroid/content/pm/ProviderInfo;)V
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package io.sentry.android.core;

import org.jetbrains.annotations.ApiStatus;

/** Result of querying frame delay for a given time range. */
@ApiStatus.Internal
public final class SentryFramesDelayResult {

private final double delaySeconds;
private final int framesContributingToDelayCount;

public SentryFramesDelayResult(
final double delaySeconds, final int framesContributingToDelayCount) {
this.delaySeconds = delaySeconds;
this.framesContributingToDelayCount = framesContributingToDelayCount;
}

/**
* @return the total frame delay in seconds, or -1 if incalculable (e.g. no frame data available)
*/
public double getDelaySeconds() {
return delaySeconds;
}

/**
* @return the number of frames that contributed to the delay (slow + frozen frames)
*/
public int getFramesContributingToDelayCount() {
return framesContributingToDelayCount;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
import io.sentry.SentryUUID;
import io.sentry.android.core.BuildInfoProvider;
import io.sentry.android.core.ContextUtils;
import io.sentry.android.core.SentryFramesDelayResult;
import io.sentry.util.Objects;
import java.lang.ref.WeakReference;
import java.lang.reflect.Field;
import java.util.Iterator;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.CopyOnWriteArraySet;
import java.util.concurrent.TimeUnit;
import org.jetbrains.annotations.ApiStatus;
Expand All @@ -35,6 +38,8 @@
public final class SentryFrameMetricsCollector implements Application.ActivityLifecycleCallbacks {
private static final long oneSecondInNanos = TimeUnit.SECONDS.toNanos(1);
private static final long frozenFrameThresholdNanos = TimeUnit.MILLISECONDS.toNanos(700);
private static final int MAX_FRAMES_COUNT = 3600;
private static final long MAX_FRAME_AGE_NANOS = 5L * 60 * 1_000_000_000L; // 5 minutes

private final @NotNull BuildInfoProvider buildInfoProvider;
private final @NotNull Set<Window> trackedWindows = new CopyOnWriteArraySet<>();
Expand All @@ -53,6 +58,10 @@ public final class SentryFrameMetricsCollector implements Application.ActivityLi
private long lastFrameStartNanos = 0;
private long lastFrameEndNanos = 0;

// frame buffer for getFramesDelay queries, sorted by frame end time
private final @NotNull ConcurrentSkipListSet<DelayedFrame> delayedFrames =
new ConcurrentSkipListSet<>();

@SuppressLint("NewApi")
public SentryFrameMetricsCollector(
final @NotNull Context context,
Expand Down Expand Up @@ -177,6 +186,16 @@ public SentryFrameMetricsCollector(
isSlow(cpuDuration, (long) ((float) oneSecondInNanos / (refreshRate - 1.0f)));
final boolean isFrozen = isSlow && isFrozen(cpuDuration);

final long frameStartTime = startTime;

// store frames with delay for getFramesDelay queries
if (delayNanos > 0) {
if (delayedFrames.size() <= MAX_FRAMES_COUNT) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: An off-by-one error in the size check delayedFrames.size() <= MAX_FRAMES_COUNT allows the buffer to grow to 3601 elements, exceeding the intended maximum of 3600.
Severity: LOW

Suggested Fix

Change the buffer capacity check from delayedFrames.size() <= MAX_FRAMES_COUNT to delayedFrames.size() < MAX_FRAMES_COUNT. This will correctly prevent additions once the buffer size reaches the maximum limit, ensuring it does not exceed 3600 elements.

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/internal/util/SentryFrameMetricsCollector.java#L193

Potential issue: There is an off-by-one error in the buffer capacity check for
`delayedFrames`. The condition `delayedFrames.size() <= MAX_FRAMES_COUNT` allows one
more frame to be added after the buffer has reached its maximum intended size (`3600`).
When the size is exactly `3600`, the check passes, and another frame is added, bringing
the total size to `3601`. This violates the intended capacity limit and is inconsistent
with similar logic in `SpanFrameMetricsCollector`.

Did we get this right? ๐Ÿ‘ / ๐Ÿ‘Ž to inform future reviews.

delayedFrames.add(new DelayedFrame(frameStartTime, lastFrameEndNanos, delayNanos));
}
pruneOldFrames(lastFrameEndNanos);
}

for (FrameMetricsCollectorListener l : listenerMap.values()) {
l.onFrameMetricCollected(
startTime,
Expand Down Expand Up @@ -354,6 +373,87 @@ public long getLastKnownFrameStartTimeNanos() {
return -1;
}

/**
* Queries the frame delay for a given time range.
*
* <p>This is useful for external consumers (e.g. React Native SDK) that need to query frame delay
* for an arbitrary time range without registering their own frame listener.
*
* @param startSystemNanos start of the time range in {@link System#nanoTime()} units
* @param endSystemNanos end of the time range in {@link System#nanoTime()} units
* @return a {@link SentryFramesDelayResult} with the delay in seconds and the number of frames
* contributing to delay, or a result with delaySeconds=-1 if incalculable
*/
public @NotNull SentryFramesDelayResult getFramesDelay(
final long startSystemNanos, final long endSystemNanos) {
if (!isAvailable) {
return new SentryFramesDelayResult(-1, 0);
}

if (endSystemNanos <= startSystemNanos) {
return new SentryFramesDelayResult(-1, 0);
}

long totalDelayNanos = 0;
int delayFrameCount = 0;

if (!delayedFrames.isEmpty()) {
final Iterator<DelayedFrame> iterator =
delayedFrames.tailSet(new DelayedFrame(startSystemNanos)).iterator();

while (iterator.hasNext()) {
final @NotNull DelayedFrame frame = iterator.next();

if (frame.startNanos >= endSystemNanos) {
break;
}

// The delay portion of a frame is at the end: [frameEnd - delay, frameEnd]
final long delayStart = frame.endNanos - frame.delayNanos;
final long delayEnd = frame.endNanos;

// Intersect the delay interval with the query range
final long overlapStart = Math.max(delayStart, startSystemNanos);
final long overlapEnd = Math.min(delayEnd, endSystemNanos);

if (overlapEnd > overlapStart) {
totalDelayNanos += (overlapEnd - overlapStart);
delayFrameCount++;
}
}
}

final double delaySeconds = totalDelayNanos / 1e9d;
return new SentryFramesDelayResult(delaySeconds, delayFrameCount);
}

private void pruneOldFrames(final long currentNanos) {
final long cutoff = currentNanos - MAX_FRAME_AGE_NANOS;
delayedFrames.headSet(new DelayedFrame(cutoff)).clear();
}

private static class DelayedFrame implements Comparable<DelayedFrame> {
final long startNanos;
final long endNanos;
final long delayNanos;

/** Sentinel constructor for set range queries (tailSet/headSet). */
DelayedFrame(final long timestampNanos) {
this(timestampNanos, timestampNanos, 0);
}

DelayedFrame(final long startNanos, final long endNanos, final long delayNanos) {
this.startNanos = startNanos;
this.endNanos = endNanos;
this.delayNanos = delayNanos;
}

@Override
public int compareTo(final @NotNull DelayedFrame o) {
return Long.compare(this.endNanos, o.endNanos);
}
Comment on lines +452 to +454
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The DelayedFrame.compareTo implementation only compares endNanos, causing the ConcurrentSkipListSet to incorrectly discard frames with identical end times, leading to potential data loss.
Severity: LOW

Suggested Fix

Update the DelayedFrame.compareTo method to be more robust. Instead of only comparing endNanos, it should also include other frame properties like startNanos or delayNanos in the comparison to ensure uniqueness for the ConcurrentSkipListSet.

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/internal/util/SentryFrameMetricsCollector.java#L452-L454

Potential issue: The `DelayedFrame.compareTo` method only compares the `endNanos`
property. When instances of `DelayedFrame` are stored in a `ConcurrentSkipListSet`, two
distinct frames that happen to have the exact same `endNanos` value will be treated as
duplicates. This results in one of the frames being silently discarded, leading to a
potential loss of frame metric data. While the probability of a nanosecond-perfect
collision is low, it is a correctness issue where the set does not store all unique
frames as intended.

Did we get this right? ๐Ÿ‘ / ๐Ÿ‘Ž to inform future reviews.

}

@ApiStatus.Internal
public interface FrameMetricsCollectorListener {
/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,152 @@ class SentryFrameMetricsCollectorTest {
assertEquals(0, collector.getProperty<Set<Window>>("trackedWindows").size)
}

@Test
fun `getFramesDelay returns -1 when not available`() {
val buildInfo =
mock<BuildInfoProvider> { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.M) }
val collector = fixture.getSut(context, buildInfo)

val result = collector.getFramesDelay(0, TimeUnit.SECONDS.toNanos(1))
assertEquals(-1.0, result.delaySeconds)
assertEquals(0, result.framesContributingToDelayCount)
}

@Test
fun `getFramesDelay returns -1 for invalid time range`() {
val collector = fixture.getSut(context)

val result = collector.getFramesDelay(2000, 1000)
assertEquals(-1.0, result.delaySeconds)
assertEquals(0, result.framesContributingToDelayCount)
}

@Test
fun `getFramesDelay returns zero delay when no slow frames recorded`() {
val buildInfo =
mock<BuildInfoProvider> { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.O) }
val collector = fixture.getSut(context, buildInfo)
Shadows.shadowOf(Looper.getMainLooper()).idle()
val listener =
collector.getProperty<Window.OnFrameMetricsAvailableListener>("frameMetricsAvailableListener")

collector.startCollection(mock())

// emit a fast frame (21ns cpu time โ€” well under 16ms budget)
listener.onFrameMetricsAvailable(createMockWindow(), createMockFrameMetrics(), 0)

// choreographer is at end of range so no pending delay
val choreographer = collector.getProperty<Choreographer>("choreographer")
choreographer.injectForField("mLastFrameTimeNanos", TimeUnit.SECONDS.toNanos(1))

val result = collector.getFramesDelay(0, TimeUnit.SECONDS.toNanos(1))
assertEquals(0.0, result.delaySeconds)
assertEquals(0, result.framesContributingToDelayCount)
}

@Test
fun `getFramesDelay calculates delay from slow frames`() {
val buildInfo =
mock<BuildInfoProvider> { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.O) }
val collector = fixture.getSut(context, buildInfo)
val listener =
collector.getProperty<Window.OnFrameMetricsAvailableListener>("frameMetricsAvailableListener")

collector.startCollection(mock())

// emit a slow frame (~100ms extra = ~116ms total, well over 16ms budget)
listener.onFrameMetricsAvailable(
createMockWindow(),
createMockFrameMetrics(extraCpuDurationNanos = TimeUnit.MILLISECONDS.toNanos(100)),
0,
)

// emit a frozen frame (~1000ms extra = ~1016ms total, well over 700ms)
listener.onFrameMetricsAvailable(
createMockWindow(),
createMockFrameMetrics(extraCpuDurationNanos = TimeUnit.MILLISECONDS.toNanos(1000)),
0,
)

// choreographer is at end of range so no pending delay
Shadows.shadowOf(Looper.getMainLooper()).idle()
val choreographer = collector.getProperty<Choreographer>("choreographer")
choreographer.injectForField("mLastFrameTimeNanos", TimeUnit.SECONDS.toNanos(5))

val result = collector.getFramesDelay(0, TimeUnit.SECONDS.toNanos(5))
assertTrue(result.delaySeconds > 0)
assertEquals(2, result.framesContributingToDelayCount)
}

@Test
fun `getFramesDelay handles partial frame overlap`() {
val buildInfo =
mock<BuildInfoProvider> { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.O) }
val collector = fixture.getSut(context, buildInfo)
val listener =
collector.getProperty<Window.OnFrameMetricsAvailableListener>("frameMetricsAvailableListener")

collector.startCollection(mock())

// emit a frozen frame (~1s)
listener.onFrameMetricsAvailable(
createMockWindow(),
createMockFrameMetrics(extraCpuDurationNanos = TimeUnit.SECONDS.toNanos(1)),
0,
)

// choreographer is at end of range
Shadows.shadowOf(Looper.getMainLooper()).idle()
val choreographer = collector.getProperty<Choreographer>("choreographer")
choreographer.injectForField("mLastFrameTimeNanos", TimeUnit.SECONDS.toNanos(5))

// query a range that only partially overlaps the frozen frame
// the frame starts around 50ns (INTENDED_VSYNC_TIMESTAMP), so querying from a later point
// should reduce the delay proportionally
val result = collector.getFramesDelay(0, TimeUnit.SECONDS.toNanos(5))
assertTrue(result.delaySeconds > 0)
assertEquals(1, result.framesContributingToDelayCount)
}

@Test
fun `old frames are automatically pruned`() {
val buildInfo =
mock<BuildInfoProvider> { whenever(it.sdkInfoVersion).thenReturn(Build.VERSION_CODES.O) }
val collector = fixture.getSut(context, buildInfo)
Shadows.shadowOf(Looper.getMainLooper()).idle()
val listener =
collector.getProperty<Window.OnFrameMetricsAvailableListener>("frameMetricsAvailableListener")
val choreographer = collector.getProperty<Choreographer>("choreographer")

collector.startCollection(mock())

val t0 = TimeUnit.MINUTES.toNanos(10) // start at a realistic base time

// emit a slow frame at t0
val frameMetrics1 =
createMockFrameMetrics(extraCpuDurationNanos = TimeUnit.MILLISECONDS.toNanos(100))
whenever(frameMetrics1.getMetric(FrameMetrics.INTENDED_VSYNC_TIMESTAMP)).thenReturn(t0)
listener.onFrameMetricsAvailable(createMockWindow(), frameMetrics1, 0)

choreographer.injectForField("mLastFrameTimeNanos", t0 + TimeUnit.SECONDS.toNanos(1))

// verify frame exists
val resultBefore = collector.getFramesDelay(t0, t0 + TimeUnit.SECONDS.toNanos(1))
assertEquals(1, resultBefore.framesContributingToDelayCount)

// emit another slow frame >5 minutes later to trigger auto-pruning
val t1 = t0 + TimeUnit.MINUTES.toNanos(6)
val frameMetrics2 =
createMockFrameMetrics(extraCpuDurationNanos = TimeUnit.MILLISECONDS.toNanos(100))
whenever(frameMetrics2.getMetric(FrameMetrics.INTENDED_VSYNC_TIMESTAMP)).thenReturn(t1)
listener.onFrameMetricsAvailable(createMockWindow(), frameMetrics2, 0)

// the first frame should have been pruned (>5min old)
choreographer.injectForField("mLastFrameTimeNanos", t1 + TimeUnit.SECONDS.toNanos(1))
val resultAfter = collector.getFramesDelay(t0, t0 + TimeUnit.SECONDS.toNanos(1))
assertEquals(0, resultAfter.framesContributingToDelayCount)
}

private fun createMockWindow(refreshRate: Float = 60F): Window {
val mockWindow = mock<Window>()
val mockDisplay = mock<Display>()
Expand Down
Loading