Metrics for page resources #26
Conversation
| @@ -45,6 +45,7 @@ Metrics are gathered in the format used by a statsd / graphite / grafana stack - | |||
| * PageChange - When the page loads, or whenever a navigation event occurs, we log a counter for the page visit and a gauge for how long the page load took. The page load is a measure of the time between the navigation event firing, the event loop lagging for over 25ms and finally the event loop freeing up for 125ms. Basically we wait for the page to get busy, then we wait for it to finish being busy. | |||
| * PageLoad - We use `window.performance` to get details of the initial page load. | |||
| * XhrStats - We monkey-patch over `XMLHttpRequest` in order to identify every AJAX request, the destination, the transition between states, final response size and timing. | |||
| * PageResources - `connectEnd`, `connectStart`, `domainLookupEnd`, `domainLookupStart`, `duration`, `fetchStart`, `redirectEnd`, `redirectStart`, `requestStart`, `responseEnd`, `responseStart`, `secureConnectionStart`, `startTime`, `workerStart`. | |||
There was a problem hiding this comment.
Seems inconsistent to list these for pageLoad but not the others
|
I agree that the 4.6KB is worth it but might be nice to have a build time config that can exclude them for custom builds. I've created #27 to consider that |
|
This work only adds |
|
Sorry that's what I meant, still reeling from yesterdays rabbit hunt |
| } | ||
|
|
||
| pageResources.findRule = function (entry) { | ||
| var config = window.barometer.resources |
There was a problem hiding this comment.
I'm thinking about this for bandwidthStats:
if (barometer.bandwidth === false) return
var config = barometer.bandwidth || {}
this will give null or undefined values a default config
There was a problem hiding this comment.
I was going with the idea that you don't define it if you don't want it. If you do define it, it should be an array of properties to match on and the metrics we want to capture. The default value should be [] in the code you've highlighted - I'll get to it on Sunday ;)
There was a problem hiding this comment.
This does seem like one that should be disabled by default, maybe bandwidth as well, not sure?
There was a problem hiding this comment.
Ah sorry, I've just re-read this now I'm fully behind a computer, I think we were talking about different things. I think you're right, bandwidth should be an opt-in, and maybe locked behind an onPageChanged event to limit any impact on end user experience. The other modules in this project check for appropriate browser support before they kick off - a similar approach can be taken for config. If there's no config, the module simply won't initialise.
|
I've gone to town on the documentation @msaspence |
|
I've added a new test and it's passing in all environments 👍 |
| pageResources.track = function (entry, metrics) { | ||
| var parts = entry.name.replace(/https?:\/\//i, '').split('/') | ||
| var prefix = 'resources.' + entry.entryType + '.' | ||
| prefix += parts.shift().replace(/[^a-z0-9_]/gi, '_') |
There was a problem hiding this comment.
Might be worth setting /[^a-z0-9_]/gi as a var?
There was a problem hiding this comment.
The less code the better ;) Space is precious! If the code became unreadable, then fair enough. For now, I think it's good enough.
|
Minor comment, overall looks great 👍 |
|
👍 |
This PR enables a config driven approach to gathering page resource metrics. Resolves #3
The functionality is best read on the
README.md, the TL;DR is that the feature is controlled via:The production build size currently increases from
2.31KBto2.79KB. Not too bad considering what we get.