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

Jump to bottom

Collect request stats #9633

Merged
merged 11 commits into from
Apr 12, 2024
Merged

Collect request stats #9633

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
60a99fb
Add an initial request stats implementation
benjervis Apr 9, 2024
2616b83
Update function name
benjervis Apr 10, 2024
0fcff55
Collect stats on visited asset_groups
benjervis Apr 10, 2024
adda334
Fix right associations
benjervis Apr 10, 2024
d97c10f
Merge branch 'v2' into req-stats
benjervis Apr 10, 2024
82388b7
Merge branch 'v2' into req-stats
benjervis Apr 11, 2024
797e11d
Merge branch 'v2' into req-stats
benjervis Apr 11, 2024
3086ef2
Fix tracer race condition
mattcompiles Apr 11, 2024
06a50e3
Move the request stats to the build success event
benjervis Apr 12, 2024
3277f54
Add request stats to build failure
mattcompiles Apr 12, 2024
170f528
Merge branch 'v2' into req-stats
mattcompiles Apr 12, 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
1 change: 1 addition & 0 deletions packages/core/core/src/Parcel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ export default class Parcel {
},
};

this.#requestTracker.flushStats();
Copy link
Contributor

@MonicaOlejniczak MonicaOlejniczak Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know if we want to go down this rabbit hole, but it's pretty common for success events to store stats regarding the build. Instead of having flushStats, you could add the stats directly to build success under a new key and then get the reporter to log it.

That way the RequestTracker does not have to care about how to report the info, it just gives it to whatever wants to consume it. This is similar to what @marcins noted before, but can be extended to other stats we want to measure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree this would be nicer. We opted not to do it that way to avoid adding Parcel public API that would be a breaking change to remove or modify in future. Thoughts?

Copy link
Contributor

@MonicaOlejniczak MonicaOlejniczak Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As an aside, I'm looking at adding high-level (and potentially more granular) perf metrics and am starting to explore options. Having a stats object or something similar to that would be useful.

In regards to the public API, you could name the field as unstable_stats to make no guarantees to consumers. Or in a similar fashion, expose that there is a stats object but type it as a record with any value. That way, we are saying we will provide it but make no guarantees about what's in there. We'd probably need something internal to give us types we know about to make logs or w/e on them, but it doesn't need to be exposed.

await this.#reporterRunner.report(event);
await this.#requestTracker.runRequest(
createValidationRequest({optionsRef: this.#optionsRef, assetRequests}),
Expand Down
34 changes: 34 additions & 0 deletions packages/core/core/src/RequestTracker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,7 @@ export default class RequestTracker {
farm: WorkerFarm;
options: ParcelOptions;
signal: ?AbortSignal;
stats: Map<RequestType, number> = new Map();

constructor({
graph,
Expand Down Expand Up @@ -1205,6 +1206,9 @@ export default class RequestTracker {

try {
let node = this.graph.getRequestNode(requestNodeId);

this.stats.set(request.type, (this.stats.get(request.type) ?? 0) + 1);

let result = await request.run({
input: request.input,
api,
Expand All @@ -1227,6 +1231,36 @@ export default class RequestTracker {
}
}

flushStats() {
let requestTypeEntries = {};
type RequestTypes = Array<$Keys<typeof requestTypes>>;

for (let key of (Object.keys(requestTypes): RequestTypes)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

/nit you could just create this mapping at the top of the file so it's done once, and it could be more idiomatic using Object.entries

Copy link
Contributor

Choose a reason for hiding this comment

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

True about top of the file. However we used keys because the flow types to get entries happy was super ugly.

requestTypeEntries[requestTypes[key]] = key;
}

let formattedStats = {};
let messageLines = ['Request stats'];

for (let [requestType, count] of this.stats.entries()) {
let requestTypeName = requestTypeEntries[requestType];
formattedStats[requestTypeName] = count;
messageLines.push(`${requestTypeName}\t\t${count}`);
}

let message = messageLines.join('\n');

logger.verbose({
origin: '@parcel/core',
message,
meta: {
requestStats: formattedStats,
benjervis marked this conversation as resolved.
Show resolved Hide resolved
},
});

this.stats = new Map();
}

createAPI<TResult>(
requestId: NodeId,
previousInvalidations: Array<RequestInvalidation>,
Expand Down
15 changes: 15 additions & 0 deletions packages/core/core/src/requests/AssetGraphRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import type {StaticRunOpts, RunAPI} from '../RequestTracker';
import type {EntryResult} from './EntryRequest';
import type {PathRequestInput} from './PathRequest';
import type {Diagnostic} from '@parcel/diagnostic';
import logger from '@parcel/logger';

import invariant from 'assert';
import nullthrows from 'nullthrows';
Expand Down Expand Up @@ -200,7 +201,9 @@ export class AssetGraphBuilder {
'A root node is required to traverse',
);

let visitedAssetGroups = new Set();
let visited = new Set([rootNodeId]);

const visit = (nodeId: NodeId) => {
if (errors.length > 0) {
return;
Expand All @@ -223,6 +226,10 @@ export class AssetGraphBuilder {
(!visited.has(childNodeId) || child.hasDeferred) &&
this.shouldVisitChild(nodeId, childNodeId)
) {
if (child.type === 'asset_group') {
visitedAssetGroups.add(childNodeId);
}

visited.add(childNodeId);
visit(childNodeId);
}
Expand All @@ -232,6 +239,14 @@ export class AssetGraphBuilder {
visit(rootNodeId);
await this.queue.run();

logger.verbose({
origin: '@parcel/core',
message: 'Asset graph walked',
meta: {
visitedAssetGroupsCount: visitedAssetGroups.size,
},
});

if (this.prevChangedAssetsPropagation) {
// Add any previously seen Assets that have not been propagated yet to
// 'this.changedAssetsPropagation', but only if they still remain in the graph
Expand Down
Loading