Skip to content

Commit

Permalink
fix: miscellaneous improvements for tracing UI (#23558)
Browse files Browse the repository at this point in the history
- feat(tracing): mark API requests with "API" label
- feat(tracing): do not attribute any resources to `route.` API calls;
  otherwise, network traffic might get inside the `route.` actions.
- fix(tracing): map actionIds from primary contexts to actionIds from
  non-primary contexts
- fix(tracing): show leading `/` in URL path in network panel

This is a result of a pair-programming session with @pavelfeldman
  • Loading branch information
aslushnikov committed Jun 7, 2023
1 parent 0f40904 commit 2e327c9
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 9 deletions.
1 change: 1 addition & 0 deletions packages/playwright-core/src/server/har/harTracer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ export class HarTracer {
if (!this._shouldIncludeEntryWithUrl(event.url.toString()))
return;
const harEntry = createHarEntry(event.method, event.url, undefined, this._options);
harEntry._apiRequest = true;
if (!this._options.omitCookies)
harEntry.request.cookies = event.cookies;
harEntry.request.headers = Object.entries(event.headers).map(([name, value]) => ({ name, value }));
Expand Down
12 changes: 10 additions & 2 deletions packages/trace-viewer/src/ui/modelUtil.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,13 @@ function indexModel(context: ContextEntry) {
for (let i = 0; i < context.actions.length; ++i) {
const action = context.actions[i] as any;
action[contextSymbol] = context;
action[nextInContextSymbol] = context.actions[i + 1];
}
let lastNonRouteAction = undefined;
for (let i = context.actions.length - 1; i >= 0; i--) {
const action = context.actions[i] as any;
action[nextInContextSymbol] = lastNonRouteAction;
if (!action.apiName.includes('route.'))
lastNonRouteAction = action;
}
for (const event of context.events)
(event as any)[contextSymbol] = context;
Expand All @@ -109,6 +115,7 @@ function mergeActions(contexts: ContextEntry[]) {
offset = context.actions[0].startTime - context.actions[0].wallTime;
}

const nonPrimaryIdToPrimaryId = new Map<string, string>();
for (const context of nonPrimaryContexts) {
for (const action of context.actions) {
if (offset) {
Expand All @@ -122,12 +129,13 @@ function mergeActions(contexts: ContextEntry[]) {
const key = `${action.apiName}@${action.wallTime}`;
const existing = map.get(key);
if (existing && existing.apiName === action.apiName) {
nonPrimaryIdToPrimaryId.set(action.callId, existing.callId);
if (action.error)
existing.error = action.error;
if (action.attachments)
existing.attachments = action.attachments;
if (action.parentId)
existing.parentId = action.parentId;
existing.parentId = nonPrimaryIdToPrimaryId.get(action.parentId) ?? action.parentId;
continue;
}
map.set(key, { ...action, context });
Expand Down
7 changes: 7 additions & 0 deletions packages/trace-viewer/src/ui/networkResourceDetails.css
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
border-radius: 4px;
margin: 2px;
line-height: 20px;
min-width: 30px;
text-align: center;
}

.network-request-title-status.status-failure {
Expand All @@ -51,6 +53,11 @@
background-color: var(--vscode-statusBar-background);
}

.network-request-title-status.status-route.api {
color: var(--vscode-statusBar-foreground);
background-color: var(--vscode-statusBarItem-remoteBackground);
}

.network-request-title-url {
overflow: hidden;
text-overflow: ellipsis;
Expand Down
6 changes: 4 additions & 2 deletions packages/trace-viewer/src/ui/networkResourceDetails.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export const NetworkResourceDetails: React.FunctionComponent<{
const routeStatus = formatRouteStatus(resource);
const requestContentTypeHeader = resource.request.headers.find(q => q.name === 'Content-Type');
const requestContentType = requestContentTypeHeader ? requestContentTypeHeader.value : '';
const resourceName = resource.request.url.substring(resource.request.url.lastIndexOf('/') + 1);
const resourceName = resource.request.url.substring(resource.request.url.lastIndexOf('/'));
let contentType = resource.response.content.mimeType;
const charset = contentType.match(/^(.*);\s*charset=.*$/);
if (charset)
Expand All @@ -79,7 +79,7 @@ export const NetworkResourceDetails: React.FunctionComponent<{

const renderTitle = React.useCallback(() => {
return <div className='network-request-title'>
{routeStatus && <div className={'network-request-title-status status-route'}>{routeStatus}</div> }
{routeStatus && <div className={`network-request-title-status status-route ${routeStatus}`}>{routeStatus}</div> }
{resource.response._failureText && <div className={'network-request-title-status status-failure'}>{resource.response._failureText}</div>}
{!resource.response._failureText && <div className={'network-request-title-status ' + formatStatus(resource.response.status)}>{resource.response.status}</div>}
<div className='network-request-title-status'>{resource.request.method}</div>
Expand Down Expand Up @@ -147,5 +147,7 @@ function formatRouteStatus(request: Entry): string {
return 'continued';
if (request._wasFulfilled)
return 'fulfilled';
if (request._apiRequest)
return 'api';
return '';
}
1 change: 1 addition & 0 deletions packages/trace/src/har.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export type Entry = {
_wasAborted?: boolean;
_wasFulfilled?: boolean;
_wasContinued?: boolean;
_apiRequest?: boolean;
};

export type Request = {
Expand Down
10 changes: 5 additions & 5 deletions tests/library/trace-viewer.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,9 @@ test('should have network requests', async ({ showTraceViewer }) => {
const traceViewer = await showTraceViewer([traceFile]);
await traceViewer.selectAction('http://localhost');
await traceViewer.showNetworkTab();
await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/200GETstyle.csstext\/css/]);
await expect(traceViewer.networkRequests).toContainText([/200GETscript.jsapplication\/javascript/]);
await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/200GET\/style.csstext\/css/]);
await expect(traceViewer.networkRequests).toContainText([/200GET\/script.jsapplication\/javascript/]);
});

test('should have network request overrides', async ({ page, server, runAndTrace }) => {
Expand All @@ -228,7 +228,7 @@ test('should have network request overrides', async ({ page, server, runAndTrace
});
await traceViewer.selectAction('http://localhost');
await traceViewer.showNetworkTab();
await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/abort.*style.cssx-unknown/]);
});

Expand All @@ -239,7 +239,7 @@ test('should have network request overrides 2', async ({ page, server, runAndTra
});
await traceViewer.selectAction('http://localhost');
await traceViewer.showNetworkTab();
await expect(traceViewer.networkRequests).toContainText([/200GETframe.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/200GET\/frame.htmltext\/html/]);
await expect(traceViewer.networkRequests).toContainText([/continue.*script.jsapplication\/javascript/]);
});

Expand Down

0 comments on commit 2e327c9

Please sign in to comment.