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

core(lantern): create network graph from trace (experimental) #16026

Merged
merged 65 commits into from
Jun 3, 2024
Merged
Changes from 1 commit
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
b4fa823
start on lantern from trace
connorjclark May 14, 2024
a487855
redirects
connorjclark Apr 25, 2024
acaa25c
timings
connorjclark Apr 25, 2024
5bde7b5
typescript
connorjclark Apr 25, 2024
aca1615
update
connorjclark Apr 25, 2024
6a50568
more redirect stuff
connorjclark Apr 25, 2024
171f7b7
update
connorjclark May 1, 2024
bc5ebbf
update
connorjclark May 1, 2024
318c6de
initiator call stack
connorjclark May 2, 2024
037e690
add a testing tool
connorjclark May 2, 2024
4e07d3a
xhr
connorjclark May 2, 2024
5a916b2
types
connorjclark May 2, 2024
9802496
fix timestamps
connorjclark May 13, 2024
f955dd6
fix test
connorjclark May 14, 2024
0c27d0b
update
connorjclark May 14, 2024
2b7d434
update
connorjclark May 14, 2024
53b713d
update te
connorjclark May 14, 2024
2b19b21
test: update interactive test trace
connorjclark May 14, 2024
0ccc87e
Merge branch 'inter-trace' into lantern-from-trace
connorjclark May 14, 2024
300ed78
update
connorjclark May 14, 2024
96d2ed7
fix timing
connorjclark May 14, 2024
9d8969e
Merge remote-tracking branch 'origin/main' into lantern-from-trace
connorjclark May 15, 2024
78e66e5
passing tests
connorjclark May 15, 2024
4166ca8
vary
connorjclark May 15, 2024
c2927c3
update
connorjclark May 15, 2024
552dda9
wip
connorjclark May 15, 2024
c0c75d0
move to lantern lib
connorjclark May 15, 2024
862cda9
wip
connorjclark May 15, 2024
32944ce
:wqerge remote-tracking branch 'origin/main' into lantern-from-trace
connorjclark May 15, 2024
d23acb0
wip
connorjclark May 15, 2024
dff6fe5
wip
connorjclark May 15, 2024
b5c0ec8
wip
connorjclark May 16, 2024
09ff562
wip
connorjclark May 17, 2024
8db8c7c
test
connorjclark May 20, 2024
bd244c7
wip
connorjclark May 20, 2024
b689180
wip
connorjclark May 20, 2024
ffc56e2
wip
connorjclark May 20, 2024
7d637a4
Merge remote-tracking branch 'origin/main' into lantern-from-trace
connorjclark May 30, 2024
e4196d8
update te
connorjclark May 30, 2024
11c0b5b
run lh tests using lantern with trace and cdt
connorjclark May 30, 2024
fc6b342
disable some tests that need more work
connorjclark May 30, 2024
841e534
remove dated comments
connorjclark May 30, 2024
61b52b7
update test
connorjclark May 30, 2024
84ef26a
tests: use new trace in metrics-test for pwa
connorjclark May 30, 2024
1644637
Merge branch 'test-update-metric-trace' into lantern-from-trace
connorjclark May 30, 2024
d6dd650
remove rounding thing that is not needed anymore
connorjclark May 30, 2024
c0e6226
only do lantern smoke for ToT
connorjclark May 30, 2024
40d0f2a
Merge remote-tracking branch 'origin/main' into lantern-from-trace
connorjclark May 30, 2024
183e50d
include audits in trace tests, and exclude many more
connorjclark May 30, 2024
9fdcbab
generate network revents in createTestTrace; fix redirect setup in ne…
connorjclark May 31, 2024
30460e7
work more on createTestTrace
connorjclark May 31, 2024
025fd52
decompose createGraphFromTrace into a few private methods
connorjclark May 31, 2024
8c938e4
comment
connorjclark May 31, 2024
976a91c
ts
connorjclark May 31, 2024
9811633
lint, fix a test
connorjclark May 31, 2024
20f72d0
fix minor things with createTestTrace, disabled tests
connorjclark May 31, 2024
6edccfe
network sizes
connorjclark May 31, 2024
a436e25
lint
connorjclark May 31, 2024
6b85157
more tweaks to redirect stuff
connorjclark May 31, 2024
6126b22
fix type
connorjclark May 31, 2024
b0a78af
fix wrong access for requestId in Lantern node
connorjclark May 31, 2024
007a6f7
account for all workers in our lookup
connorjclark Jun 3, 2024
34316d5
be more explicit
connorjclark Jun 3, 2024
bf7b5d2
pr
connorjclark Jun 3, 2024
df982f7
Merge remote-tracking branch 'origin/main' into lantern-from-trace
connorjclark Jun 3, 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
273 changes: 152 additions & 121 deletions core/lib/lantern/page-dependency-graph.js
Original file line number Diff line number Diff line change
Expand Up @@ -593,14 +593,13 @@ class PageDependencyGraph {
}

/**
* @param {LH.TraceEvent[]} mainThreadEvents
* @param {LH.Trace} trace
* @param {LH.Artifacts.TraceEngineResult} traceEngineResult
* @param {LH.Artifacts.URL} URL
* @return {Map<number, number[]>}
*/
static async createGraphFromTrace(mainThreadEvents, trace, traceEngineResult, URL) {
// TODO: trace engine should handle this.
static _findServiceWorkerThreads(trace) {
// TODO: trace engine should provide a map of service workers, like it does for workers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's possible that map already includes (same-process) serviceworkers… do you know it doesn't?
regardless having pid matching for it too seems like a requirement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, by looking at core/test/fixtures/traces/amp-m86.trace.json I see that service worker threads also get the same events a worker thread does for purposes of WorkersHandler

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified this method to account for all workers, so we build our own comprehensive pid/tid worker map.

const serviceWorkerThreads = new Map();

for (const event of trace.traceEvents) {
if (!(event.name === 'thread_name' && event.args.name === 'ServiceWorker thread')) {
continue;
Expand All @@ -614,112 +613,157 @@ class PageDependencyGraph {
}
}

/** @type {Lantern.NetworkRequest[]} */
const lanternRequests = [];
return serviceWorkerThreads;
}

for (const request of traceEngineResult.data.NetworkRequests.byTime) {
if (request.args.data.connectionId === undefined ||
request.args.data.connectionReused === undefined) {
throw new Error('Trace is too old');
}
/**
* @param {LH.Artifacts.TraceEngineResult} traceEngineResult
* @param {Map<number, number[]>} serviceWorkerThreads
* @param {import('@paulirish/trace_engine/models/trace/types/TraceEvents.js').SyntheticNetworkRequest} request
* @return {Lantern.NetworkRequest=}
*/
static _createLanternRequest(traceEngineResult, serviceWorkerThreads, request) {
if (request.args.data.connectionId === undefined ||
request.args.data.connectionReused === undefined) {
throw new Error('Trace is too old');
}

let url;
try {
// globalThis does exist.
// eslint-disable-next-line no-undef
url = new globalThis.URL(request.args.data.url);
} catch (e) {
continue;
}
let url;
try {
url = new URL(request.args.data.url);
} catch (e) {
return;
}

const timing = request.args.data.timing ? {
...request.args.data.timing,
workerFetchStart: -1,
workerRespondWithSettled: -1,
} : undefined;

const networkRequestTime = timing ?
timing.requestTime * 1000 :
request.args.data.syntheticData.downloadStart / 1000;

const parsedURL = {
scheme: url.protocol.split(':')[0],
// Intentional, DevTools uses different terminology
host: url.hostname,
securityOrigin: url.origin,
};

let fromWorker = false;
// TODO: should also check pid
if (traceEngineResult.data.Workers.workerIdByThread.has(request.tid)) {
fromWorker = true;
}
const tids = serviceWorkerThreads.get(request.pid);
if (tids?.includes(request.tid)) {
fromWorker = true;
}

// `initiator` in the trace does not contain the stack trace for JS-initiated
// requests. Instead, that is stored in the `stackTrace` property of the SyntheticNetworkRequest.
// There are some minor differences in the fields, accounted for here.
// Most importantly, there seems to be fewer frames in the trace than the equivalent
// events over the CDP. This results in less accuracy in determining the initiator request,
// which means less edges in the graph, which mean worse results. Should fix.
/** @type {Lantern.NetworkRequest['initiator']} */
const initiator = request.args.data.initiator ?? {type: 'other'};
if (request.args.data.stackTrace) {
const callFrames = request.args.data.stackTrace.map(f => {
return {
scriptId: String(f.scriptId),
url: f.url,
lineNumber: f.lineNumber - 1,
columnNumber: f.columnNumber - 1,
functionName: f.functionName,
};
});
initiator.stack = {callFrames};
}

let resourceType = request.args.data.resourceType;
if (request.args.data.initiator?.fetchType === 'xmlhttprequest') {
// @ts-expect-error yes XHR is a valid ResourceType. TypeScript const enums are so unhelpful.
resourceType = 'XHR';
}

lanternRequests.push({
requestId: request.args.data.requestId,
connectionId: request.args.data.connectionId,
connectionReused: request.args.data.connectionReused,
url: request.args.data.url,
protocol: request.args.data.protocol,
parsedURL,
documentURL: request.args.data.requestingFrameUrl,
rendererStartTime: request.ts / 1000,
networkRequestTime,
responseHeadersEndTime: request.args.data.syntheticData.downloadStart / 1000,
networkEndTime: request.args.data.syntheticData.finishTime / 1000,
transferSize: request.args.data.encodedDataLength,
resourceSize: request.args.data.decodedBodyLength,
fromDiskCache: request.args.data.syntheticData.isDiskCached,
fromMemoryCache: request.args.data.syntheticData.isMemoryCached,
isLinkPreload: request.args.data.isLinkPreload,
finished: request.args.data.finished,
failed: request.args.data.failed,
statusCode: request.args.data.statusCode,
initiator,
timing,
resourceType,
mimeType: request.args.data.mimeType,
priority: request.args.data.priority,
frameId: request.args.data.frame,
fromWorker,
record: request,
// Set below.
redirects: undefined,
redirectSource: undefined,
redirectDestination: undefined,
initiatorRequest: undefined,
const parsedURL = {
scheme: url.protocol.split(':')[0],
// Intentional, DevTools uses different terminology
host: url.hostname,
securityOrigin: url.origin,
};

const timing = request.args.data.timing ? {
...request.args.data.timing,
// These two timings are not included in the trace.
workerFetchStart: -1,
workerRespondWithSettled: -1,
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
} : undefined;

const networkRequestTime = timing ?
timing.requestTime * 1000 :
request.args.data.syntheticData.downloadStart / 1000;

let fromWorker = false;
// TODO: should also check pid
if (traceEngineResult.data.Workers.workerIdByThread.has(request.tid)) {
fromWorker = true;
}
const tids = serviceWorkerThreads.get(request.pid);
if (tids?.includes(request.tid)) {
fromWorker = true;
}

// `initiator` in the trace does not contain the stack trace for JS-initiated
// requests. Instead, that is stored in the `stackTrace` property of the SyntheticNetworkRequest.
// There are some minor differences in the fields, accounted for here.
// Most importantly, there seems to be fewer frames in the trace than the equivalent
// events over the CDP. This results in less accuracy in determining the initiator request,
// which means less edges in the graph, which mean worse results. Should fix.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix

Meaning we should add more info to the trace events?

Copy link
Collaborator Author

@connorjclark connorjclark Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - this is chromium work

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should track in b/324059246

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • initiator (and any relevant js callstack)

i think it is known already that these call stack frames are missing data.

cc @paulirish @adrianaixba

relevant: https://chromium-review.googlesource.com/c/chromium/src/+/5280651/7..11/third_party/blink/renderer/core/inspector/inspector_trace_events.cc#b848

/** @type {Lantern.NetworkRequest['initiator']} */
const initiator = request.args.data.initiator ?? {type: 'other'};
if (request.args.data.stackTrace) {
const callFrames = request.args.data.stackTrace.map(f => {
return {
scriptId: String(f.scriptId),
url: f.url,
lineNumber: f.lineNumber - 1,
columnNumber: f.columnNumber - 1,
functionName: f.functionName,
};
});
initiator.stack = {callFrames};
}

let resourceType = request.args.data.resourceType;
if (request.args.data.initiator?.fetchType === 'xmlhttprequest') {
// @ts-expect-error yes XHR is a valid ResourceType. TypeScript const enums are so unhelpful.
resourceType = 'XHR';
}

return {
requestId: request.args.data.requestId,
connectionId: request.args.data.connectionId,
connectionReused: request.args.data.connectionReused,
url: request.args.data.url,
protocol: request.args.data.protocol,
parsedURL,
documentURL: request.args.data.requestingFrameUrl,
rendererStartTime: request.ts / 1000,
networkRequestTime,
responseHeadersEndTime: request.args.data.syntheticData.downloadStart / 1000,
networkEndTime: request.args.data.syntheticData.finishTime / 1000,
transferSize: request.args.data.encodedDataLength,
resourceSize: request.args.data.decodedBodyLength,
fromDiskCache: request.args.data.syntheticData.isDiskCached,
fromMemoryCache: request.args.data.syntheticData.isMemoryCached,
isLinkPreload: request.args.data.isLinkPreload,
finished: request.args.data.finished,
failed: request.args.data.failed,
statusCode: request.args.data.statusCode,
initiator,
timing,
resourceType,
mimeType: request.args.data.mimeType,
priority: request.args.data.priority,
frameId: request.args.data.frame,
fromWorker,
record: request,
// Set below.
redirects: undefined,
redirectSource: undefined,
redirectDestination: undefined,
initiatorRequest: undefined,
};
}

/**
*
* @param {Lantern.NetworkRequest[]} lanternRequests
*/
static _linkInitiators(lanternRequests) {
/** @type {Map<string, Lantern.NetworkRequest[]>} */
const requestsByURL = new Map();
for (const request of lanternRequests) {
const requests = requestsByURL.get(request.url) || [];
requests.push(request);
requestsByURL.set(request.url, requests);
}

for (const request of lanternRequests) {
const initiatorRequest = PageDependencyGraph.chooseInitiatorRequest(request, requestsByURL);
if (initiatorRequest) {
request.initiatorRequest = initiatorRequest;
}
}
}

/**
* @param {LH.TraceEvent[]} mainThreadEvents
* @param {LH.Trace} trace
* @param {LH.Artifacts.TraceEngineResult} traceEngineResult
* @param {LH.Artifacts.URL} URL
*/
static async createGraphFromTrace(mainThreadEvents, trace, traceEngineResult, URL) {
const serviceWorkerThreads = this._findServiceWorkerThreads(trace);

/** @type {Lantern.NetworkRequest[]} */
const lanternRequests = [];
for (const request of traceEngineResult.data.NetworkRequests.byTime) {
const lanternRequest =
this._createLanternRequest(traceEngineResult, serviceWorkerThreads, request);
if (lanternRequest) {
lanternRequests.push(lanternRequest);
}
}

// TraceEngine consolidates all redirects into a single request object, but lantern needs
Expand Down Expand Up @@ -760,20 +804,7 @@ class PageDependencyGraph {
}
}

/** @type {Map<string, Lantern.NetworkRequest[]>} */
const requestsByURL = new Map();
for (const request of lanternRequests) {
const requests = requestsByURL.get(request.url) || [];
requests.push(request);
requestsByURL.set(request.url, requests);
}

for (const request of lanternRequests) {
const initiatorRequest = PageDependencyGraph.chooseInitiatorRequest(request, requestsByURL);
if (initiatorRequest) {
request.initiatorRequest = initiatorRequest;
}
}
this._linkInitiators(lanternRequests);

// This would already be sorted by rendererStartTime, if not for the redirect unwrapping done
// above.
Expand Down
Loading