Closed Bug 1144363 Opened 6 years ago Closed 6 years ago

There are telemetry logs defined in gDevTools that never fire and appear invalid

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: bgrins, Assigned: miker)

References

Details

Attachments

(1 file)

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
Flags: needinfo?(mratcliffe)
Flags: needinfo?(jeff)
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
Blocks: 1046234
Ouch, this is due to a last minute change... fixing.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
(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.
Flags: needinfo?(jeff)
Really quick review... instantiated this._telemetry and changed tab histograms to linear.
Attachment #8579261 - Flags: review?(bgrinstead)
Blocks: 1091796
No longer blocks: 1046234
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?
Attachment #8579261 - Flags: review?(bgrinstead) → review+
(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.
Duplicate of this bug: 1146013
Summary: There are telelmetry logs defined in gDevTools that never fire and appear invalid → There are telemetry logs defined in gDevTools that never fire and appear invalid
https://hg.mozilla.org/mozilla-central/rev/4dfa4f306220
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.