I noticed an exception when the browser toolbox closed, 'this._telemetry is undefined'. It's referring to the calls inside of gDevTools.pingTelemetry [0].

The Telemetry object is loaded in the gDevTools file, so I'm assuming it was meant to be used but none of the calls to update histograms there are happening. However, if I add `this._telemetry = new Telemetry();` before the calls, I see these errors: Warning: An attempt was made to write to the DEVTOOLS_TABS_OPEN_AVERAGE_LINEAR histogram, which is not defined in Histograms.json Warning: An attempt was made to write to the DEVTOOLS_TABS_PINNED_AVERAGE_EXPONENTIAL histogram, which is not defined in Histograms.json These are the histograms that are meant to be written to according to the code: "DEVTOOLS_TABS_OPEN_PEAK_LINEAR" "DEVTOOLS_TABS_OPEN_AVERAGE_LINEAR" "DEVTOOLS_TABS_PINNED_PEAK_EXPONENTIAL" "DEVTOOLS_TABS_PINNED_AVERAGE_EXPONENTIAL" Mike, Jeff, what's going on here? Which of these (if any) do we care about? None of them are being written to at the moment, so we can remove the code entirely if none of them are important anymore. [0]: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#488

They were just added in bug 1046234, so I think it's safe to assume we want them to work! It looks like the set of names added[1] in Histograms.json got a bit jumbled and does not match up with what the code is using. [1]: https://hg.mozilla.org/mozilla-central/rev/ec213bf54bcf

Ouch, this is due to a last minute change... fixing.

(In reply to Brian Grinstead [:bgrins] from comment #1) > These are the histograms that are meant to be written to according to the > code: > > "DEVTOOLS_TABS_OPEN_PEAK_LINEAR" > "DEVTOOLS_TABS_OPEN_AVERAGE_LINEAR" > "DEVTOOLS_TABS_PINNED_PEAK_EXPONENTIAL" > "DEVTOOLS_TABS_PINNED_AVERAGE_EXPONENTIAL" > > Mike, Jeff, what's going on here? Which of these (if any) do we care about? > None of them are being written to at the moment, so we can remove the code > entirely if none of them are important anymore. > > [0]: > https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/ > gDevTools.jsm#488 We care about all of them but I was told at the last minute to change some EXPONENTIAL histograms to LINEAR and trusted our broken telemetry tests. Current telemetry tests are broken in lots of ways and my fix causes a seemingly unrelated crash in another part of our codebase. We decided to land the code without the tests so try was green and I missed this issue. In the meantime I will test locally to ensure that the histograms are written to. this._telemetry was no longer defined because of the dark theme telemetry test backout that was a prerequisite to this code until very recently. I will fix the dark theme telemetry as part of bug 1091796.

Really quick review... instantiated this._telemetry and changed tab histograms to linear.

Comment on attachment 8579261 [details] [diff] [review] 1144363-telemetry-error.patch Review of attachment 8579261 [details] [diff] [review]: ----------------------------------------------------------------- r+ because it obviously fixes the error, but what's going on with the tests? Is there a bug on file to enable them?

(In reply to Brian Grinstead [:bgrins] from comment #6) > Comment on attachment 8579261 [details] [diff] [review] > 1144363-telemetry-error.patch > > Review of attachment 8579261 [details] [diff] [review]: > ----------------------------------------------------------------- > > r+ because it obviously fixes the error, but what's going on with the tests? > Is there a bug on file to enable them? All of the current telemetry tests are hugely buggy and don't even detect when no telemetry is logged at all. Bug 1098374 is about accessing the actual telemetry data and reading the real results rather that relying on monkeypatching our telemetry module. The problem is that the patch that fixes this causes aborts in histogram.cc e.g. 10:37:35 INFO - [1945] ###!!! ABORT: file /builds/slave/fx-team-l64-d-0000000000000000/build/src/ipc/chromium/src/base/histogram.cc, line 732 Seems like a platform bug but it is on me to narrow it down and write a simple test case. Of course, it only happens on try.