Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

## Unreleased

### Fixes

- Bound TTID/TTFD to prevent inflated transactions ([#6210](https://github.com/getsentry/sentry-react-native/pull/6210))

### Features

- Expose `pauseAppHangTracking` and `resumeAppHangTracking` APIs on iOS ([#6192](https://github.com/getsentry/sentry-react-native/pull/6192))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,30 +57,41 @@
});
const ttfdSpan = await addTimeToFullDisplay({ event, rootSpanId, transactionStartTimestampSeconds, ttidSpan });

if (ttidSpan?.start_timestamp && ttidSpan?.timestamp) {
const ttidDurationMs =
ttidSpan?.start_timestamp && ttidSpan?.timestamp
? (ttidSpan.timestamp - ttidSpan.start_timestamp) * 1000
: undefined;
const ttidDeadlineExceeded = ttidDurationMs !== undefined && isDeadlineExceeded(ttidDurationMs);

if (ttidDurationMs !== undefined && !ttidDeadlineExceeded) {
event.measurements['time_to_initial_display'] = {
value: (ttidSpan.timestamp - ttidSpan.start_timestamp) * 1000,
value: ttidDurationMs,
unit: 'millisecond',
};
}

if (ttfdSpan?.start_timestamp && ttfdSpan?.timestamp) {
const durationMs = (ttfdSpan.timestamp - ttfdSpan.start_timestamp) * 1000;
if (isDeadlineExceeded(durationMs)) {
const ttfdDurationMs =
ttfdSpan?.start_timestamp && ttfdSpan?.timestamp
? (ttfdSpan.timestamp - ttfdSpan.start_timestamp) * 1000
: undefined;
const ttfdDeadlineExceeded = ttfdDurationMs !== undefined && isDeadlineExceeded(ttfdDurationMs);

if (ttfdDurationMs !== undefined) {
if (ttfdDeadlineExceeded) {
if (event.measurements['time_to_initial_display']) {
event.measurements['time_to_full_display'] = event.measurements['time_to_initial_display'];
}
} else {
event.measurements['time_to_full_display'] = {
value: durationMs,
value: ttfdDurationMs,
unit: 'millisecond',
};
}
}

const newTransactionEndTimestampSeconds = Math.max(
ttidSpan?.timestamp ?? -1,
ttfdSpan?.timestamp ?? -1,
(!ttidDeadlineExceeded && ttidSpan?.timestamp) ? ttidSpan.timestamp : -1,
(!ttfdDeadlineExceeded && ttfdSpan?.timestamp) ? ttfdSpan.timestamp : -1,
event.timestamp ?? -1,
);
if (newTransactionEndTimestampSeconds !== -1) {
Expand Down Expand Up @@ -126,14 +137,18 @@
});
}

const manualDurationMs = (ttidEndTimestampSeconds - transactionStartTimestampSeconds) * 1000;
const manualStatus = isDeadlineExceeded(manualDurationMs) ? 'deadline_exceeded' : 'ok';

if (ttidSpan?.status && ttidSpan.status !== 'ok') {

Check warning on line 143 in packages/core/src/js/tracing/integrations/timeToDisplayIntegration.ts

View check run for this annotation

@sentry/warden / warden: code-review

[53E-M7K] TTFD existing-span update branch sets status='ok' even when deadline exceeded (additional location)

In `addTimeToFullDisplay`, when an unfinished TTFD span already exists in `event.spans` (created via `startTimeToFullDisplaySpan()`), the update branch unconditionally sets `ttfdSpan.status = 'ok'` regardless of duration. This is inconsistent with: (a) the TTFD create branch a few lines below, which correctly uses `isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok'`, and (b) the analogous TTID update branch (`addTimeToInitialDisplay`) which uses `manualStatus` derived from the deadline check. As a result, a TTFD span whose duration exceeds 30s will be reported with `status: 'ok'` in Sentry's UI even though the PR's stated intent is to mark deadline-exceeded TTFD spans accordingly (and the transaction timestamp protection in the same file already keys off duration, so timestamps are still bounded — only the span status is wrong). The new test `deadline exceeded full display does not extend transaction timestamp` only asserts `event.timestamp` and does not catch this; the analogous TTID test asserts `ttidSpan.status === 'deadline_exceeded'`.
ttidSpan.status = 'ok';
ttidSpan.status = manualStatus;

Check warning on line 144 in packages/core/src/js/tracing/integrations/timeToDisplayIntegration.ts

View check run for this annotation

@sentry/warden / warden: find-bugs

Existing TTFD span update path hardcodes `'ok'` status, bypassing deadline check

The `addTimeToFullDisplay` existing-span update path (around line 246 of this file) still hardcodes `ttfdSpan.status = 'ok'` instead of using `isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok'`, which is inconsistent with the analogous TTID fix in this hunk and allows a deadline-exceeded TTFD span to be emitted with the wrong status.
Comment thread
sentry-warden[bot] marked this conversation as resolved.
ttidSpan.timestamp = ttidEndTimestampSeconds;
debug.log(`[${INTEGRATION_NAME}] Updated existing ttid span.`, ttidSpan);
return ttidSpan;
}

ttidSpan = createSpanJSON({
status: manualStatus,
op: UI_LOAD_INITIAL_DISPLAY,
description: 'Time To Initial Display',
start_timestamp: transactionStartTimestampSeconds,
Expand Down Expand Up @@ -180,7 +195,9 @@
const viewNames = event.contexts?.app?.view_names;
const screenName = Array.isArray(viewNames) ? viewNames[0] : viewNames;

const durationMs = (ttidTimestampSeconds - transactionStartTimestampSeconds) * 1000;
const ttidSpan = createSpanJSON({
status: isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok',
op: UI_LOAD_INITIAL_DISPLAY,
description: screenName ? `${screenName} initial display` : 'Time To Initial Display',
start_timestamp: transactionStartTimestampSeconds,
Comment thread
sentry[bot] marked this conversation as resolved.
Expand Down
37 changes: 37 additions & 0 deletions packages/core/test/tracing/reactnavigation.ttid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -433,8 +433,45 @@
);
});

test('should mark ttid as deadline_exceeded and not extend transaction when ttid exceeds 30s', () => {
jest.runOnlyPendingTimers(); // Flush app start transaction

mockedNavigation.navigateToNewScreen();
const activeSpanId = spanToJSON(getActiveSpan()).span_id;
const transactionStartTimestamp = spanToJSON(getActiveSpan()).start_timestamp;

// Simulate a stale TTID 60 seconds after the transaction start
mockRecordedTimeToDisplay({
ttidNavigation: {
[activeSpanId]: transactionStartTimestamp + 60,
},
});
jest.runOnlyPendingTimers(); // Flush ttid transaction

const transaction = getLastTransaction(transportSendMock);
expect(transaction).toEqual(
expect.objectContaining<TransactionEvent>({
type: 'transaction',
spans: expect.arrayContaining([
expect.objectContaining<Partial<SpanJSON>>({
op: 'ui.load.initial_display',
status: 'deadline_exceeded',
}),
]),
}),
);
expect(transaction.measurements).toBeOneOf([
undefined,
expect.not.objectContaining<Required<TransactionEvent>['measurements']>({
time_to_initial_display: expect.any(Object),
}),
]);
// Transaction timestamp should NOT be extended to the stale TTID
expect(transaction.timestamp - transaction.start_timestamp).toBeLessThan(30);
});

test('should not sample empty back navigation transactions with navigation processing', () => {
jest.runOnlyPendingTimers(); // Flush app start transaction

Check warning on line 474 in packages/core/test/tracing/reactnavigation.ttid.test.tsx

View check run for this annotation

@sentry/warden / warden: code-review

[53E-M7K] TTFD existing-span update branch sets status='ok' even when deadline exceeded (additional location)

In `addTimeToFullDisplay`, when an unfinished TTFD span already exists in `event.spans` (created via `startTimeToFullDisplaySpan()`), the update branch unconditionally sets `ttfdSpan.status = 'ok'` regardless of duration. This is inconsistent with: (a) the TTFD create branch a few lines below, which correctly uses `isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok'`, and (b) the analogous TTID update branch (`addTimeToInitialDisplay`) which uses `manualStatus` derived from the deadline check. As a result, a TTFD span whose duration exceeds 30s will be reported with `status: 'ok'` in Sentry's UI even though the PR's stated intent is to mark deadline-exceeded TTFD spans accordingly (and the transaction timestamp protection in the same file already keys off duration, so timestamps are still bounded — only the span status is wrong). The new test `deadline exceeded full display does not extend transaction timestamp` only asserts `event.timestamp` and does not catch this; the analogous TTID test asserts `ttidSpan.status === 'deadline_exceeded'`.

mockedNavigation.navigateToNewScreen();
mockAutomaticTimeToDisplay();
Expand Down
71 changes: 71 additions & 0 deletions packages/core/test/tracing/timetodisplay.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,77 @@
);
});

test('deadline exceeded full display does not extend transaction timestamp', async () => {
let transactionStartTimestamp: number;

startSpanManual(
{
name: 'Root Manual Span',
startTime: secondAgoTimestampMs(),
},
(activeSpan: Span | undefined) => {
transactionStartTimestamp = spanToJSON(activeSpan).start_timestamp;
startTimeToInitialDisplaySpan();
startTimeToFullDisplaySpan();

render(<TimeToInitialDisplay record={true} />);
render(<TimeToFullDisplay record={true} />);

mockRecordedTimeToDisplay({
ttid: {
[spanToJSON(activeSpan).span_id]: nowInSeconds(),
},
ttfd: {
[spanToJSON(activeSpan).span_id]: nowInSeconds() + 60,
},
});

activeSpan?.end();
},
);

await jest.runOnlyPendingTimersAsync();
await client.flush();

Check warning on line 326 in packages/core/test/tracing/timetodisplay.test.tsx

View check run for this annotation

@sentry/warden / warden: code-review

TTFD existing-span update branch sets status='ok' even when deadline exceeded

In `addTimeToFullDisplay`, when an unfinished TTFD span already exists in `event.spans` (created via `startTimeToFullDisplaySpan()`), the update branch unconditionally sets `ttfdSpan.status = 'ok'` regardless of duration. This is inconsistent with: (a) the TTFD create branch a few lines below, which correctly uses `isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok'`, and (b) the analogous TTID update branch (`addTimeToInitialDisplay`) which uses `manualStatus` derived from the deadline check. As a result, a TTFD span whose duration exceeds 30s will be reported with `status: 'ok'` in Sentry's UI even though the PR's stated intent is to mark deadline-exceeded TTFD spans accordingly (and the transaction timestamp protection in the same file already keys off duration, so timestamps are still bounded — only the span status is wrong). The new test `deadline exceeded full display does not extend transaction timestamp` only asserts `event.timestamp` and does not catch this; the analogous TTID test asserts `ttidSpan.status === 'deadline_exceeded'`.
const event = client.event!;
expect(event.timestamp! - transactionStartTimestamp!).toBeLessThan(30);

Check warning on line 328 in packages/core/test/tracing/timetodisplay.test.tsx

View check run for this annotation

@sentry/warden / warden: find-bugs

[KRK-MVH] Existing TTFD span update path hardcodes `'ok'` status, bypassing deadline check (additional location)

The `addTimeToFullDisplay` existing-span update path (around line 246 of this file) still hardcodes `ttfdSpan.status = 'ok'` instead of using `isDeadlineExceeded(durationMs) ? 'deadline_exceeded' : 'ok'`, which is inconsistent with the analogous TTID fix in this hunk and allows a deadline-exceeded TTFD span to be emitted with the wrong status.
});

test('deadline exceeded manual initial display does not extend transaction timestamp', async () => {
let transactionStartTimestamp: number;

startSpanManual(
{
name: 'Root Manual Span',
startTime: secondAgoTimestampMs(),
},
(activeSpan: Span | undefined) => {
transactionStartTimestamp = spanToJSON(activeSpan).start_timestamp;
startTimeToInitialDisplaySpan();

render(<TimeToInitialDisplay record={true} />);

mockRecordedTimeToDisplay({
ttid: {
[spanToJSON(activeSpan).span_id]: nowInSeconds() + 60,
},
});

activeSpan?.end();
},
);

await jest.runOnlyPendingTimersAsync();
await client.flush();

const event = client.event!;
expectNoInitialDisplayMeasurementOnSpan(event);
expect(event.timestamp! - transactionStartTimestamp!).toBeLessThan(30);

const ttidSpan = event.spans!.find(s => s.op === 'ui.load.initial_display');
expect(ttidSpan?.status).toBe('deadline_exceeded');
});

test('full display which ended before initial display is extended to initial display end', async () => {
const fullDisplayEndTimestampMs = secondInFutureTimestampMs();
const initialDisplayEndTimestampMs = secondInFutureTimestampMs() + 500;
Expand Down
Loading