1215277 - [Metrics] "low" and "high" values for "APP_MEMORY" histograms are wrong

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect)

Description

[Russ Nicoletti [:russn]]

The "low" and "high" values for the "APP_MEMORY" histograms are seven digits. App memory is always greater than 10MB so the "low" and "high" values should be eight digits.

The following histograms need to be updated:

  "DEVTOOLS_HUD_APP_MEMORY_CONTENTINTERACTIVE"
  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONINTERACTIVE"
  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONLOADED"
  "DEVTOOLS_HUD_APP_MEMORY_VISUALLYLOADED"
  "DEVTOOLS_HUD_APP_MEMORY_MEDIAENUMERATED"
  "DEVTOOLS_HUD_APP_MEMORY_FULLYLOADED"
  "DEVTOOLS_HUD_APP_MEMORY_SCANEND"

Comment 1

[Russ Nicoletti [:russn]]

Attachment: app-memory-low-high.patch

This is a patch to correct typos in the low/high attributes of the "APP_MEMORY" histograms.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8674617 [details] [diff] [review]
app-memory-low-high.patch

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

Please ask someone from telemetry if it is safe to make this change.

According to the MDN page[1]:

"Changing histogram declarations after the histogram has been released is tricky. You will need to create a new histogram with the new parameters."

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Miscellaneous

Comment 3

[Jan Keromnes [:janx]]

Comment on attachment 8674617 [details] [diff] [review]
app-memory-low-high.patch

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

Forwarding this Histogram.json change review to Telemetry peer Vladan.

Also, I believe you can't change a histogram's parameter without also changing its ID.

Comment 4

[Jan Keromnes [:janx]]

(Sorry for the duplicate info, I didn't see that Ryan had already said everything.)

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8674617 [details] [diff] [review]
app-memory-low-high.patch

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

Yes, you should define a new histogram if you're changing any of the bucketing parameters

Comment 6

[Russ Nicoletti [:russn]]

Attachment: bug-1215277.patch

Ok, here's a patch for creating 'v2' app-memory histograms.

Comment 7

[Jan Keromnes [:janx]]

I suppose these histograms are referenced somewhere in the Advanced Telemetry code. Wouldn't it be better to replace the histograms, and change the code that sends data for them, in a single patch?

Comment 8

[Russ Nicoletti [:russn]]

(In reply to Jan Keromnes [:janx] from comment #7)
> I suppose these histograms are referenced somewhere in the Advanced
> Telemetry code. Wouldn't it be better to replace the histograms, and change
> the code that sends data for them, in a single patch?

The histograms are not referenced in the Advanced Telemetry code. It simply packages up the histogram data that it receives from the Developer HUD; it doesn't examine of manipulate the histogram data.

The Advanced Telemetry code tells the Developer HUD when it needs to send telemetry data to the telemetry server; the Developer HUD then retrieves the histogram data (via Services.telemetry.getKeyedHistogramById) and returns it to the AT system module as a JSON object. The AT code adds the appropriate wrapper then ships it all off to the telemetry server -- the histogram payload from the Developer HUD is sent verbatim.

Comment 9

[Jan Keromnes [:janx]]

What I meant to say is, instead of adding a new set of (unused) histograms, and later change the code responsible for addressing the right histograms to make them use the new histograms, wouldn't it be better to do both changes in the same patch?

Comment 10

[Russ Nicoletti [:russn]]

Ok, I think I understand your comment now. The Developer HUD code that is adding data to the APP_MEMORY histograms is adding eight-digit values, as it should. But the existing APP_MEMORY histograms have seven-digit 'low' and 'high' ranges. So the code doesn't need to be changed because it is correct; it's the existing histograms that are in need of being replaced with new ones with the proper 'low/high' values.

Comment 11

[Jan Keromnes [:janx]]

Oh wow, bugzilla's review tool is hiding a file! No wonder you're confused by what I'm asking for.

You've already changed the hud.js code to point to the "_V2" Histograms (I can see that by looking at the raw attachment), but that change doesn't appear when I click on "Review" (Splinter makes it look like you only changed the histograms.json file).

Sorry about the confusion, I'll review the raw attachment directly.

Comment 12

[Jan Keromnes [:janx]]

Comment on attachment 8675111 [details] [diff] [review]
bug-1215277.patch

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

I reviewed the raw patch (because Splinter hides a file change) and it looks good to me!

Now let's wait for Vladan's review of the histogram parameters.

> In /b2g/chrome/content/devtools/hud.js:
>        memoryWatcher.front(target).residentUnique().then(value => {
> -        eventName = 'app_memory_' + name;
> +        // bug 1215277, need 'v2' for app-memory histograms
> +        eventName = 'app_memory_' + name + '_v2';

Nit: Please start this comment with an uppercase "B" and end it with a period ".". Thanks!

Comment 13

[Vladan Djeric (:vladan)]

Comment on attachment 8675111 [details] [diff] [review]
bug-1215277.patch

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

- How many distinct keys do you expect to get reported?
- Delete the V1 histograms in the same patch?
- Add an alert_emails field so we know the owner of the histogram & so that monitoring tools can notify of changes
- Set a real expiry version, we don't want to accumulate histograms forever, especially if they stop being used at some point. You can put it say 1 year (8 versions) in the future and you'll get a reminder to update the expiry date before it ever expires
- Telemetry is opt-in on B2G, right?

Comment 14

[Russ Nicoletti [:russn]]

[Blocking Requested - why for this release]:

If this bug is not fixed, the 2.5 telemetry feature will not record application memory data. This is a critical feature for 2.5 telemetry.

Comment 15

[Russ Nicoletti [:russn]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #13)
> Comment on attachment 8675111 [details] [diff] [review]
> bug-1215277.patch
> 
> Review of attachment 8675111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> - How many distinct keys do you expect to get reported?

The key is an app (certified). I haven't counted but I would guess there would be 10-20 certified apps on a device?

> - Delete the V1 histograms in the same patch? 

I thought we had to keep the v1 histograms but it sounds like we don't. I will update the patch to remove the v1 histograms.

> - Add an alert_emails field so we know the owner of the histogram & so that
> monitoring tools can notify of changes
> - Set a real expiry version, we don't want to accumulate histograms forever,
> especially if they stop being used at some point. You can put it say 1 year
> (8 versions) in the future and you'll get a reminder to update the expiry
> date before it ever expires

Ok, I’ve added alert_emails and expires_in_version to all “telemetry” histograms

> - Telemetry is opt-in on B2G, right?

Yes, see bug 1181295 and bug 1208205

Comment 16

[Russ Nicoletti [:russn]]

Comment on attachment 8675111 [details] [diff] [review]
bug-1215277.patch

# HG changeset patch
# User Russ Nicoletti <[e-mail]>
# Date 1445387919 25200
#      Tue Oct 20 17:38:39 2015 -0700
# Node ID 931d946c715aeae08d5bc8ef1b29567245d18c0b
# Parent  0d5765f295a8d5d77dca9e9c2f2fb3f128cc164d
Bug 1215277 - [Metrics] 'low' and 'high' values for APP_MEMORY histograms are wrong r=janx,vlad

diff --git a/b2g/chrome/content/devtools/hud.js b/b2g/chrome/content/devtools/hud.js
--- a/b2g/chrome/content/devtools/hud.js
+++ b/b2g/chrome/content/devtools/hud.js
@@ -831,17 +831,18 @@ var performanceEntriesWatcher = {
       let time = epoch - this._appLaunchStartTime;
       let eventName = 'app_startup_time_' + name;
 
       // Events based on performance marks are for telemetry only, they are
       // not displayed in the HUD front end.
       target._logHistogram({name: eventName, value: time});
 
       memoryWatcher.front(target).residentUnique().then(value => {
-        eventName = 'app_memory_' + name;
+        // bug 1215277, need 'v2' for app-memory histograms
+        eventName = 'app_memory_' + name + '_v2';
         target._logHistogram({name: eventName, value: value});
       }, err => {
         console.error(err);
       });
     });
   },
 
   untrackTarget(target) {
diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -9076,173 +9076,193 @@
     "alert_emails": ["[e-mail]"],
     "expires_in_version": "never",
     "kind": "enumerated",
     "n_values": 10,
     "releaseChannelCollection": "opt-out",
     "description": "Reports whether the graphics sanity test passed an OS snapshot test. 0=Pass, 1=Fail, 2=Error, 3=Timed out."
   },
   "DEVTOOLS_HUD_JANK": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "exponential",
     "keyed": "true",
     "description": "The duration which a thread is blocked in ms, keyed by appName.",
     "high": "5000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_REFLOW_DURATION": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "exponential",
     "keyed": "true",
     "description": "The duration a reflow takes in ms, keyed by appName.",
     "high": "1000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_REFLOWS": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "count",
     "keyed": "true",
     "description": "A count of the number of reflows, keyed by appName."
   },
   "DEVTOOLS_HUD_SECURITY_CATEGORY": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "enumerated",
     "keyed": "true",
     "description": "The security error enums, keyed by appName.",
     "n_values": 8
   },
   "DEVTOOLS_HUD_ERRORS": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "count",
     "keyed": "true",
     "description": "Number of errors, keyed by appName."
   },
   "DEVTOOLS_HUD_WARNINGS": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "count",
     "keyed": "true",
     "description": "Number of warnings, keyed by appName."
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_CONTENTINTERACTIVE": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'contentInteractive' performance mark, keyed by appName.",
     "high": "2000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_NAVIGATIONINTERACTIVE": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'navigationInteractive' performance mark, keyed by appName.",
     "high": "3000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_NAVIGATIONLOADED": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'navigationLoaded' performance mark, keyed by appName.",
     "high": "4000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_VISUALLYLOADED": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'visuallyLoaded' performance mark, keyed by appName.",
     "high": "5000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_MEDIAENUMERATED": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'mediaEnumerated' performance mark, keyed by appName.",
     "high": "5000",
     "n_buckets": 10
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_FULLYLOADED": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'fullyLoaded' performance mark, keyed by appName.",
     "high": "30000",
     "n_buckets": 30
   },
   "DEVTOOLS_HUD_APP_STARTUP_TIME_SCANEND": {
-    "expires_in_version": "never",
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The duration in ms between application launch and the 'scanEnd' performance mark, keyed by appName.",
     "high": "30000",
     "n_buckets": 30
   },
-  "DEVTOOLS_HUD_APP_MEMORY_CONTENTINTERACTIVE": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_CONTENTINTERACTIVE_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'contentInteractive' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "3000000",
+    "low": "20000000",
+    "high": "30000000",
     "n_buckets": 10
   },
-  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONINTERACTIVE": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONINTERACTIVE_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'navigationInteractive' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "3000000",
+    "low": "20000000",
+    "high": "30000000",
     "n_buckets": 10
   },
-  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONLOADED": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_NAVIGATIONLOADED_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'navigationLoaded' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "3000000",
+    "low": "20000000",
+    "high": "30000000",
     "n_buckets": 10
   },
-  "DEVTOOLS_HUD_APP_MEMORY_VISUALLYLOADED": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_VISUALLYLOADED_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'visuallyLoaded' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "3000000",
+    "low": "20000000",
+    "high": "30000000",
     "n_buckets": 10
   },
-  "DEVTOOLS_HUD_APP_MEMORY_MEDIAENUMERATED": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_MEDIAENUMERATED_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'mediaEnumerated' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "3000000",
+    "low": "20000000",
+    "high": "40000000",
     "n_buckets": 10
   },
-  "DEVTOOLS_HUD_APP_MEMORY_FULLYLOADED": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_FULLYLOADED_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'fullyLoaded' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "4000000",
+    "low": "20000000",
+    "high": "40000000",
     "n_buckets": 20
   },
-  "DEVTOOLS_HUD_APP_MEMORY_SCANEND": {
-    "expires_in_version": "never",
+  "DEVTOOLS_HUD_APP_MEMORY_SCANEND_V2": {
+    "alert_emails": ["[e-mail]","[e-mail]"],
+    "expires_in_version": "52",
     "kind": "linear",
     "keyed": "true",
     "description": "The USS memory consumed by an application at the time of the 'scanEnd' performance mark, keyed by appName.",
-    "low": "2000000",
-    "high": "4000000",
+    "low": "20000000",
+    "high": "40000000",
     "n_buckets": 20
   },
   "GRAPHICS_SANITY_TEST_REASON": {
     "alert_emails": ["[e-mail]"],
     "expires_in_version": "43",
     "kind": "enumerated",
     "n_values": 20,
     "releaseChannelCollection": "opt-out",
[russ@mozillas-MacBook-Pro:/Volumes/firefoxos/s

Comment 17

[Russ Nicoletti [:russn]]

Attachment: bug-1215277.patch

Please ignore comment 16.

Comment 18

[Vladan Djeric (:vladan)]

Comment on attachment 8676586 [details] [diff] [review]
bug-1215277.patch

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

20 unique keys per histogram is fine. The telemetry dash & backend would be slow if all keyed histograms had hundreds or thousands of different keys

::: b2g/chrome/content/devtools/hud.js
@@ +841,1 @@
>          target._logHistogram({name: eventName, value: value});

oic, you change the histogram name to uppercase in _logHistogram()

Comment 19

[Russ Nicoletti [:russn]]

try success: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ada05c8dad8

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/6ef57e39eef1

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ef57e39eef1