[Metrics] Histogram support for user-timing-based metrics

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rnicoletti, Assigned: rnicoletti)

Tracking

unspecified
FxOS-S7 (18Sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

4 years ago
Bug 1177226 added support for app startup and app memory metrics based on user-timing events. In order to create histograms for these metrics, the supported user-timing-based histograms need to be added to Histograms.json. The supported user-timing-based histograms are:

app_startup_time_contentInteractive
app_startup_time_navigationInteractive
app_startup_time_navigationLoaded
app_startup_time_visuallyLoaded
app_startup_time_fullyLoaded
app_startup_time_mediaEnumerated
app_startup_time_scanEnd

app_memory_contentInteractive
app_memory_navigationInteractive
app_memory_navigationLoaded
app_memory_visuallyLoaded
app_memory_fullyLoaded
app_memory_mediaEnumerated
app_memory_scanEnd
Assignee

Updated

4 years ago
Assignee

Updated

4 years ago
Assignee: nobody → rnicoletti
Status: NEW → ASSIGNED
Assignee

Comment 1

4 years ago
Hi Jan, 

This patch is mostly about adding to Historgrams.json the definitions of the histograms for the metrics that are user-timing-based.

Also, I've added a condition to validate when we get a user-timing event that the performance mark is supported in terms of HUD telemetry. We can't anticipate all possible performance marks that will be used so we're initially supporting commonly used performance marks.

Additionally, I thought it would be useful to use a pref to get the supported performance marks so that developers can locally test using any performance mark.
Attachment #8654439 - Flags: feedback?(janx)
It's recently been restated on dev-platform that *all* new Telemetry probes need review from a data collection peer, as mentioned in the red box at the top of the MDN page[1].

I don't believe everyone realized this in the past... anyway, just thought it would be good to be aware of.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
Comment on attachment 8654439 [details] [diff] [review]
histogram support for user-timing-based metrics

Review of attachment 8654439 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your patch, I added a few comments below. Please also mind trailing spaces and indentation conventions.

Additionally, Ryan mentioned in comment 2 that new telemetry probes need a review from a data collection peer. Can you please make sure that all Advanced Telemetry probes were subjected to review by either :bsmedberg, :vladan, or :ally?

Flagging a data collection peer on this as well: Firefox OS Advanced Telemetry would like to send User Timing events (http://www.w3.org/TR/user-timing/) as histograms, is that OK?

::: b2g/chrome/content/devtools/hud.js
@@ +67,5 @@
> +        'visuallyLoaded',
> +        'fullyLoaded',
> +        'mediaEnumerated',
> +        'scanEnd'
> +  ],

This list strikes me as a performanceEntriesWatcher-only thing, so please move it down into that watcher. Also, once it's inside the watcher, maybe calling it simply `_supported` will be explicit enough?

@@ +125,5 @@
> +    try {
> +      let prefString = Services.prefs.getCharPref("devtools.telemetry.supported_performance_marks");
> +      this._supportedPerformanceMarks = prefString.split(',');
> +        
> +    } catch(e) {}

Please move this into `performanceEntriesWatcher.init()`.

Also, this file never checks for prefs directly. Please create a dedicated setting for that pref in https://dxr.mozilla.org/mozilla-central/source/b2g/chrome/content/settings.js#540, and then only watch the setting here.

@@ +197,5 @@
>      }
> +  },
> +
> +  isMarkSupported(name) {
> +    return developerHUD._supportedPerformanceMarks.indexOf(name) !== -1; 

Nit: Please only create helper function when you can't access the list directly or additional logic is needed. In this case, you can just access `this._supported[name]` in the performanceEntriesWatcher.

@@ +765,5 @@
>            let appStartPos = name.indexOf('@app') + CHARS_UNTIL_APP_NAME;
>            let length = (name.indexOf('.') - appStartPos);
>            this._appLaunchName = name.substr(appStartPos, length);
>            this._appLaunchStartTime = epoch;
> +        } else if (developerHUD.isMarkSupported(name)) {

As mentioned above, please change this to `else if (this._supported[name])` (or `this._supportedMark` if you feel the name should be more explicit).

::: toolkit/components/telemetry/Histograms.json
@@ +8835,5 @@
> +  "DEVTOOLS_HUD_APP_MEMORY_CONTENTINTERACTIVE": {
> +    "expires_in_version": "never",
> +    "kind": "linear",
> +    "keyed": "true",
> +    "description": "The uss memory consumed by an application at the time of the 'contentInteractive' performance mark, keyed by appName.",

Nit: Please change "USS" to uppercase in all descriptions.
Attachment #8654439 - Flags: review?(benjamin)
Attachment #8654439 - Flags: feedback?(janx)
Attachment #8654439 - Flags: feedback+

Comment 4

4 years ago
I am the data steward for Firefox. I suspect that this data is not relevant to Firefox at all, and you need to get approval from the Firefox OS data steward, Jonas Sicking. Can you please verify that these metrics aren't relevant to Firefox?
Flags: needinfo?(jonas)
Assignee

Comment 5

4 years ago
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> I am the data steward for Firefox. I suspect that this data is not relevant
> to Firefox at all, and you need to get approval from the Firefox OS data
> steward, Jonas Sicking. Can you please verify that these metrics aren't
> relevant to Firefox?

It is true that these metrics are specific to Firefox OS; they are not relevant to Firefox.
I take it this is data that we would collect for Gaia apps and send to mozilla? If so Ravi is the data steward for this part (I only cover the platform pieces that expose user data to 3rd party websites that the user visit).
Flags: needinfo?(jonas)
Comment on attachment 8654439 [details] [diff] [review]
histogram support for user-timing-based metrics

Ravi, for Firefox OS Advanced Telemetry we would like to send User Timing events (http://www.w3.org/TR/user-timing/) as histograms, is that OK?
Attachment #8654439 - Flags: review?(benjamin) → review?(rdandu)
Assignee

Comment 8

4 years ago
Hi Jan, I've updated the patch to address your review comments.
Attachment #8654439 - Attachment is obsolete: true
Attachment #8654439 - Flags: review?(rdandu)
Attachment #8656058 - Flags: review?(janx)
Comment on attachment 8656058 [details] [diff] [review]
histogram support for user-timing-based metrics

(Still waiting for Ravi to verify that it's OK for us to send User Timing events as telemetry histograms, see comment 7).
Attachment #8656058 - Flags: review?(rdandu)
Comment on attachment 8656058 [details] [diff] [review]
histogram support for user-timing-based metrics

Review of attachment 8656058 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good for my part, thank you for addressing my earlier comments. I added a few more.

::: b2g/chrome/content/devtools/hud.js
@@ +765,5 @@
>    _fronts: new Map(),
>    _appLaunchName: null,
>    _appLaunchStartTime: null,
> +  _supported: [
> +        'contentInteractive',

Nit: Here and below, please use only 2-space indentation.

@@ +803,4 @@
>  
>          // FIXME There is a potential race condition that can result
>          // in some performance entries being disregarded. See bug 1189942.
>          if (name.indexOf('appLaunch') != -1) {

Nit: Please change this pre-existing `!=` to a strict comparison: `!==`.

@@ +807,5 @@
>            let appStartPos = name.indexOf('@app') + CHARS_UNTIL_APP_NAME;
>            let length = (name.indexOf('.') - appStartPos);
>            this._appLaunchName = name.substr(appStartPos, length);
>            this._appLaunchStartTime = epoch;
> +        } else if (this._supported.indexOf(name) !== -1) {

Nit: Please use bail-out style in this function:

> if (detail.type !== 'mark') {
>   return;
> }
> // let name = ...
> if (name.indexOf('appLaunch') !== -1) {
>   // let appStartPos = ...
>   return;
> }
> if (this._supported.indexOf(name) === -1) {
>   return;
> }
> // let origin = ...

::: b2g/chrome/content/settings.js
@@ +572,5 @@
>    'devtools.eventlooplag.threshold': 100,
>    'devtools.remote.wifi.visible': {
>      resetToPref: true
>    },
> +  'devtools.telemetry.supported_performance_marks': {

Thanks for mapping the pref to a setting!

Nit: Please also add the pref to modules/libpref/init/all.js along with its default value. This will make it show up in prefs/settings tools, and it will be more obvious how to use it.
Attachment #8656058 - Flags: review?(janx) → feedback+
Assignee

Comment 11

4 years ago
Hi Jan, thanks for your comments. This patch should address all of them.
Attachment #8657381 - Flags: review?(janx)
Comment on attachment 8657381 [details] [diff] [review]
histogram support for user-timing-based metrics

Review of attachment 8657381 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there! Sorry for the last-minute nits.

::: b2g/chrome/content/devtools/hud.js
@@ +777,5 @@
>  
>    init(client) {
>      this._client = client;
> +
> +    SettingsListener.observe('devtools.telemetry.supported_performance_marks', this._supported, supported => {

Nit: You're giving `SettingsListener.observe()` an Array as default setting value (`this._supported`) while it's actually supposed to be a String (`this._supported.join(',')`).

Also, to make this line shorter, please create local shorthand variables, e.g.:

    let setting = 'devtools.telemetry.supported_performance_marks';
    let value = this._supported.join(',');
    SettingsListener.observe(setting, value, supported => { ... });

@@ +814,4 @@
>          let CHARS_UNTIL_APP_NAME = 7; // '@app://'
> +        let appStartPos = name.indexOf('@app') + CHARS_UNTIL_APP_NAME;
> +        let length = (name.indexOf('.') - appStartPos);
> +        this._appLaunchName = name.substr(appStartPos, length);

Nit: As below, please use `slice(position,position)` here instead of `substr(position,length)`.

@@ +826,3 @@
>  
> +      let origin = detail.origin;
> +      origin = origin.substr(0, origin.indexOf('.'));

Nit: In this particular case, substr() is equivalent to slice(), but since you're using an indexOf I'd prefer `origin.slice(...)` here (substr() asks for (position,length) while slice() asks for (position,position), and indexOf is a position).

::: modules/libpref/init/all.js
@@ +890,5 @@
>  // GCLI commands directory
>  pref("devtools.commands.dir", "");
>  
> +// Allows setting the performance marks for which telemetry metrics will be recorded.
> +pref("devtools.telemetry.supported_performance_marks", "");

Nit: This file is supposed to set the default value of prefs. Maybe you could set it to the current default value, i.e.

"contentInteractive,navigationInteractive,navigationLoaded,visuallyLoaded,fullyLoaded,mediaEnumerated,scanEnd"

? Otherwise, I believe your `resetToPref:true` from earlier might replace `performanceEntriesWatcher._supported` by an empty Array.
Attachment #8657381 - Flags: review?(janx) → feedback+
Assignee

Comment 13

4 years ago
Ok, Jan, here's another patch for you. I do appreciate your comments, the code is becoming cleaner and cleaner.
Attachment #8657381 - Attachment is obsolete: true
Attachment #8658377 - Flags: review?(janx)

Comment 14

4 years ago
(In reply to Jan Keromnes [:janx] from comment #7)
> histogram support for user-timing-based metrics
> 
> Ravi, for Firefox OS Advanced Telemetry we would like to send User Timing
> events (http://www.w3.org/TR/user-timing/) as histograms, is that OK?

Hi Jan,
For Foxfooding builds (under the foxfooding build flag), it is ok to add.

For Commercial builds (2.5), the bar is much higher:
1) How much data is added by adding the user-timing?  
2) Value: is the value of adding this that the high precision timestamp enables us to get accurate performance numbers?
Flags: needinfo?(janx)
Thank you for having a look Ravi!

> 1) How much data is added by adding the user-timing?

Only a fixed list of user-timing events are being sent for each app:

    contentInteractive
    navigationInteractive
    navigationLoaded
    visuallyLoaded
    fullyLoaded
    mediaEnumerated
    scanEnd

I believe these should only be fired once every time an app is launched on the device.

> 2) Value: is the value of adding this that the high precision timestamp
> enables us to get accurate performance numbers?

The events will allow us to better understand how long it takes for an app to go through its different stages of start-up, and see where the bottlenecks are, in real world usage.

Russ and Tamara, would you like to give more complete answers to Ravi's questions?
Flags: needinfo?(thills)
Flags: needinfo?(rnicoletti)
Flags: needinfo?(janx)
Comment on attachment 8658377 [details] [diff] [review]
histogram support for user-timing-based metrics

Review of attachment 8658377 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Russ! All good now, r+ with a small nit.

::: b2g/chrome/content/devtools/hud.js
@@ +780,5 @@
> +    let setting = 'devtools.telemetry.supported_performance_marks';
> +    let defaultValue = this._supported.join(',');
> +
> +    SettingsListener.observe(setting, defaultValue, supported => {
> +      let value = supported || defaultValue;

Nit: Imagine that a user wants to stop sending any user-timing event (not likely, I agree) by setting the pref value to "". Since "" is falsy, this code will override it with `defaultValue`, which is not what the user intended. I'm not sure it's useful to do anything to `supported` before splitting it into `this._supported`.
Attachment #8658377 - Flags: review?(janx) → review+
Assignee

Comment 17

4 years ago
(In reply to Jan Keromnes [:janx] from comment #15)
> Thank you for having a look Ravi!
> 
> > 1) How much data is added by adding the user-timing?
> 
> Only a fixed list of user-timing events are being sent for each app:
> 
>     contentInteractive
>     navigationInteractive
>     navigationLoaded
>     visuallyLoaded
>     fullyLoaded
>     mediaEnumerated
>     scanEnd
> 
> I believe these should only be fired once every time an app is launched on
> the device.

Correct, data is sent only when an app is cold-launched. No data is sent when an app comes from the background to the foreground. The data being recorded is not the high-precision timestamp. It is the duration (in ms) between when the app was launched and the performance mark(s).

> 
> > 2) Value: is the value of adding this that the high precision timestamp
> > enables us to get accurate performance numbers?
> 
> The events will allow us to better understand how long it takes for an app
> to go through its different stages of start-up, and see where the
> bottlenecks are, in real world usage.
> 
Jan has explained the benefits perfectly. As I mentioned above, we're not interested in the high-resolution performance mark data; we're using the performance marks to determine how long it takes the application to reach the various stages of startup. Raptor is using the same logic to determine application startup times in fixed, testing conditions. We'll be collecting the real world data.

> Russ and Tamara, would you like to give more complete answers to Ravi's
> questions?
Flags: needinfo?(rnicoletti)
Assignee

Comment 18

4 years ago
Ravi, comment 17 has my additional explanations to your questions from comment 14.
Flags: needinfo?(rdandu)

Comment 19

4 years ago
Hi Russ, JAn, thanks for the data. Lets add this under the "Enhanced" bucket of collection. More details at https://docs.google.com/document/d/1iEDhJQS0ymaWdOe1OuVMuB31qTDl9tDZekqTsmKS37E
Flags: needinfo?(rdandu)

Updated

4 years ago
Attachment #8656058 - Flags: review?(rdandu) → review+
Russ, the patch seems to have landed without a fix to my nit from comment 16. Could you please file a follow-up bug to address it, or at least drive-by fix it in an upcoming HUD patch of yours?
Flags: needinfo?(rnicoletti)
Also, can you please make sure that Advanced Telemetry respects the "Enhanced" data collection choice from the User? (See Ravi's link: selecting only "Basic" in the FTE should prevent Advanced Telemetry from working).
https://hg.mozilla.org/mozilla-central/rev/96d8527b5995
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S7 (18Sep)
Assignee

Comment 25

4 years ago
(In reply to Jan Keromnes [:janx] from comment #22)
> Russ, the patch seems to have landed without a fix to my nit from comment
> 16. Could you please file a follow-up bug to address it, or at least
> drive-by fix it in an upcoming HUD patch of yours?

This was an oversight, sorry about that. I'm not currently working on a HUD patch so I will create a follow-up bug to address it.
Flags: needinfo?(rnicoletti)
Assignee

Comment 26

4 years ago
(In reply to Jan Keromnes [:janx] from comment #23)
> Also, can you please make sure that Advanced Telemetry respects the
> "Enhanced" data collection choice from the User? (See Ravi's link: selecting
> only "Basic" in the FTE should prevent Advanced Telemetry from working).

Yes, we will take care of this as part of bug 1181295.
Flags: needinfo?(thills)
You need to log in before you can comment on or make changes to this bug.