1166295 - [Metrics] Record Memory Usage on a per app basis

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Tamara Hills [:thills]]

We need to record memory usage on a per app basis for architecture validation.  This will be stored as a histogram and eventually delivered as a ping to the Telemetry Server.

Comment 1

[Russ Nicoletti [:russn]]

Attachment: bug-1166295.diff

Hi Jan,

Can you take a look at this patch and give me your feedback on whether you think this is a reasonable approach to recording telemetry data for application memory? Thanks.

Comment 2

[Jan Keromnes [:janx]]

Comment on attachment 8607929 [details] [diff] [review]
bug-1166295.diff

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

Overall this approach looks good! However, I'd prefer something more integrated:

- The `Target` constructor could create a `this.histograms = new Map()` just after `this.metrics = new Map()`.
- `Target.prototype.register` could have an optional `histogram` parameter like `{suffix: '_APP_MEMORY', expiration: 'never', type: 1, min: 1, max: 100, buckets: 100}`.
- If `histogram` is defined, `register` would add a new histogram with `this.histograms.set(metric, Services.telemetry.newHistogram(...))`.
- `Target.prototype._send` would check for `this.histograms.get(metric.name)`, and if defined, add the value to it.
- For every metric you want to track, find the watcher that calls `target.register` on it, and add the optional `histogram` parameter there.

For example, the patch could look like this:

--- a/b2g/chrome/content/devtools/hud.js
+++ b/b2g/chrome/content/devtools/hud.js
@@ -30,6 +30,7 @@ XPCOMUtils.defineLazyGetter(this, 'MemoryFront', function() {
 });
 
 Cu.import('resource://gre/modules/Frames.jsm');
+Cu.import('resource://gre/modules/Services.jsm');
 
 /**
  * The Developer HUD is an on-device developer tool that displays widgets,
@@ -43,6 +44,7 @@ let developerHUD = {
   _conn: null,
   _watchers: [],
   _logging: true,
+  _telemetry: true,
 
   /**
    * This method registers a metric watcher that will watch one or more metrics
@@ -90,6 +92,9 @@ developerHUD.init
     SettingsListener.observe('hud.logging', this._logging, enabled => {
       this._logging = enabled;
     });
+
+    // TODO get telemetry pref (TBD) to determine if telemetry data should be recorded
+    this._telemetry = true;
   },
 
   uninit: function dwp_uninit() {
@@ -172,6 +177,7 @@ function Target(frame, actor) {
   this.frame = frame;
   this.actor = actor;
   this.metrics = new Map();
+  this.histograms = new Map();
 }
 
 Target.prototype = {
@@ -179,8 +185,23 @@ Target.prototype = {
   /**
    * Register a metric that can later be updated. Does not update the front-end.
    */
-  register: function target_register(metric) {
+  register: function target_register(metric, histogram) {
     this.metrics.set(metric, 0);
+    if (histogram) {
+      // Get app name from manifest
+      let manifest = this.frame.appManifestUrl;
+      let nameStart = manifest.indexOf('/') + 2;
+      let nameEnd = manifest.indexOf('.', nameStart);
+      let name = manifest.substring(nameStart, nameEnd);
+
+      this.histograms.set(metric, Services.telemetry.newHistogram(
+        name + '_' + histogram.suffix,
+        histogram.expiration,
+        histogram.type,
+        histogram.min,
+        histogram.max,
+        histogram.buckets
+      ));
 
   uninit: function dwp_uninit() {
@@ -256,6 +277,14 @@ Target.prototype._send =
       frame = getContentWindow();
     }
 
+    let histogram = this.histograms.get(data.metric.name);
+    if (histogram && developerHUD._telemetry) {
+      histogram.add(data.metric.value);
+
+      let snapshot = histogram.snapshot()
+      // TODO Something with `snapshot`.
+    }
+
     shell.sendEvent(frame, 'developer-hud-update', Cu.cloneInto(data, frame));
   }
 
@@ -586,7 +615,15 @@ let memoryWatcher = {
   },
 
   trackTarget: function mw_trackTarget(target) {
-    target.register('uss');
+    let ussHistogram = {
+      suffix: 'APP_MEMORY',
+      expiration: 'never',
+      type: 1, // linear
+      min: 1   * 1024 * 1024,
+      max: 100 * 1024 * 1024,
+      buckets: 100
+    }
+    target.register('uss', ussHistogram);
     target.register('memory');
     this._fronts.set(target, MemoryFront(this._client, target.actor));
     if (this._active) {

Comment 3

[Jan Keromnes [:janx]]

Hi Russ, any plans for moving this forward? If you prefer, I can take over the patch and let Vivien review it. It would allow easy per-app-frame Telemetry reporting when the HUD is enabled (for per-app USS, reflows, jank, errors, etc).

Comment 4

[Russ Nicoletti [:russn]]

Attachment: hud.telemetry.diff

I have been moving it forward, although I couldn't get your patch to work. In any event, we're no longer using the concept of a histogram category so my changes are somewhat different now. Attached is my latest patch. The changes include:

* generically sending "advanced telemetry" events for existing hud data being watched
* sending the category of security warnings/errors as opposed to only the count, in the case of advanced telemetry
* creating and sending advanced telemetry events based on telemetry console log entries (see "Console log telemetry data" here: https://docs.google.com/document/d/1IDBRiftj_PTRE-Yu5o4EYxYMRlgN3bGutGlbCu5TgLk)

I'm assigning this to myself for now.

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8616862 [details] [diff] [review]
hud.telemetry.diff

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

Thanks for the new patch.

I am a bit worried by its approach, because it makes HUD widgets and telemetry mutually exclusive. I see telemetry as just another consumer of HUD data, and telemetry / widgets should be able to work alongside each other. However, when you enable advanced telemetry, no "developer-hud-update" events will be sent or logged, breaking listeners for that event (e.g. the widgets) and consumers of HUD logs (e.g. FxOS Remote Developer HUD).

Please add a function the the "Target" prototype called something like "_sendTelementryEvent", and then use it for any telemetry calls. If it begins by checking if "developerHUD._telemetry" is true, you could call it directly from "Target._send()", and also from any telemetry-specific cases in the watchers (like the security error categories and the advanced log events).

::: b2g/chrome/content/devtools/hud.js
@@ +42,5 @@
>    _client: null,
>    _conn: null,
>    _watchers: [],
>    _logging: true,
> +  _advancedTelemetry: true, // TODO observe advanced telemetry setting

Nit: I'd like to keep names as short and simple as possible. Since "advancedTelemetry" is the only "telemetry" the HUD knows about, please call the boolean attribute "_telemetry".

@@ +43,5 @@
>    _conn: null,
>    _watchers: [],
>    _logging: true,
> +  _advancedTelemetry: true, // TODO observe advanced telemetry setting
> +  _eventToSend: null,

Please don't use "_eventToSend". Instead, in "Target._send()", please send a separate telemetry event if "developerHUD._telemetry" is true.

@@ +64,5 @@
>  
> +    this._eventToSend =
> +      (this._advancedTelemetry) ?
> +        'advanced-telemetry-update' :
> +        'developer-hud-update';

Same as above, please remove "_eventToSend".

@@ +194,5 @@
>     * Modify one of a target's metrics, and send out an event to notify relevant
>     * parties (e.g. the developer HUD, automated tests, etc).
>     */
>    update: function target_update(metric, message) {
> +

Nit: No need for this new line.

@@ +217,5 @@
> +        var appName = this.frame.appManifestURL.substring(appNameIndexStart, appNameIndexEnd);
> +        metric.appName = appName;
> +      }
> +      data.metric = metric;
> +    }

Please don't change the "Target.update()" logic, this is not the right place for it.

Please move this "if" block over to "Target._send()" (or to the new "Target._sendTelemetryEvent()" function I'm suggesting). You will be able to use "data.manifest" to extract the "appName". Also, "appName" will remain constant across every metric within a Target's lifetime, so you could save it in "this" instead of in every "metric" object.

@@ +233,5 @@
> +        for (let name of metrics.keys()) {
> +          data.metrics.push({name: name, value: metrics.get(name)});
> +        }
> +      }
> +    

Nit: Trailing spaces.

@@ +278,5 @@
>        frame = getContentWindow();
>      }
>  
> +    console.log('RNRNRN sending ' + developerHUD._eventToSend + ' with this data: ' + JSON.stringify(data));
> +    shell.sendEvent(frame, developerHUD._eventToSend, Cu.cloneInto(data, frame));

Please always send a 'developer-hud-update' event, and additionally, if "developerHUD._telemetry" is true, send a separate telemetry event to whoever will be listening for it.

Even better, you could write a separate "_sendTelemetryEvent" function inside the "Target" prototype for all advanced telemetry, and call it from here.

@@ +364,5 @@
>    consoleListener: function cw_consoleListener(type, packet) {
>      let target = this._targets.get(packet.from);
>      let metric = {};
>      let output = '';
> +    let incrementable = true;

Nit: I don't like this "incrementable" boolean. Please remove it, and use any telemetry calls in parallel to the regular HUD logic.

@@ +387,5 @@
> +          if (developerHUD._advancedTelemetry) {
> +            metric.value = pageError.category;
> +
> +            // Indicate that this metric is not "incrementable"
> +            incrementable = false;

Nit: Since you'll be removing "incrementable", please replace this line with a call to "sendTelemetryEvent()" with custom data (i.e. including "pageError.category"). Then, let the rest of the consoleWatcher code continue as before. If necessary, you can use a different metric name here for the telemetry call, and in "sendTelemetryEvent()" ignore any metric called "security" to avoid duplicate information.

@@ +415,5 @@
> +
> +            // The processing of advanced telemetry log entries isn't
> +            // associated with a 'watched' setting; its processing
> +            // is handled entirely by the above function, no need to
> +            // continue.

Maybe "info" will be tracked by the HUD at a later time. Please call "_sendTelemetryEvent" here but let the code continue (today, "info" won't be watched so the function will return anyway).

@@ +416,5 @@
> +            // The processing of advanced telemetry log entries isn't
> +            // associated with a 'watched' setting; its processing
> +            // is handled entirely by the above function, no need to
> +            // continue.
> +            return; 

Nit: Trailing space.

@@ +449,5 @@
> +      target.bump(metric, output);
> +    }
> +    else {
> +      // Otherwise, just update it
> +      target.update(metric, output);

Please remove this if/else block. Since you'll have a separate code path to send security error categories to telemetry, you can let the developer HUD bump its security error counter normally.

@@ +490,5 @@
> +
> +      if (telemetryData[TELEMETRY_IDENTIFIER_IDX] != 'telemetry') {
> +        return;
> +      }
> +    

Nit: Trailing spaces.

@@ +498,5 @@
> +        metric.value = telemetryData[VALUE_IDX];
> +
> +        // The metric's app name, if a 'context' was provided, is the
> +        // specified context appended to the specified app name.
> +        metric.appName = (telemetryData.length === 5) ? 

Nit: Trailing space.

@@ +501,5 @@
> +        // specified context appended to the specified app name.
> +        metric.appName = (telemetryData.length === 5) ? 
> +          (telemetryData[APP_NAME_IDX] + '-' + telemetryData[CONTEXT_IDX]) :
> +           telemetryData[APP_NAME_IDX];
> +      

Nit: Trailing spaces.

@@ +502,5 @@
> +        metric.appName = (telemetryData.length === 5) ? 
> +          (telemetryData[APP_NAME_IDX] + '-' + telemetryData[CONTEXT_IDX]) :
> +           telemetryData[APP_NAME_IDX];
> +      
> +        target.update(metric);

Rather than calling "target.update", I think this function should call a "target._sendTelemetryEvent()" function directly.

Comment 6

[Russ Nicoletti [:russn]]

Attachment: rn_6-10_hud.js.diff

Hi Jan. Thanks for your detailed feedback. This patch should be a lot cleaner, more consistent with the existing design, and I believe it addresses your concerns.

Comment 7

[Jan Keromnes [:janx]]

Comment on attachment 8620485 [details] [diff] [review]
rn_6-10_hud.js.diff

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

Thanks Russ! This approach does look a lot cleaner. Sorry for the many nits.

::: b2g/chrome/content/devtools/hud.js
@@ +261,5 @@
>      shell.sendEvent(frame, 'developer-hud-update', Cu.cloneInto(data, frame));
> +
> +    this._sendTelemetryEvent(data.metric);
> +
> +  },

Nit: No need for the empty line at the end of this function.

@@ +268,5 @@
> +    if (!developerHUD._telemetry) {
> +      return;
> +    }
> +
> +    let frame = this.frame;

Nit: In general, please declare such shortcut variables just before using them. In this case, it would be before the appName extraction code line 282 (which by the way should use "frame" instead of "this.frame").

@@ +276,5 @@
> +    if (metric.name === 'security') {
> +      return;
> +    }
> +
> +    let data = {};

Nit: You don't seem to use "data" before the line "data.metric = metric", please move its declaration there. Also, you could directly write "let data = { metric: metric }".

@@ +283,5 @@
> +    // standard HUD metrics) or it can can can be set on the metric object
> +    // (in the case of telemetry log entries).
> +    // If this is the first time getting the app name for this target, get
> +    // it now and store it. In any case, if the app name is set on the
> +    // metric, it takes precedence over the app name corresponding to the target.

Thanks for explaining! This wasn't directly obvious to me.

@@ +297,5 @@
> +
> +    data.metric = metric;
> +
> +    console.log('RNRNRN sending ' + 'telemetry-update-event' + ' with this data: ' + JSON.stringify(metric));
> +    shell.sendEvent(frame, 'telemetry-update-event', Cu.cloneInto(data, frame));

I'm curious why you need to send the "telemetry-update-event" to a particular app frame. We do that for "developer-hud-update"s so that in Gaia[1] we know where to inject the widgets overlay.

Who will be the consumer of "telemetry-update-event"? I imagine you could use the frame information to extract the appName on the listener side, but you're already doing it here.

Also, nit: If the event name isn't set in stone yet, I think it's not necessary to append "-event" to it. What about "telemetry-update"? (Or "advanced-telemetry-update" if you prefer, since it goes out of the HUD code, it might be useful to disambiguate it from other telemetry events).

[1] The Gaia code receiving "developer-hud-update" events: https://github.com/mozilla-b2g/gaia/blob/a80c017c06acee175f911419a2c60c5cb76a37c3/apps/system/js/devtools/developer_hud.js#L18
It uses the fact that the event was sent to a specific "frame" to access its parent AppWindow, in order to append a widget overlay to it: https://github.com/mozilla-b2g/gaia/blob/a80c017c06acee175f911419a2c60c5cb76a37c3/apps/system/js/devtools/developer_hud.js#L51

@@ +400,5 @@
>  
>          if (this._security.indexOf(pageError.category) > -1) {
>            metric.name = 'security';
> +
> +          // Advanced telemetry 

Nit: Trailing whitespace.

@@ +408,5 @@
> +            // security event (which sends only the count).
> +            // Send the 'at-security' event here, then continue the normal
> +            // processing so that the HUD security metric is sent.
> +            let at_metric = {
> +              name: 'at-security', // distinguish from HUD security metric

Nit: It's relatively easy to guess that "at" means "advanced telemetry", but I'd prefer a metric name that reflects its value, rather than its purpose. Would you be willing to change it to something like "security-category"?

@@ +410,5 @@
> +            // processing so that the HUD security metric is sent.
> +            let at_metric = {
> +              name: 'at-security', // distinguish from HUD security metric
> +              value: pageError.category
> +            }; 

Nit: Trailing whitespace.

Comment 8

[Russ Nicoletti [:russn]]

Hi Jan, I have addressed your most recent feedback but I'm unsure how to address the issue of not sending the telemetry event to a particular app frame. I hadn't thought about that, truthfully I just copied that line from where the HUD event is sent. If not being sent to the app frame, what would be the target? I tried 'window' but the system app didn't receive the event. By the way, where can I find documentation on 'shell.sendEvent'? I've looked but was not successful. Thanks.

Comment 9

[Jan Keromnes [:janx]]

Hi Russ, I was just curious why the events were being sent to an app frame, that wasn't an issue (you can totally keep it that way).

It's just that when I seach for "telemetry-update-event" in Gaia (or in DXR) I don't find any existing listeners for that event, and you're not adding one in your patch. My question is just: Where is the code that listens for "telemetry-update-event"?

Comment 10

[Russ Nicoletti [:russn]]

Tamara has been working on the consumer of the "telemetry-update-event". So far, her work is only in her branch: [1]. 

[1] https://github.com/tamarahills/gaia/blob/bugfix/AdvanceTelemetry/apps/system/js/advanced_telemetry.js

Comment 11

[Russ Nicoletti [:russn]]

Attachment: rn_6-15_hud.js.patch

Hi Jan,

This is my latest patch. Part of the feedback I hope you can give me is insight into the exception [1] I'm seeing during the "handleTelemetryLogEntry" flow. This is the flow that creates a telemetry event based on a "telemetry" console log entry. I'm not seeing how this flow is different from, for example, the "reflow duration" flow, which works fine.

[1] I/GeckoDump(25435): Handler function threw an exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMessageSender.sendAsyncMessage]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js :: DebuggerServer.connectToChild/destroy< :: line 900"  data: no]

Comment 12

[Russ Nicoletti [:russn]]

Jan, I understand now that the exception I referred to in comment 11 is unrelated to the issue I'm having with handleTelemetryLogEntry -- I'm seeing the exception in the logcat when handleTelemetryLogEntry is not called. But now that I have you interested :) I'll describe the issue I'm having. The advanced-telemetry-update event that is sent in the handleTelemetryLogEntry flow it never received by the system app (advanced_telemetry.js -- see link in comment 10). I see in the logcat that the event is sent, but I see no evidence at all that the event comes to the advanced_telemetry listener. Also, I don't see any errors in the logcat near where the event is sent. What do you make of this?

Comment 13

[Jan Keromnes [:janx]]

Comment on attachment 8622727 [details] [diff] [review]
rn_6-15_hud.js.patch

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

Thanks Russ, almost there! I'd just like you to split up the `handleTelemetryMetric` function again, the rest of my comments is about small details.

As for your latest comment, I'll try to investigate where your event lands. One potential problem I see is that _sendTelemetryEvent might send events to the wrong frame sometimes (when frame is the system app, you need to send to getContentWindow() instead, see _send for reference), but that problem should only affect metrics from the system app.

::: b2g/chrome/content/devtools/hud.js
@@ +35,5 @@
> +
> +function telemetryDebug(...args) {
> +  if (_telemetryDebug) {
> +    args.unshift('[AdvancedTelemetry]');
> +    console.log.apply(console, args);

Nit: Since you're using fancy ES6 syntax, please use `console.log(...args)` here.

@@ +271,5 @@
>        frame = getContentWindow();
>      }
>  
>      shell.sendEvent(frame, 'developer-hud-update', Cu.cloneInto(data, frame));
> +

Nit: Unnecessary empty line.

@@ +273,5 @@
>  
>      shell.sendEvent(frame, 'developer-hud-update', Cu.cloneInto(data, frame));
> +
> +    this._sendTelemetryEvent(data.metric);
> +

Nit: Unnecessary empty line.

@@ +289,5 @@
> +    // it now and store it. In any case, if the app name is set on the
> +    // metric, it takes precedence over the app name corresponding to the target.
> +    if (!this.appName) {
> +      var appNameIndexStart = this.frame.appManifestURL.indexOf('/') + 2;
> +      var appNameIndexEnd = this.frame.appManifestURL.indexOf('.', appNameIndexStart);

Nit: Please use let instead of var.

@@ +290,5 @@
> +    // metric, it takes precedence over the app name corresponding to the target.
> +    if (!this.appName) {
> +      var appNameIndexStart = this.frame.appManifestURL.indexOf('/') + 2;
> +      var appNameIndexEnd = this.frame.appManifestURL.indexOf('.', appNameIndexStart);
> +      this.appName = this.frame.appManifestURL.substring(appNameIndexStart, appNameIndexEnd);

Nit: These lines are getting long, please use a `let manifest = this.frame.appManifestURL` shortcut variable at the top of this if block.

@@ +299,5 @@
> +    }
> +
> +    let data = { metric: metric };
> +
> +    telemetryDebug('sending ' + 'advanced-telemetry-update' + ' with this data: ' + JSON.stringify(data));

Nit: No need to separate the three first strings.

@@ +450,5 @@
>          if (sourceURL) {
>            output += ' ' + this.formatSourceURL(packet);
>          }
> +
> +        // Telemetry also records reflow duration

Nit: Please add a period after this comment.

@@ +477,5 @@
>  
>      return source;
> +  },
> +
> +  handleTelemetryMetric: function cw_handleTelemetryMetric(metric, data, target) {

In my opinion, this function adds a lot of unnecessary code. Please remove it, and call `target._sendTelemetryEvent()` directly inside the `consoleListener` where needed.

@@ +480,5 @@
> +
> +  handleTelemetryMetric: function cw_handleTelemetryMetric(metric, data, target) {
> +    if (!developerHUD._telemetry || !this._watching[metric.name]) {
> +      return;
> +    }

You won't need to check for `developerHUD._telemetry` in `consoleListener`, because `target._sendTelemetryEvent` already checks that.

Also, what do you think about not verifying `this._watching[metric.name]` for telemetry? I suggested something like that in point 2. of bug 1167710 comment 7.

@@ +489,5 @@
> +        let securityMetric = {
> +          name: 'security',
> +          value: data
> +        };
> +        telemetryMetric = securityMetric;

As suggested above, please move this code into the `consoleListener`, and rewrite it to something smaller like `target._sendTelemetryEvent({name: 'security', value: pageError.category})`.

@@ +490,5 @@
> +          name: 'security',
> +          value: data
> +        };
> +        telemetryMetric = securityMetric;
> +        metric.telemetrySent = true;

Nit: At this point, the telemetry event isn't actually sent yet. Please rename this flag something like "skipTelemetry".

@@ +498,5 @@
> +        let reflowMetric = {
> +          name: 'reflow-duration',
> +          value: data
> +        };
> +        telemetryMetric = reflowMetric;

Please move this code into the `consoleListener` as something like `target._sendTelemetryEvent({name: 'reflow-duration', value: Math.round(duration)})`.

@@ +511,5 @@
> +    }
> +  },
> +
> +  handleTelemetryLogEntry:
> +    function cw_handleTelemetryLogEntry(target, packet) {

Nit: I find this function name a little confusing, would you be open to other suggestions, e.g. "handleTelemetryMessage"? Also, I'll remove these cw_repeatedFunctionNames at some point, so don't worry about them.

@@ +522,5 @@
> +    // the log content
> +    let separator = '|';
> +    let logContent = packet.message.arguments.toString();
> +
> +    if (logContent.indexOf('telemetry') > -1) {

Nit: In general, bail-out style is preferred. Please return if (locContent.indexOf('telemetry') < 0), and de-indent the rest of this function.

@@ +536,5 @@
> +      if (telemetryData[TELEMETRY_IDENTIFIER_IDX] != 'telemetry') {
> +        return;
> +      }
> +
> +      if (telemetryData.length === 4 || telemetryData.length === 5) {

Nit: Please use bail-out style.

Comment 14

[Jan Keromnes [:janx]]

Actually, since your question specifically mentions the system app, you should probably make _sendTelemetryEvent select the right frame to send the event to (like _send does).

Comment 15

[Russ Nicoletti [:russn]]

I was sure your hint in comment 14 would solve the "handleTelemetryLogEntry" problem, but alas, it has not so far. After adding that code I'm finding the advanced telemetry event is still not received by the system app. I'm noticing that after setting the frame to 'getContentWindow()', frame.appManifestURL is undefined. Is that expected? Also, frame.location is "app://system.gaiamobile.org/index.html"

Comment 16

[Russ Nicoletti [:russn]]

I'm handling the situation differently such that the issue of comment 12 and comment 14 is no longer relevant: I've changed the code that listens for the ttl events such that it directly sends an advanced telemetry event with the relevant information instead of using a console log to communicate the information to the HUD backend. This seems like a better/cleaner/simpler approach than what I was doing previously.

I am keeping the changes I made to _sendTelemetryEvent that selects the right frame to send the event to (like _send does). In any event, "handleTelemetryLogEntry" will work as expected as long as the system app does use console logs to communicate telemetry information to the HUD backend and I see no reason that it should.

Comment 17

[Jan Keromnes [:janx]]

I'm glad you found a way around your problem, although I'm not exactly sure what you meant by "ttl events" or "I see no reason that it should".

If you want, I could try applying a newer version of your patch, as well as Tamara's code on github, and see what I can make of it. Please let me know if you're interested.

Comment 18

[Russ Nicoletti [:russn]]

Sorry for the confusion. What I meant to say is, because the system app is handling the time-to-load metric internally (listening to "apploadtime" events and updating the HUD front end) it would be inefficient, in the case of "advanced telemetry", for the system app to send the time-to-load data to the HUD backend (via the console) and then for the HUD backend to send it back to the system app as an advanced telemetry event. It seems more efficient, and more consistent with the existing HUD code, for the system app to send the advanced telemetry event after getting the time-to-load data. 

I appreciate your offer of investigating the issue I was having. However, the code on github is implementing the "more efficient" approach where the problem I has having doesn't exist. If you'd like to see the code for the "more efficient" approach, it is here [1]

Since we'll be at Whistler soon, I think it would be better to discuss the handleTelemetryLogEntry issue in more detail in person. (I'm looking forward to that!)

[1] https://github.com/tamarahills/gaia/blob/bugfix/AdvanceTelemetry/apps/system/js/ttl_telemetry.js

Comment 19

[Jan Keromnes [:janx]]

Ah, right, that makes sense. I also look forward to chatting about this at Whistler! If we don't run into each other I'll try to ping you.

Comment 20

[Russ Nicoletti [:russn]]

These hud.js changes are being tracked by bug 1178512

Comment 21

[Jan Keromnes [:janx]]

Russ, please mark patches as "obsolete" where needed (on the attachment, click on "Details", then on "edit details" and mark the "Obsolete" checkbox).

Comment 22

[Russ Nicoletti [:russn]]

Marking this bug invalid as we have determined it is more useful to record uss memory at the time of performance marks -- bug 1190622