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

Fix: Implement fallback system to screens that aren't reporting on the native layer the time to display. #4042

Merged
merged 56 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 47 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
d0c0cf7
implement fallback system
lucas-zimerman Aug 22, 2024
1a52620
clear format
lucas-zimerman Aug 23, 2024
84301f7
backup
lucas-zimerman Aug 30, 2024
3423a61
wip
lucas-zimerman Sep 2, 2024
84c12ff
Merge branch 'main' into test/deadline
lucas-zimerman Sep 3, 2024
1e59d5b
wip iOS
lucas-zimerman Sep 11, 2024
8851215
ioscode
lucas-zimerman Sep 11, 2024
feb08cd
Merge branch 'main' into test/deadline
lucas-zimerman Sep 11, 2024
299fb34
fix typo
lucas-zimerman Sep 11, 2024
734354d
fix-time
lucas-zimerman Sep 11, 2024
dabf98b
removed wrapper placeholder, moved java implementation to separated f…
lucas-zimerman Sep 11, 2024
41879c1
refactored android to use turbo module, refactored fallback emitter
lucas-zimerman Sep 12, 2024
a181bcc
add tests and minor fixes
lucas-zimerman Sep 12, 2024
298a007
missing test file
lucas-zimerman Sep 12, 2024
a93b249
yarn fix
lucas-zimerman Sep 12, 2024
da76e21
fix ios module name
lucas-zimerman Sep 12, 2024
53e67ba
Update samples/react-native/src/Screens/PerformanceScreen.tsx
lucas-zimerman Sep 12, 2024
0344adb
Update samples/react-native/src/Screens/PerformanceScreen.tsx
lucas-zimerman Sep 12, 2024
b115fdd
Update samples/react-native/src/Screens/PerformanceScreen.tsx
lucas-zimerman Sep 12, 2024
99e74cf
name missing
lucas-zimerman Sep 12, 2024
1bb5c4d
Merge branch 'main' into test/deadline
lucas-zimerman Sep 18, 2024
6507648
fix java module name, add isAvailable to ignore macos
lucas-zimerman Sep 19, 2024
2f838b8
yarn fix
lucas-zimerman Sep 19, 2024
a8f5126
use impl method
lucas-zimerman Sep 19, 2024
fa03cc1
nit impl name
lucas-zimerman Sep 19, 2024
b6093d5
refactor how to use time to display fallback
lucas-zimerman Sep 20, 2024
d03004c
yarn fix
lucas-zimerman Sep 20, 2024
7e5f764
fix ios reference
lucas-zimerman Sep 20, 2024
49272a1
Merge branch 'main' into test/deadline
lucas-zimerman Sep 23, 2024
a22824b
changelog
lucas-zimerman Sep 23, 2024
c99200d
Apply suggestions from code review
lucas-zimerman Sep 23, 2024
64fd760
Apply suggestions from code review
lucas-zimerman Sep 24, 2024
dc9ca95
fix build
lucas-zimerman Sep 24, 2024
4710843
Removes unused java imports (#4119)
antonis Sep 25, 2024
1cd3b8e
Merge branch 'main' into test/deadline
lucas-zimerman Oct 2, 2024
54189db
Merge branch 'main' into test/deadline
lucas-zimerman Oct 10, 2024
45fca09
add missing App Metrics import
lucas-zimerman Oct 10, 2024
5c0681d
Merge branch 'main' into test/deadline
lucas-zimerman Oct 14, 2024
966ce1e
refactor tests / remove closeAll
lucas-zimerman Oct 14, 2024
4bb6434
commit java suggestion
lucas-zimerman Oct 14, 2024
add7f95
build fix
lucas-zimerman Oct 14, 2024
55e6589
small refactor and add clearTimeout
lucas-zimerman Oct 14, 2024
9885a15
use sentry timestampInSeconds
lucas-zimerman Oct 14, 2024
d87b40e
removed fallback from navigation, comment on timeout difference
lucas-zimerman Oct 14, 2024
962bb35
requested changes java
lucas-zimerman Oct 14, 2024
4b820fe
yarn fix / remove clearTimeout
lucas-zimerman Oct 14, 2024
a99c3c1
add note and fix time tests
lucas-zimerman Oct 14, 2024
4203401
Kw fallback emitter suggestion (#4171)
lucas-zimerman Oct 14, 2024
3dbcc41
Merge branch 'main' into test/deadline
lucas-zimerman Oct 14, 2024
4557f7a
fix build
lucas-zimerman Oct 14, 2024
9a5ce6d
fix test
lucas-zimerman Oct 14, 2024
342961d
forgot git add for mocked file
lucas-zimerman Oct 14, 2024
427e1c6
add try/catch block on java code
lucas-zimerman Oct 14, 2024
fcb9935
fix lint
lucas-zimerman Oct 14, 2024
2a7c8fa
always use main thread on ttid
lucas-zimerman Oct 15, 2024
d43ce2b
Merge branch 'v5' into test/deadline
lucas-zimerman Oct 15, 2024
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 @@ -4,6 +4,7 @@

### Fixes

- Enhanced accuracy of time-to-display spans. ([#4042](https://github.com/getsentry/sentry-react-native/pull/4042))
- Handles error with string cause ([#4163](https://github.com/getsentry/sentry-react-native/pull/4163))
- Use `appLaunchedInForeground` to determine invalid app start data on Android ([#4146](https://github.com/getsentry/sentry-react-native/pull/4146))
- Upload source maps for all release variants on Android (not only the last found) ([#4125](https://github.com/getsentry/sentry-react-native/pull/4125))
Expand Down
13 changes: 7 additions & 6 deletions android/src/main/java/io/sentry/react/RNSentryModuleImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,11 @@
import java.io.InputStream;
import java.nio.charset.Charset;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
import java.util.concurrent.CountDownLatch;

import io.sentry.Breadcrumb;
import io.sentry.DateUtils;
import io.sentry.HubAdapter;
import io.sentry.ILogger;
import io.sentry.ISentryExecutorService;
Expand Down Expand Up @@ -135,10 +131,13 @@ public class RNSentryModuleImpl {
/** Max trace file size in bytes. */
private long maxTraceFileSize = 5 * 1024 * 1024;

private final @NotNull SentryDateProvider dateProvider;

public RNSentryModuleImpl(ReactApplicationContext reactApplicationContext) {
packageInfo = getPackageInfo(reactApplicationContext);
this.reactApplicationContext = reactApplicationContext;
this.emitNewFrameEvent = createEmitNewFrameEvent();
this.dateProvider = new SentryAndroidDateProvider();
}

private ReactApplicationContext getReactApplicationContext() {
Expand All @@ -150,8 +149,6 @@ private ReactApplicationContext getReactApplicationContext() {
}

private @NotNull Runnable createEmitNewFrameEvent() {
final @NotNull SentryDateProvider dateProvider = new SentryAndroidDateProvider();

return () -> {
final SentryDate endDate = dateProvider.now();
WritableMap event = Arguments.createMap();
Expand Down Expand Up @@ -701,6 +698,10 @@ public void disableNativeFramesTracking() {
}
}

public void getNewScreenTimeToDisplay(Promise promise) {
RNSentryTimeToDisplay.GetTimeToDisplay(promise, dateProvider);
}

private String getProfilingTracesDirPath() {
if (cacheDirPath == null) {
cacheDirPath = new File(getReactApplicationContext().getCacheDir(), "sentry/react").getAbsolutePath();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,4 @@ public List<ViewManager> createViewManagers(
new RNSentryOnDrawReporterManager(reactContext)
);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package io.sentry.react;

import com.facebook.react.bridge.Promise;

import android.view.Choreographer;

import org.jetbrains.annotations.NotNull;
import io.sentry.SentryDate;
import io.sentry.SentryDateProvider;
import io.sentry.android.core.SentryAndroidDateProvider;

public class RNSentryTimeToDisplay {
public static void GetTimeToDisplay(Promise promise, SentryDateProvider dateProvider) {
Choreographer choreographer = Choreographer.getInstance();
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved

// Invoke the callback after the frame is rendered
choreographer.postFrameCallback(frameTimeNanos -> {
final SentryDate endDate = dateProvider.now();

promise.resolve(endDate.nanoTimestamp() / 1e9);
});
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,9 @@ public String getCurrentReplayId() {
public void crashedLastRun(Promise promise) {
this.impl.crashedLastRun(promise);
}

@Override
public void getNewScreenTimeToDisplay(Promise promise) {
this.impl.getNewScreenTimeToDisplay(promise);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -173,4 +173,9 @@ public String getCurrentReplayId() {
public void crashedLastRun(Promise promise) {
this.impl.crashedLastRun(promise);
}

@ReactMethod()
public void getNewScreenTimeToDisplay(Promise promise) {
this.impl.getNewScreenTimeToDisplay(promise);
}
}
9 changes: 9 additions & 0 deletions ios/RNSentry.mm
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#import <dlfcn.h>
#import "RNSentry.h"
#import "RNSentryTimeToDisplay.h"

#if __has_include(<React/RCTConvert.h>)
#import <React/RCTConvert.h>
Expand Down Expand Up @@ -62,6 +63,7 @@ + (void)storeEnvelope:(SentryEnvelope *)envelope;
@implementation RNSentry {
bool sentHybridSdkDidBecomeActive;
bool hasListeners;
RNSentryTimeToDisplay *_timeToDisplay;
}

- (dispatch_queue_t)methodQueue
Expand Down Expand Up @@ -106,6 +108,8 @@ + (BOOL)requiresMainQueueSetup {
sentHybridSdkDidBecomeActive = true;
}

_timeToDisplay = [[RNSentryTimeToDisplay alloc] init];

#if SENTRY_TARGET_REPLAY_SUPPORTED
[RNSentryReplay postInit];
#endif
Expand Down Expand Up @@ -778,4 +782,9 @@ - (NSDictionary*) fetchNativeStackFramesBy: (NSArray<NSNumber*>*)instructionsAdd
}
#endif

RCT_EXPORT_METHOD(getNewScreenTimeToDisplay:(RCTPromiseResolveBlock)resolve
rejecter:(RCTPromiseRejectBlock)reject) {
[_timeToDisplay getTimeToDisplay:resolve];
}

@end
7 changes: 7 additions & 0 deletions ios/RNSentryTimeToDisplay.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#import <React/RCTBridgeModule.h>

@interface RNSentryTimeToDisplay : NSObject

- (void)getTimeToDisplay:(RCTResponseSenderBlock)callback;

@end
43 changes: 43 additions & 0 deletions ios/RNSentryTimeToDisplay.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#import "RNSentryTimeToDisplay.h"
#import <QuartzCore/QuartzCore.h>
#import <React/RCTLog.h>

@implementation RNSentryTimeToDisplay
antonis marked this conversation as resolved.
Show resolved Hide resolved
{
CADisplayLink *displayLink;
RCTResponseSenderBlock resolveBlock;
}

// Rename requestAnimationFrame to getTimeToDisplay
- (void)getTimeToDisplay:(RCTResponseSenderBlock)callback
{
// Store the resolve block to use in the callback.
resolveBlock = callback;

#if TARGET_OS_IOS
// Create and add a display link to get the callback after the screen is rendered.
displayLink = [CADisplayLink displayLinkWithTarget:self selector:@selector(handleDisplayLink:)];
[displayLink addToRunLoop:[NSRunLoop mainRunLoop] forMode:NSRunLoopCommonModes];
#else
resolveBlock(@[]); // Return nothing if not iOS.
#endif
}

#if TARGET_OS_IOS
- (void)handleDisplayLink:(CADisplayLink *)link {
// Get the current time
NSTimeInterval currentTime = [[NSDate date] timeIntervalSince1970] * 1000.0; // Convert to milliseconds

// Ensure the callback is valid and pass the current time back
if (resolveBlock) {
resolveBlock(@[@(currentTime)]); // Call the callback with the current time
resolveBlock = nil; // Clear the block after it's called
}

// Invalidate the display link to stop future callbacks
[displayLink invalidate];
displayLink = nil;
}
#endif

@end
2 changes: 2 additions & 0 deletions src/js/NativeRNSentry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@ import type { UnsafeObject } from './utils/rnlibrariesinterface';
export interface Spec extends TurboModule {
addListener: (eventType: string) => void;
removeListeners: (id: number) => void;
getNewScreenTimeToDisplay(): Promise<number | undefined | null>;
addBreadcrumb(breadcrumb: UnsafeObject): void;
captureEnvelope(
bytes: string,
options: {
hardCrashed: boolean;
},
): Promise<boolean>;

captureScreenshot(): Promise<NativeScreenshot[] | undefined | null>;
clearBreadcrumbs(): void;
crash(): void;
Expand Down
5 changes: 2 additions & 3 deletions src/js/tracing/reactnavigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati

private _navigationContainer: NavigationContainer | null = null;
private _newScreenFrameEventEmitter: SentryEventEmitter | null = null;

private readonly _maxRecentRouteLen: number = 200;

private _latestRoute?: NavigationRoute;
Expand Down Expand Up @@ -247,8 +246,7 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati
isAutoInstrumented: true,
});

!routeHasBeenSeen &&
latestTtidSpan &&
if (!routeHasBeenSeen && latestTtidSpan) {
this._newScreenFrameEventEmitter?.once(
NewFrameEventName,
({ newFrameTimestampInSeconds }: NewFrameEvent) => {
Expand All @@ -265,6 +263,7 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati
setSpanDurationAsMeasurementOnTransaction(latestTransaction, 'time_to_initial_display', latestTtidSpan);
},
);
}

this._navigationProcessingSpan?.updateName(`Processing navigation to ${route.name}`);
this._navigationProcessingSpan?.setStatus('ok');
Expand Down
6 changes: 6 additions & 0 deletions src/js/utils/sentryeventemitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { EmitterSubscription, NativeModule } from 'react-native';
import { NativeEventEmitter } from 'react-native';

import { getRNSentryModule } from '../wrapper';
import { createSentryFallbackEventEmitter } from './sentryeventemitterfallback';

export const NewFrameEventName = 'rn_sentry_new_frame';
export type NewFrameEventName = typeof NewFrameEventName;
Expand Down Expand Up @@ -32,6 +33,7 @@ export function createSentryEventEmitter(
if (!sentryNativeModule) {
return createNoopSentryEventEmitter();
}
const fallbackEventEmitter = createSentryFallbackEventEmitter();

const openNativeListeners = new Map<NewFrameEventName, EmitterSubscription>();
const listenersMap = new Map<NewFrameEventName, Map<(event: NewFrameEvent) => void, true>>();
Expand Down Expand Up @@ -70,6 +72,8 @@ export function createSentryEventEmitter(
openNativeListeners.set(eventType, nativeListener);

listenersMap.set(eventType, new Map());

fallbackEventEmitter.initAsync();
},
closeAllAsync() {
openNativeListeners.forEach(subscription => {
Expand All @@ -81,6 +85,8 @@ export function createSentryEventEmitter(
addListener,
removeListener,
once(eventType: NewFrameEventName, listener: (event: NewFrameEvent) => void) {
fallbackEventEmitter?.startListenerAsync();

const tmpListener = (event: NewFrameEvent): void => {
listener(event);
removeListener(eventType, tmpListener);
Expand Down
93 changes: 93 additions & 0 deletions src/js/utils/sentryeventemitterfallback.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
import { logger, timestampInSeconds } from '@sentry/utils';
import { DeviceEventEmitter } from 'react-native';

import { NATIVE } from '../wrapper';
import { NewFrameEventName } from './sentryeventemitter';

export type FallBackNewFrameEvent = { newFrameTimestampInSeconds: number; isFallback?: boolean };
export interface SentryEventEmitterFallback {
/**
* Initializes the fallback event emitter
* This method is synchronous in JS but the event emitter starts asynchronously.
*/
initAsync: () => void;
startListenerAsync: () => void;
}

/**
* Creates emitter that allows to listen to UI Frame events when ready.
*/
export function createSentryFallbackEventEmitter(): SentryEventEmitterFallback {
let NativeEmitterCalled: boolean = false;
let isListening = false;

function defaultFallbackEventEmitter(): void {
// https://reactnative.dev/docs/timers#timers
// NOTE: The current implementation of requestAnimationFrame is the same
// as setTimeout(0). This isn't exactly how requestAnimationFrame
// is supposed to work on web, so it doesn't get called when UI Frames are rendered.: https://github.com/facebook/react-native/blob/5106933c750fee2ce49fe1945c3e3763eebc92bc/packages/react-native/ReactCommon/react/runtime/TimerManager.cpp#L442-L443
requestAnimationFrame(() => {
if (NativeEmitterCalled) {
NativeEmitterCalled = false;
isListening = false;
return;
}
const seconds = timestampInSeconds();
waitForNativeResponseOrFallback(seconds, 'JavaScript');
});
}

function waitForNativeResponseOrFallback(fallbackSeconds: number, origin: string): void {
let firstAttemptCompleted = false;

const checkNativeResponse = (): void => {
if (NativeEmitterCalled) {
NativeEmitterCalled = false;
isListening = false;
return; // Native Replied the bridge with a timestamp.
}
if (!firstAttemptCompleted) {
firstAttemptCompleted = true;
setTimeout(checkNativeResponse, 3_000);
} else {
logger.log(`[Sentry] Native event emitter did not reply in time. Using ${origin} fallback emitter.`);
isListening = false;
DeviceEventEmitter.emit(NewFrameEventName, {
newFrameTimestampInSeconds: fallbackSeconds,
isFallback: true,
});
}
};

// Start the retry process
checkNativeResponse();
}

return {
initAsync() {
DeviceEventEmitter.addListener(NewFrameEventName, () => {
// Avoid noise from pages that we do not want to track.
if (isListening) {
NativeEmitterCalled = true;
}
});
},

startListenerAsync() {
isListening = true;

NATIVE.getNewScreenTimeToDisplay()
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
.then(resolve => {
if (resolve) {
waitForNativeResponseOrFallback(resolve, 'Native');
} else {
defaultFallbackEventEmitter();
}
})
.catch((reason: Error) => {
logger.error('Failed to recceive Native fallback timestamp.', reason);
defaultFallbackEventEmitter();
});
},
};
}
9 changes: 9 additions & 0 deletions src/js/wrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ interface SentryNativeWrapper {
getCurrentReplayId(): string | null;

crashedLastRun(): Promise<boolean | null>;
getNewScreenTimeToDisplay(): Promise<number | null | undefined>;
}

const EOL = utf8ToBytes('\n');
Expand Down Expand Up @@ -656,6 +657,14 @@ export const NATIVE: SentryNativeWrapper = {
return typeof result === 'boolean' ? result : null;
},

getNewScreenTimeToDisplay(): Promise<number | null | undefined> {
if (!this.enableNative || !this._isModuleLoaded(RNSentry)) {
return Promise.resolve(null);
}

return RNSentry.getNewScreenTimeToDisplay();
},

/**
* Gets the event from envelopeItem and applies the level filter to the selected event.
* @param data An envelope item containing the event.
Expand Down
1 change: 1 addition & 0 deletions test/mockWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ const NATIVE: MockInterface<NativeType> = {
initNativeReactNavigationNewFrameTracking: jest.fn(),

crashedLastRun: jest.fn(),
getNewScreenTimeToDisplay: jest.fn().mockResolvedValue(42),
};

NATIVE.isNativeAvailable.mockReturnValue(true);
Expand Down
Loading
Loading