AdvancedTelemetry.js is a system module to collect histograms from Gecko and ship them off to the telemetry server periodically. AdvancedTelemetry does the following: - Keep track of how often metrics should be sent - Keep track of where they should be sent - Format the ping packet to match to new Unified Telemetry format v4 - Handle sending failures. - Keep track of when Advanced Telemetry is turned on/off and collect metrics accordingly.

Comment on attachment 8632793 [details] [gaia] tamarahills:bugfix/1183101-AdvancedTelemetry > mozilla-b2g:master Hi Marshall, Wanted to see if you are still available to feedback this before I start working on the test suites for it. Thanks, -tamara

Hi Ryan, Jan sent us to you for feedbacks/reviews in hud area while he's out. Attached is a WIP for feedback. Essentially, what it does is: 1. Replaces the _sendTelemetryEvent with a logHistogram call. logHistogram logs data into the gecko histograms. 2. Periodically, gaia will ask for that set of histograms and hud.js will send it back to gaia for shipping out to the telemetry server as per the mobile settings vs. desktop settings. Thanks, -tamara

Comment on attachment 8633599 [details] [diff] [review] Gecko Portion Review of attachment 8633599 [details] [diff] [review]: ----------------------------------------------------------------- Seems okay overall, mostly just various style nits. Will need another round of review. ::: b2g/chrome/content/devtools/hud.js @@ +284,3 @@ > }, > > + _clearTelemetryData: function target_clearTelemetryData() { Current DevTools style does not use named functions like this anymore, so you can just use: _clearTelemetryData() @@ +299,3 @@ > return; > } > + telemetryDebug('calling sendTelemetryData'); This is fine, but do you really want debug logging enabled by default at the top of the file? Seemed surprising to me, but up to you. @@ +299,5 @@ > return; > } > + telemetryDebug('calling sendTelemetryData'); > + let frame = this.frame; > + var payload = { Nit: prefer let @@ +335,4 @@ > } > > + // The app name can come from this target object (in the case of > + // standard HUD metrics) or it can can can be set on the metric object I don't follow this, it seems like you always set it on the metric object? Nit: only one "can" @@ +344,3 @@ > > + if (!metric.custom) { > + var keyed = Services.telemetry.getKeyedHistogramById(metric.name); Nit: prefer let @@ +348,5 @@ > + keyed.add(metric.appName, parseInt(metric.value, 10)); > + developerHUD._histograms.add(metric.name); > + } > + } else { > + if (!developerHUD._customHistograms.has(metric.appName + '_'+ metric.name)) { Capture |metric.appName + '_'+ metric.name| in a variable, so you can reuse it in the 2 places it appears. ::: toolkit/components/telemetry/Histograms.json @@ +8475,5 @@ > "expires_in_version": "43", > "kind": "boolean", > "description": "Is the title shown in the url bar (as determined by the titlebar title or url preference)?" > }, > + "jank": { The names are way too generic. Please review the style of names in the rest of this file. Perhaps something like "DEVTOOLS_HUD_JANK" for this one? @@ +8482,5 @@ > + "keyed": "true", > + "description": "The duration which a thread is blocked in ms", > + "high": "5000", > + "n_buckets": 10, > + "extended_statistics_ok": true The seems like a deprecated field[1], why do you need it? [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Declaring_a_Histogram @@ +8497,5 @@ > + "reflows": { > + "expires_in_version": "never", > + "kind": "count", > + "keyed": "true", > + "description": "number of reflows in the video app" Start the description with an upper case letter. I think you mean "keyed app". But, I think it's even better to end with ", keyed by app name" (or whatever the key actually is), so it's easier to understand later. See other examples in the file.

Comment on attachment 8632793 [details] [gaia] tamarahills:bugfix/1183101-AdvancedTelemetry > mozilla-b2g:master Hey Tamara, I left some comments on Github that may have been confusing if you were reading in real time. I cleaned it up and just left the important feedback :) After looking over the proposed gecko patch for reference, I think I understand how this is supposed to work now, and this looks good as a first crack.

Hi Ryan, Thanks for the feedbacks and good catches. Attached is another patch with with the issues addressed. Thanks, -tamara

Comment on attachment 8635430 [details] [diff] [review] 1183101.diff Review of attachment 8635430 [details] [diff] [review]: ----------------------------------------------------------------- Getting close! I'd like one more round so I can double check the casing of the custom histograms. ::: b2g/chrome/content/devtools/hud.js @@ +288,5 @@ > + Services.telemetry.getKeyedHistogramById(item).clear(); > + }); > + > + developerHUD._customHistograms.forEach(function(item) { > + var array = item.split('_'); Nit: prefer let @@ +306,5 @@ > + }; > + developerHUD._histograms.forEach(function(item) { > + payload.keyedHistograms[item] = Services.telemetry.getKeyedHistogramById(item).snapshot(); > + }); > + payload.addonHistograms = Services.telemetry.addonHistogramSnapshots; Doesn't this mean you're including *all* addon histograms, even ones that might not have been created by hud.js? Is it correct to include such histograms in the payload? @@ +347,5 @@ > + keyed.add(metric.appName, parseInt(metric.value, 10)); > + developerHUD._histograms.add(keyedMetricName); > + } > + } else { > + let histogramName = metric.appName + '_' + metric.name; Custom histograms should still follow naming conventions. What's an example of one of these custom metric names today? Without having an example, I would suggest a prefix of "DEVTOOLS_HUD_CUSTOM_", or at least "DEVTOOLS_HUD_", up to you. Also, |histogramName| should be converted to upper case. @@ +520,5 @@ > output += ' ' + this.formatSourceURL(packet); > } > > // Telemetry also records reflow duration. > + target._logHistogram({name: 'reflow_duration', Nit: probably more clear to move each object field to its own line, like "security_category" above. ::: toolkit/components/telemetry/Histograms.json @@ +8476,5 @@ > "kind": "boolean", > "description": "Is the title shown in the url bar (as determined by the titlebar title or url preference)?" > }, > + "DEVTOOLS_HUD_JANK": { > + "expires_in_version": "default", Some of these have expires "default" and others "never". I am guessing you want "never" for all of them?

Comment on attachment 8632793 [details] [gaia] tamarahills:bugfix/1183101-AdvancedTelemetry > mozilla-b2g:master Hi Marshall, Thanks for the feedback and good suggestions/catches. Here's another version with added tests and also I moved the getSettings into the telemetry.js so it's now shared by AdvancedTelemetry and AppUsageMetrics. Also, just FYI, i'm out on vaca this week. Will be back on Monday. Thanks, -tamara

Hi Ryan, Here is an updated patch. I believe I have taken care of the nits. Good catch on the addon histograms. I think you are right. For now, we should just not include any other histograms. I changed the format of the custom metric to be prefixed with to DEVTOOLS_HUD_CUSTOM_. As well, I upperCase'd all the appNames and metric names. Thanks, -tamara

Comment on attachment 8643109 [details] [diff] [review] Gecko Portion Review of attachment 8643109 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/devtools/hud.js @@ +289,5 @@ > + Services.telemetry.getKeyedHistogramById(item).clear(); > + }); > + > + developerHUD._customHistograms.forEach(function(item) { > + let array = item.split('_'); Splitting like this is only safe if you are sure the app name and metric name will never contain "_" themselves. Are we sure this is the case? @@ +290,5 @@ > + }); > + > + developerHUD._customHistograms.forEach(function(item) { > + let array = item.split('_'); > + Services.telemetry.getAddonHistogram(array[3].toUpperCase(), It might be nicer for future readers to capture `array[3].toUpperCase()` as a named variable, like appName. Same with array[4] below. @@ +361,5 @@ > + let histogramName = CUSTOM_PREFIX + metric.appName.toUpperCase() + > + '_' + metric.name.toUpperCase(); > + if (!developerHUD._customHistograms.has(histogramName)) { > + try { > + Services.telemetry.registerAddonHistogram(metric.appName.toUpperCase(), Is `registerAddonHistogram` really the correct API? I don't know Telemetry APIs in detail, have you asked the Telemetry team about this? We aren't discussing add-ons here, so the API seems suspect. Would `newHistogram` be better? Again, I do not know for sure myself.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > Comment on attachment 8643109 [details] [diff] [review] > Gecko Portion > Review of attachment 8643109 [details] [diff] [review]: > ----------------------------------------------------------------- > Splitting like this is only safe if you are sure the app name and metric > name will never contain "_" themselves. Are we sure this is the case? Hi Ryan, yes, that is right. What I was thinking of doing is stripping out the '_' in my Gaia patch in bug 1176992. I could return either return an error or just fix it up for the caller. That is one approach. Let me know your thoughts on this. > Is `registerAddonHistogram` really the correct API? I don't know Telemetry > APIs in detail, have you asked the Telemetry team about this? We aren't > discussing add-ons here, so the API seems suspect. > > Would `newHistogram` be better? Again, I do not know for sure myself. Hi Ryan, yes, I did discuss with Telemetry team when I met them at Whistler. The issue is that any newHistogram needs to be declared in Histograms.json, compiled into the Telemetry server, so it can't be done dynamically. We were thinking that this is a bit of a difficulty for the developer so Telemetry team recommended using addonHistograms to do it dynamically as those don't need to be precompiled into the server. Thanks, -tamara

(In reply to Tamara Hills [:thills] from comment #11) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #10) > > Comment on attachment 8643109 [details] [diff] [review] > > Gecko Portion > > Review of attachment 8643109 [details] [diff] [review]: > > ----------------------------------------------------------------- > > Splitting like this is only safe if you are sure the app name and metric > > name will never contain "_" themselves. Are we sure this is the case? > > Hi Ryan, yes, that is right. What I was thinking of doing is stripping out > the '_' in my Gaia patch in bug 1176992. I could return either return an > error or just fix it up for the caller. That is one approach. Let me know > your thoughts on this. Hmm, I don't feel strongly about it. As long you are aware of the potential for trouble, it's okay with me.

Comment on attachment 8643109 [details] [diff] [review] Gecko Portion Review of attachment 8643109 [details] [diff] [review]: ----------------------------------------------------------------- ::: b2g/chrome/content/devtools/hud.js @@ +361,5 @@ > + let histogramName = CUSTOM_PREFIX + metric.appName.toUpperCase() + > + '_' + metric.name.toUpperCase(); > + if (!developerHUD._customHistograms.has(histogramName)) { > + try { > + Services.telemetry.registerAddonHistogram(metric.appName.toUpperCase(), Is there any value to supplying the app name as the "addon ID" (first arg) of `registerAddonHistogram`? It seems like you could supply a constant value, such as "devtools-hud" or something. Then, for the histogram name, just use your `histogramName` value. This way the underscore splitting won't be needed, IIUC.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13) > Comment on attachment 8643109 [details] [diff] [review] > Gecko Portion > > Review of attachment 8643109 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: b2g/chrome/content/devtools/hud.js > @@ +361,5 @@ > > + let histogramName = CUSTOM_PREFIX + metric.appName.toUpperCase() + > > + '_' + metric.name.toUpperCase(); > > + if (!developerHUD._customHistograms.has(histogramName)) { > > + try { > > + Services.telemetry.registerAddonHistogram(metric.appName.toUpperCase(), > > Is there any value to supplying the app name as the "addon ID" (first arg) > of `registerAddonHistogram`? It seems like you could supply a constant > value, such as "devtools-hud" or something. Then, for the histogram name, > just use your `histogramName` value. > > This way the underscore splitting won't be needed, IIUC. Hi Ryan, We could do this, but I think I'd prefer keeping the concept of an app inside somewhere. The original thought when I talked with Telemetry folks was that a gaia app is akin to an addon. If we remove the app from the addon id, then we'd still need to keep the appName as part of the histogram ID, and that would still require the splitting. Thanks, -tamara

(In reply to Tamara Hills [:thills] from comment #14) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13) > > Comment on attachment 8643109 [details] [diff] [review] > > Gecko Portion > > > > Review of attachment 8643109 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: b2g/chrome/content/devtools/hud.js > > @@ +361,5 @@ > > > + let histogramName = CUSTOM_PREFIX + metric.appName.toUpperCase() + > > > + '_' + metric.name.toUpperCase(); > > > + if (!developerHUD._customHistograms.has(histogramName)) { > > > + try { > > > + Services.telemetry.registerAddonHistogram(metric.appName.toUpperCase(), > > > > Is there any value to supplying the app name as the "addon ID" (first arg) > > of `registerAddonHistogram`? It seems like you could supply a constant > > value, such as "devtools-hud" or something. Then, for the histogram name, > > just use your `histogramName` value. > > > > This way the underscore splitting won't be needed, IIUC. > > > Hi Ryan, > > We could do this, but I think I'd prefer keeping the concept of an app > inside somewhere. The original thought when I talked with Telemetry folks > was that a gaia app is akin to an addon. If we remove the app from the > addon id, then we'd still need to keep the appName as part of the histogram > ID, and that would still require the splitting. Okay, I'll leave this up to you then. I don't have a strong opinion myself, I just wanted to be sure these factors would considered.

Hi Marshall, review ping. Thanks, -tamara

Comment on attachment 8643109 [details] [diff] [review] Gecko Portion Review of attachment 8643109 [details] [diff] [review]: ----------------------------------------------------------------- Hijacking this review because I'm back from holidays. Sorry for barging in! --- Ryan, I believe some of your nits weren't entirely addressed. I suppose your r+ is conditional upon their fix: (J. Ryan Stinnett [:jryans] from comment #10) > ::: b2g/chrome/content/devtools/hud.js > @@ +290,5 @@ > > + }); > > + > > + developerHUD._customHistograms.forEach(function(item) { > > + let array = item.split('_'); > > + Services.telemetry.getAddonHistogram(array[3].toUpperCase(), > > It might be nicer for future readers to capture `array[3].toUpperCase()` as > a named variable, like appName. Same with array[4] below. `array[3].toUpperCase()` and `array[4].toUpperCase()` would still benefit from becoming named variables. (J. Ryan Stinnett [:jryans] from comment #4) > ::: toolkit/components/telemetry/Histograms.json > @@ +8497,5 @@ > > + "reflows": { > > + "expires_in_version": "never", > > + "kind": "count", > > + "keyed": "true", > > + "description": "number of reflows in the video app" > > Start the description with an upper case letter. For the two occurrences of "number" in the histogram descriptions. --- Here are a few additional nits as well: ::: b2g/chrome/content/devtools/hud.js @@ +6,5 @@ > > // settings.js loads this file when the HUD setting is enabled. > > const DEVELOPER_HUD_LOG_PREFIX = 'DeveloperHUD'; > +const CUSTOM_PREFIX = 'DEVTOOLS_HUD_CUSTOM_'; Nit: Please use a more explicit variable name like "CUSTOM_HISTOGRAM_PREFIX" or "CUSTOM_TELEMETRY_PREFIX". @@ +315,5 @@ > + developerHUD._customHistograms.forEach(function(item) { > + let array = item.split('_'); > + payload.addonHistograms[item] = Services.telemetry.getAddonHistogram( > + array[3].toUpperCase(), CUSTOM_PREFIX + array[4].toUpperCase()).snapshot(); > + }); Nit: This code looks very similar to the one in `_clearTelemetryData()`, maybe there is a way to factor out some code? E.g. > _getAddonHistogram(item) { > let array = item.toUpperCase().split('_'); > let id = array[3]; > let name = CUSTOM_HISTOGRAM_PREFIX + array[4]; > return Services.telemetry.getAddonHistogram(id, name); > }, > > _clearTelemetryData() { > // [...] > developerHUD._customHistograms.forEach(item => { > this._getAddonHistogram(item).clear(); > }); > }, > > _sendTelemetryData() { > // [...] > developerHUD._customHistograms.forEach(item => { > payload.addonHistograms[item] = > this._getAddonHistogram(item).snapshot(); > }); > }, @@ +358,5 @@ > + developerHUD._histograms.add(keyedMetricName); > + } > + } else { > + let histogramName = CUSTOM_PREFIX + metric.appName.toUpperCase() + > + '_' + metric.name.toUpperCase(); Nit: I see too many calls to `.toUpperCase()` here, please define a few variables and reuse them below, e.g. > let appName = metric.appName.toUpperCase(); > let metricName = metric.name.toUpperCase(); > let histogramName = CUSTOM_HISTOGRAM_PREFIX + appName + '_' + metricName; @@ +534,5 @@ > > // Telemetry also records reflow duration. > + target._logHistogram({ > + name: 'reflow_duration', > + value: Math.round(duration)}); Nit: `});` on separate line.

Hi Jan, Thanks for the upgrades. I had forgotten to update with the latest patch that addressed some of these. I think I got all of them. Thanks, -tamara

Comment on attachment 8632793 [details] [gaia] tamarahills:bugfix/1183101-AdvancedTelemetry > mozilla-b2g:master Looks good, sorry for the slow turnaround. I left 2 new comments but they are both nits :)

Comment on attachment 8647561 [details] [diff] [review] Gecko Portion Review of attachment 8647561 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick reply! We're almost there. Also, did you manage to try the code out to see if it works as expected? ::: b2g/chrome/content/devtools/hud.js @@ +299,5 @@ > + Services.telemetry.getKeyedHistogramById(item).clear(); > + }); > + > + developerHUD._customHistograms.forEach(function(item) { > + Services.telemetry._getAddonHistogram(item).clear(); I believe you mean `this._getAddonHistogram(item)` instead of `Services.telemetry._getAddonHistogram(item)`. Also, to get `this` to work properly, please use an arrow function: > [...].forEach(item => { > this._getAddonHistogram(item).clear(); > }); @@ +320,5 @@ > + Services.telemetry.getKeyedHistogramById(item).snapshot(); > + }); > + // Package the registered hud custom histograms > + developerHUD._customHistograms.forEach(function(item) { > + payload.addonHistograms[item] = _getAddonHistogram(item).snapshot(); Same as above, please use `this._getAddonHistogram(item)` within an arrow function. @@ +342,2 @@ > if (!this.appName) { > + if (!frame.appManifestURL) { Why change this? I believe that in the case of the System App, `this.frame` will be the content window instead of the frame (see `get frame()`), even though the `appManifestURL` still lives in the actual frame. That's why Russ created the `this.manifest` getter (see `get manifest()`) and used it here. @@ +377,5 @@ > + } > + } > + Services.telemetry.getAddonHistogram(metric.appName.toUpperCase(), > + CUSTOM_HISTOGRAM_PREFIX + > + metric.name.toUpperCase()).add(parseInt(metric.value, 10)); Please see my nit from comment 17 about reducing calls to `.toUpperCase()`.

Comment on attachment 8647561 [details] [diff] [review] Gecko Portion +const APPNAME_IDX = 3; +const HISTNAME_IDX = 4; It seems thise are only used in `_getAddonHistogram`, therefore I think it's best if they are defined there. + // The app name can come from this target object (in the case of + // 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. I had removed that comment because it is no longer accurate. The app name always comes from the target; it does not come from the metric. if (!this.appName) { - let manifest = this.manifest; - if (!manifest) { + if (!frame.appManifestURL) { return; } - let start = manifest.indexOf('/') + 2; - let end = manifest.indexOf('.', start); - this.appName = manifest.substring(start, end).toLowerCase(); + + let appManifestURL = frame.appManifestURL; + let start = appManifestURL.indexOf('/') + 2; + let end = appManifestURL.indexOf('.', start); + this.appName = appManifestURL.substring(start, end).toLowerCase(); As we discussed, it seems you mistakenly overwrote newer code with your older code. I prefer the newer code. + telemetryDebug('Histogram error: ' + err); Should this be logged as an error, that is, using `console.error`?

Hi Jan, This version has your last comments, plus Russ', plus an additional bug that I found while testing. I had to remove the metric.context and the CONTEXT_IDX as well as a block that was appending the context. I'm not sure if I should be flagging you or Ryan for this. Thanks, -tamara

(In reply to Marshall Culpepper [:marshall_law] from comment #19) > Comment on attachment 8632793 [details] > [gaia] tamarahills:bugfix/1183101-AdvancedTelemetry > mozilla-b2g:master > > Looks good, sorry for the slow turnaround. I left 2 new comments but they > are both nits :) Hi Marshall, Thanks for the review. I pushed a new patch that 1) combined the debugLongLine into debug and also removed that getSettings var in telemetry_test. I'm just putting ni to you here so you see this. Thanks, -tamara

Looks good, thanks Tamara :)

Comment on attachment 8648077 [details] [diff] [review] Gecko portion v3 Review of attachment 8648077 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Tamara! Please submit this to try before checking it in.

Try run for Gaia portion: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=019ca84ff558130586337586059011d7595df43e

Hi Jan, I had to rebase as Russ' patch had landed. I made one change to change a function name usage to _logHistogram and then another change to put a try catch around the lookup of keyed histograms.

Try Result from the gecko portion for Attachment #8649852 [details] [diff] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e2e006fcde7

Comment on attachment 8649852 [details] [diff] [review] Gecko portion -- after rebase Review of attachment 8649852 [details] [diff] [review]: ----------------------------------------------------------------- Still looks good to me, thanks!