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 all 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))
- TimetoTisplay correctly warns about not supporting the new React Native architecture ([#4160](https://github.com/getsentry/sentry-react-native/pull/4160))
- Native Wrapper method `setContext` ensures only values convertible to NativeMap are passed ([#4168](https://github.com/getsentry/sentry-react-native/pull/4168))
- Native Wrapper method `setExtra` ensures only stringified values are passed ([#4168](https://github.com/getsentry/sentry-react-native/pull/4168))
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 @@ -712,6 +709,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,39 @@
package io.sentry.react;

import com.facebook.react.bridge.Promise;

import android.os.Handler;
import android.os.Looper;
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) {
Looper mainLooper = Looper.getMainLooper();

if (mainLooper == null) {
promise.reject("GetTimeToDisplay is not able to measure the time to display: Main looper not available.");
return;
}

// Ensure the code runs on the main thread
new Handler(mainLooper)
.post(() -> {
try {
Choreographer choreographer = Choreographer.getInstance();

// Invoke the callback after the frame is rendered
choreographer.postFrameCallback(frameTimeNanos -> {
final SentryDate endDate = dateProvider.now();
promise.resolve(endDate.nanoTimestamp() / 1e9);
});
} catch (Exception exception) {
promise.reject("Failed to receive the instance of Choreographer", exception);
}
});
}
}
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 @@ -786,4 +790,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
45 changes: 21 additions & 24 deletions src/js/tracing/reactnavigation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import type { Span, Transaction as TransactionType, TransactionContext } from '@
import { logger, timestampInSeconds } from '@sentry/utils';

import type { NewFrameEvent } from '../utils/sentryeventemitter';
import { type SentryEventEmitter, createSentryEventEmitter, NewFrameEventName } from '../utils/sentryeventemitter';
import type { SentryEventEmitterFallback } from '../utils/sentryeventemitterfallback';
import { createSentryFallbackEventEmitter } from '../utils/sentryeventemitterfallback';
import { RN_GLOBAL_OBJ } from '../utils/worldwide';
import { NATIVE } from '../wrapper';
import type { OnConfirmRoute, TransactionCreator } from './routingInstrumentation';
Expand Down Expand Up @@ -77,16 +78,15 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati
public readonly name: string = ReactNavigationInstrumentation.instrumentationName;

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

private _newScreenFrameEventEmitter: SentryEventEmitterFallback | null = null;
private readonly _maxRecentRouteLen: number = 200;

private _latestRoute?: NavigationRoute;
private _latestTransaction?: TransactionType;
private _navigationProcessingSpan?: Span;

private _initialStateHandled: boolean = false;
private _stateChangeTimeout?: number | undefined;
private _stateChangeTimeout?: ReturnType<typeof setTimeout> | undefined;
private _recentRouteKeys: string[] = [];

private _options: ReactNavigationOptions;
Expand All @@ -100,8 +100,8 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati
};

if (this._options.enableTimeToInitialDisplay) {
this._newScreenFrameEventEmitter = createSentryEventEmitter();
this._newScreenFrameEventEmitter.initAsync(NewFrameEventName);
this._newScreenFrameEventEmitter = createSentryFallbackEventEmitter();
this._newScreenFrameEventEmitter.initAsync();
NATIVE.initNativeReactNavigationNewFrameTracking().catch((reason: unknown) => {
logger.error(`[ReactNavigationInstrumentation] Failed to initialize native new frame tracking: ${reason}`);
});
Expand Down Expand Up @@ -247,24 +247,21 @@ export class ReactNavigationInstrumentation extends InternalRoutingInstrumentati
isAutoInstrumented: true,
});

!routeHasBeenSeen &&
latestTtidSpan &&
this._newScreenFrameEventEmitter?.once(
NewFrameEventName,
({ newFrameTimestampInSeconds }: NewFrameEvent) => {
const activeSpan = getActiveSpan();
if (activeSpan && manualInitialDisplaySpans.has(activeSpan)) {
logger.warn(
'[ReactNavigationInstrumentation] Detected manual instrumentation for the current active span.',
);
return;
}

latestTtidSpan.setStatus('ok');
latestTtidSpan.end(newFrameTimestampInSeconds);
setSpanDurationAsMeasurementOnTransaction(latestTransaction, 'time_to_initial_display', latestTtidSpan);
},
);
if (!routeHasBeenSeen && latestTtidSpan) {
this._newScreenFrameEventEmitter?.onceNewFrame(({ newFrameTimestampInSeconds }: NewFrameEvent) => {
const activeSpan = getActiveSpan();
if (activeSpan && manualInitialDisplaySpans.has(activeSpan)) {
logger.warn(
'[ReactNavigationInstrumentation] Detected manual instrumentation for the current active span.',
);
return;
}

latestTtidSpan.setStatus('ok');
latestTtidSpan.end(newFrameTimestampInSeconds);
setSpanDurationAsMeasurementOnTransaction(latestTransaction, 'time_to_initial_display', latestTtidSpan);
});
}

this._navigationProcessingSpan?.updateName(`Processing navigation to ${route.name}`);
this._navigationProcessingSpan?.setStatus('ok');
Expand Down
Loading
Loading