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.

Sign up for GitHub

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

perf: add trace for UI startup #26636

Merged
merged 6 commits into from
Aug 28, 2024
Merged

perf: add trace for UI startup #26636

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d6d2c27
Add UI startup trace
matthewwalsh0 Aug 23, 2024
21e6184
Support default trace ID
matthewwalsh0 Aug 23, 2024
4fedf08
Use enum for trace name
matthewwalsh0 Aug 27, 2024
bfc5cd0
Merge branch 'develop' into chore/trace-ui-init
matthewwalsh0 Aug 27, 2024
0eec161
Fix linting
matthewwalsh0 Aug 27, 2024
b543539
Add missing trace names
matthewwalsh0 Aug 27, 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
34 changes: 34 additions & 0 deletions app/scripts/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { isManifestV3 } from '../../shared/modules/mv3.utils';
import { checkForLastErrorAndLog } from '../../shared/modules/browser-runtime.utils';
import { SUPPORT_LINK } from '../../shared/lib/ui-utils';
import { getErrorHtml } from '../../shared/lib/error-utils';
import { endTrace, trace } from '../../shared/lib/trace';
import ExtensionPlatform from './platforms/extension';
import { setupMultiplex } from './lib/stream-utils';
import { getEnvironmentType, getPlatform } from './lib/util';
Expand All @@ -40,10 +41,29 @@ const PHISHING_WARNING_PAGE_TIMEOUT = ONE_SECOND_IN_MILLISECONDS;
const PHISHING_WARNING_SW_STORAGE_KEY = 'phishing-warning-sw-registered';

let extensionPort;
let traceContext;
davidmurdoch marked this conversation as resolved.
Show resolved Hide resolved

start().catch(log.error);

async function start() {
const startTime = performance.now();

traceContext = trace({
name: 'UI Startup',
davidmurdoch marked this conversation as resolved.
Show resolved Hide resolved
startTime: performance.timeOrigin,
});

trace({
name: 'Load Scripts',
startTime: performance.timeOrigin,
parentContext: traceContext,
});

endTrace({
name: 'Load Scripts',
timestamp: performance.timeOrigin + startTime,
});

// create platform global
global.platform = new ExtensionPlatform();

Expand Down Expand Up @@ -75,6 +95,7 @@ async function start() {
// in later version we might try to improve it by reviving same streams.
updateUiStreams();
} else {
endTrace({ name: 'Background Connect' });
initializeUiWithTab(activeTab);
}
await loadPhishingWarningPage();
Expand Down Expand Up @@ -190,14 +211,26 @@ async function start() {

extensionPort.onMessage.addListener(messageListener);
extensionPort.onDisconnect.addListener(resetExtensionStreamAndListeners);

trace({
name: 'Background Connect',
parentContext: traceContext,
});
} else {
const messageListener = async (message) => {
if (message?.data?.method === 'startUISync') {
endTrace({ name: 'Background Connect' });
initializeUiWithTab(activeTab);
extensionPort.onMessage.removeListener(messageListener);
}
};

extensionPort.onMessage.addListener(messageListener);

trace({
name: 'Background Connect',
parentContext: traceContext,
});
}

function initializeUiWithTab(tab) {
Expand Down Expand Up @@ -289,6 +322,7 @@ function initializeUi(activeTab, connectionStream, cb) {
activeTab,
container,
backgroundConnection,
traceContext,
},
cb,
);
Expand Down
148 changes: 89 additions & 59 deletions shared/lib/trace.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,6 @@ const DATA_MOCK = {
data3: 123,
};

function mockGetMetaMetricsEnabled(enabled: boolean) {
global.sentry = {
getMetaMetricsEnabled: () => Promise.resolve(enabled),
};
}

describe('Trace', () => {
const startSpanMock = jest.mocked(startSpan);
const startSpanManualMock = jest.mocked(startSpanManual);
Expand All @@ -52,53 +46,30 @@ describe('Trace', () => {
});

describe('trace', () => {
// @ts-expect-error This function is missing from the Mocha type definitions
it.each([
['enabled', true],
['disabled', false],
])(
'executes callback if Sentry is %s',
async (_: string, sentryEnabled: boolean) => {
let callbackExecuted = false;

mockGetMetaMetricsEnabled(sentryEnabled);

await trace({ name: NAME_MOCK }, async () => {
callbackExecuted = true;
});

expect(callbackExecuted).toBe(true);
},
);
it('executes callback', () => {
let callbackExecuted = false;

// @ts-expect-error This function is missing from the Mocha type definitions
it.each([
['enabled', true],
['disabled', false],
])(
'returns value from callback if Sentry is %s',
async (_: string, sentryEnabled: boolean) => {
mockGetMetaMetricsEnabled(sentryEnabled);

const result = await trace({ name: NAME_MOCK }, async () => {
return true;
});

expect(result).toBe(true);
},
);
trace({ name: NAME_MOCK }, () => {
callbackExecuted = true;
});

it('invokes Sentry if callback provided and metrics enabled', async () => {
mockGetMetaMetricsEnabled(true);
expect(callbackExecuted).toBe(true);
});

it('returns value from callback', () => {
const result = trace({ name: NAME_MOCK }, () => true);
expect(result).toBe(true);
});

await trace(
it('invokes Sentry if callback provided', () => {
trace(
{
name: NAME_MOCK,
tags: TAGS_MOCK,
data: DATA_MOCK,
parentContext: PARENT_CONTEXT_MOCK,
},
async () => Promise.resolve(),
() => true,
);

expect(withIsolationScopeMock).toHaveBeenCalledTimes(1);
Expand All @@ -109,6 +80,7 @@ describe('Trace', () => {
name: NAME_MOCK,
parentSpan: PARENT_CONTEXT_MOCK,
attributes: DATA_MOCK,
op: 'custom',
},
expect.any(Function),
);
Expand All @@ -117,10 +89,8 @@ describe('Trace', () => {
expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
});

it('invokes Sentry if no callback provided and metrics enabled', async () => {
mockGetMetaMetricsEnabled(true);

await trace({
it('invokes Sentry if no callback provided', () => {
trace({
id: ID_MOCK,
name: NAME_MOCK,
tags: TAGS_MOCK,
Expand All @@ -136,6 +106,7 @@ describe('Trace', () => {
name: NAME_MOCK,
parentSpan: PARENT_CONTEXT_MOCK,
attributes: DATA_MOCK,
op: 'custom',
},
expect.any(Function),
);
Expand All @@ -144,24 +115,37 @@ describe('Trace', () => {
expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
});

it('does not invoke Sentry if no callback provided and no ID', async () => {
mockGetMetaMetricsEnabled(true);

await trace({
it('invokes Sentry if no callback provided with custom start time', () => {
trace({
id: ID_MOCK,
name: NAME_MOCK,
tags: TAGS_MOCK,
data: DATA_MOCK,
parentContext: PARENT_CONTEXT_MOCK,
startTime: 123,
});

expect(withIsolationScopeMock).toHaveBeenCalledTimes(0);
expect(startSpanManualMock).toHaveBeenCalledTimes(0);
expect(setTagsMock).toHaveBeenCalledTimes(0);
expect(withIsolationScopeMock).toHaveBeenCalledTimes(1);

expect(startSpanManualMock).toHaveBeenCalledTimes(1);
expect(startSpanManualMock).toHaveBeenCalledWith(
{
name: NAME_MOCK,
parentSpan: PARENT_CONTEXT_MOCK,
attributes: DATA_MOCK,
op: 'custom',
startTime: 123,
},
expect.any(Function),
);

expect(setTagsMock).toHaveBeenCalledTimes(1);
expect(setTagsMock).toHaveBeenCalledWith(TAGS_MOCK);
});
});

describe('endTrace', () => {
it('ends Sentry span matching name and ID', async () => {
it('ends Sentry span matching name and specified ID', () => {
const spanEndMock = jest.fn();
const spanMock = { end: spanEndMock } as unknown as Span;

Expand All @@ -171,7 +155,7 @@ describe('Trace', () => {
}),
);

await trace({
trace({
name: NAME_MOCK,
id: ID_MOCK,
tags: TAGS_MOCK,
Expand All @@ -184,7 +168,53 @@ describe('Trace', () => {
expect(spanEndMock).toHaveBeenCalledTimes(1);
});

it('does not end Sentry span if name and ID does not match', async () => {
it('ends Sentry span matching name and default ID', () => {
const spanEndMock = jest.fn();
const spanMock = { end: spanEndMock } as unknown as Span;

startSpanManualMock.mockImplementationOnce((_, fn) =>
fn(spanMock, () => {
// Intentionally empty
}),
);

trace({
name: NAME_MOCK,
tags: TAGS_MOCK,
data: DATA_MOCK,
parentContext: PARENT_CONTEXT_MOCK,
});

endTrace({ name: NAME_MOCK });

expect(spanEndMock).toHaveBeenCalledTimes(1);
});

it('ends Sentry span with custom timestamp', () => {
const spanEndMock = jest.fn();
const spanMock = { end: spanEndMock } as unknown as Span;

startSpanManualMock.mockImplementationOnce((_, fn) =>
fn(spanMock, () => {
// Intentionally empty
}),
);

trace({
name: NAME_MOCK,
id: ID_MOCK,
tags: TAGS_MOCK,
data: DATA_MOCK,
parentContext: PARENT_CONTEXT_MOCK,
});

endTrace({ name: NAME_MOCK, id: ID_MOCK, timestamp: 123 });

expect(spanEndMock).toHaveBeenCalledTimes(1);
expect(spanEndMock).toHaveBeenCalledWith(123);
});

it('does not end Sentry span if name and ID does not match', () => {
const spanEndMock = jest.fn();
const spanMock = { end: spanEndMock } as unknown as Span;

Expand All @@ -194,7 +224,7 @@ describe('Trace', () => {
}),
);

await trace({
trace({
name: NAME_MOCK,
id: ID_MOCK,
tags: TAGS_MOCK,
Expand Down
Loading
Loading