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: App start time span no longer created if too long #3299

Merged
merged 10 commits into from
Oct 9, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@

### Fixes

- App start time span no longer created if too long ([#3299](https://github.com/getsentry/sentry-react-native/pull/3299))
- Change log output to show what paths are considered when collecting modules ([#3316](https://github.com/getsentry/sentry-react-native/pull/3316))

### Dependencies
Expand Down
55 changes: 38 additions & 17 deletions src/js/tracing/reactnativetracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { defaultRequestInstrumentationOptions, instrumentOutgoingRequests } from
import type { Hub, IdleTransaction, Transaction } from '@sentry/core';
import { getActiveTransaction, getCurrentHub, startIdleTransaction } from '@sentry/core';
import type { EventProcessor, Integration, Transaction as TransactionType, TransactionContext } from '@sentry/types';
import { logger } from '@sentry/utils';
import { logger, timestampInSeconds } from '@sentry/utils';
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved

import { APP_START_COLD, APP_START_WARM } from '../measurements';
import type { NativeAppStartResponse } from '../NativeRNSentry';
Expand Down Expand Up @@ -328,6 +328,17 @@ export class ReactNativeTracing implements Integration {
return this._inflightInteractionTransaction;
}

/**
* Returns the App Start Duration in Milliseconds. Also returns undefined if not able do
* define the duration.
*/
private _getAppStartDurationMilliseconds(appStart: NativeAppStartResponse): number | undefined {
if (!this._appStartFinishTimestamp) {
return undefined;
}
return this._appStartFinishTimestamp * 1000 - appStart.appStartTime;
}

/**
* Instruments the app start measurements on the first route transaction.
* Starts a route transaction if there isn't routing instrumentation.
Expand All @@ -350,7 +361,11 @@ export class ReactNativeTracing implements Integration {
if (this.options.routingInstrumentation) {
this._awaitingAppStartData = appStart;
} else {
const appStartTimeSeconds = appStart.appStartTime / 1000;
const appStartDurationMilliseconds = this._getAppStartDurationMilliseconds(appStart);
const appStartTimeSeconds =
appStartDurationMilliseconds && appStartDurationMilliseconds < ReactNativeTracing._maxAppStart
? appStart.appStartTime / 1000
: timestampInSeconds();

lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
const idleTransaction = this._createRouteTransaction({
name: 'App Start',
Expand All @@ -359,20 +374,31 @@ export class ReactNativeTracing implements Integration {
});

if (idleTransaction) {
this._addAppStartData(idleTransaction, appStart);
this._addAppStartData(idleTransaction, appStart, appStartDurationMilliseconds);
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

/**
* Adds app start measurements and starts a child span on a transaction.
*/
private _addAppStartData(transaction: IdleTransaction, appStart: NativeAppStartResponse): void {
if (!this._appStartFinishTimestamp) {
private _addAppStartData(
transaction: IdleTransaction,
appStart: NativeAppStartResponse,
appStartDurationMilliseconds: number | undefined,
): void {
if (!appStartDurationMilliseconds) {
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
logger.warn('App start was never finished.');
return;
}

// we filter out app start more than 60s.
// this could be due to many different reasons.
// we've seen app starts with hours, days and even months.
if (appStartDurationMilliseconds >= ReactNativeTracing._maxAppStart) {
return;
}

const appStartTimeSeconds = appStart.appStartTime / 1000;

lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
const op = appStart.isColdStart ? APP_START_COLD_OP : APP_START_WARM_OP;
Expand All @@ -383,15 +409,6 @@ export class ReactNativeTracing implements Integration {
endTimestamp: this._appStartFinishTimestamp,
});

const appStartDurationMilliseconds = this._appStartFinishTimestamp * 1000 - appStart.appStartTime;

// we filter out app start more than 60s.
// this could be due to many different reasons.
// we've seen app starts with hours, days and even months.
if (appStartDurationMilliseconds >= ReactNativeTracing._maxAppStart) {
return;
}

const measurement = appStart.isColdStart ? APP_START_COLD : APP_START_WARM;
transaction.setMeasurement(measurement, appStartDurationMilliseconds, 'millisecond');
}
Expand Down Expand Up @@ -462,10 +479,14 @@ export class ReactNativeTracing implements Integration {

idleTransaction.registerBeforeFinishCallback(transaction => {
if (this.options.enableAppStartTracking && this._awaitingAppStartData) {
transaction.startTimestamp = this._awaitingAppStartData.appStartTime / 1000;
transaction.op = 'ui.load';
const appStartDurationMilliseconds = this._getAppStartDurationMilliseconds(this._awaitingAppStartData);

if (appStartDurationMilliseconds && appStartDurationMilliseconds < ReactNativeTracing._maxAppStart) {
transaction.startTimestamp = this._awaitingAppStartData.appStartTime / 1000;
}

this._addAppStartData(transaction, this._awaitingAppStartData);
transaction.op = UI_LOAD;
this._addAppStartData(transaction, this._awaitingAppStartData, appStartDurationMilliseconds);
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved

this._awaitingAppStartData = undefined;
}
Expand Down
37 changes: 37 additions & 0 deletions test/tracing/reactnativetracing.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ import {
APP_START_WARM as APP_START_WARM_OP,
UI_LOAD,
} from '../../src/js/tracing';
import { APP_START_WARM as APP_SPAN_START_WARM } from '../../src/js/tracing/ops';
import { ReactNativeTracing } from '../../src/js/tracing/reactnativetracing';
import { getTimeOriginMilliseconds } from '../../src/js/tracing/utils';
import { NATIVE } from '../../src/js/wrapper';
Expand Down Expand Up @@ -233,6 +234,42 @@ describe('ReactNativeTracing', () => {
}
});

it('Does not add app start span if more than 60s', async () => {
const integration = new ReactNativeTracing();

const timeOriginMilliseconds = Date.now();
const appStartTimeMilliseconds = timeOriginMilliseconds - 65000;
const mockAppStartResponse: NativeAppStartResponse = {
isColdStart: false,
appStartTime: appStartTimeMilliseconds,
didFetchAppStart: false,
};

mockFunction(getTimeOriginMilliseconds).mockReturnValue(timeOriginMilliseconds);
mockFunction(NATIVE.fetchNativeAppStart).mockResolvedValue(mockAppStartResponse);

const mockHub = getMockHub();
integration.setupOnce(addGlobalEventProcessor, () => mockHub);

await jest.advanceTimersByTimeAsync(500);

const transaction = mockHub.getScope()?.getTransaction();

expect(transaction).toBeDefined();

if (transaction) {
expect(
// @ts-expect-error access private for test
transaction.spanRecorder,
).toBeDefined();

expect(
// @ts-expect-error access private for test
transaction.spanRecorder.spans.some(span => span.op == APP_SPAN_START_WARM),
).toBe(false);
}
lucas-zimerman marked this conversation as resolved.
Show resolved Hide resolved
});

it('Does not create app start transaction if didFetchAppStart == true', async () => {
const integration = new ReactNativeTracing();

Expand Down
Loading