Collect request stats #9633
Collect request stats #9633
Conversation
| let requestTypeEntries = {}; | ||
| type RequestTypes = Array<$Keys<typeof requestTypes>>; | ||
|
|
||
| for (let key of (Object.keys(requestTypes): RequestTypes)) { |
There was a problem hiding this comment.
/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
There was a problem hiding this comment.
True about top of the file. However we used keys because the flow types to get entries happy was super ugly.
packages/core/core/src/Parcel.js
Outdated
| @@ -385,6 +385,7 @@ export default class Parcel { | |||
| }, | |||
| }; | |||
|
|
|||
| this.#requestTracker.flushStats(); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
What's the difference between this and tracing? They seem to have similar goals. Maybe the tracer could track requests in a similar way? |
| @@ -66,14 +66,14 @@ export default (new Reporter({ | |||
| case 'buildSuccess': | |||
| case 'buildFailure': | |||
| nullthrows(tracer).flush(); | |||
| tracer = null; | |||
There was a problem hiding this comment.
@marcins This actually fixes a race condition that showed up in the tests where it's possible that the tracer isn't cleared quick enough.
|
@devongovett I believe tracing has a performance overhead and is used for in depth analysis of one-off builds. This is something we want to measure across all builds. The |
↪️ Pull Request
In order to get a picture of the cache performance in our application, we want to get the request stats and the number of visited
asset_groups out of each Parcel build.These get
loggered, to be read by a custom reporter.