Skip to content
This repository has been archived by the owner. It is now read-only.

feat(metrics): emit flow events for key performance metrics #4776

Merged
merged 1 commit into from Mar 3, 2017

Conversation

@philbooth
Copy link
Contributor

@philbooth philbooth commented Mar 2, 2017

Fixes mozilla/fxa-activity-metrics#52.

@mozilla/fxa-devs r?

@philbooth philbooth force-pushed the phil/perf-flow-event branch from a3a92ff to 6802202 Mar 2, 2017
@@ -44,6 +44,10 @@ const UTM_PATTERN = /^[\w.%-]+$/;

const IS_DISABLED = config.get('client_metrics').stderr_collector_disabled;

const PERFORMANCE_PROPERTIES = new Set([
'responseStart', 'domInteractive', 'domComplete', 'loadEventEnd'

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Mar 2, 2017
Member

Another performance related event that might be useful, even though it's not an official navigationTiming stat, is loaded. loaded is emit after the first view is visible, meaning it happens after all JS, CSS, HTML is downloaded (unless done using requireOnDemand) and after any XHR requests to determine the user's auth status have completed.

PERFORMANCE_PROPERTIES.forEach(key => {
const timing = navigationTiming[key];
if (timing > 0) {
const offset = timing - navigationTiming.navigationStart;

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Mar 2, 2017
Member

One thing to watch out for, I believe these values can be null or undefined. I have performance timing disabled to give sites one less way to fingerprint me.

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Mar 2, 2017
Member

Also, I think all these values are already offsets, can you verify that?

This comment has been minimized.

@philbooth

philbooth Mar 2, 2017
Author Contributor

One thing to watch out for, I believe these values can be null or undefined.

That's (one of the reasons) why I test timing > 0 above. It's fine that we won't have this data for all flows, I see it more as an additional, secondary datapoint for us to consult when trying to understand certain trends.

I think all these values are already offsets

I copied the subtraction of navigationStart from MDN. Although they're certainly offsets in current Firefox, I don't know if that's always been/will be true across all user agents. I figured the subtraction was there to provide a measure of robustness against implementation differences (including historical ones).

@philbooth philbooth force-pushed the phil/perf-flow-event branch from 6802202 to 3e20c73 Mar 3, 2017
//
// Bear this in mind when looking at the data. The main `flow.performance`
// event represents our best approximation of overall, user-perceived
// performance.

This comment has been minimized.

@philbooth

philbooth Mar 3, 2017
Author Contributor

I'll also add appropriate comments to the flow event docs in the auth server, if/when this gets merged.

@philbooth
Copy link
Contributor Author

@philbooth philbooth commented Mar 3, 2017

@shane-tomlinson, I've re-thought this in light of our conversation in IRC. When you have a moment, please take a look.

@philbooth philbooth force-pushed the phil/perf-flow-event branch from 3e20c73 to 3dd4315 Mar 3, 2017
Copy link
Member

@shane-tomlinson shane-tomlinson left a comment

🚢 this is a great PR @philbooth!

@philbooth philbooth merged commit 33f2cf3 into master Mar 3, 2017
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 98.378%
Details
@philbooth philbooth deleted the phil/perf-flow-event branch Mar 3, 2017
@rfk rfk added this to the FxA-0: quality milestone Mar 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants