Closed Bug 1122480 Opened 5 years ago Closed 5 years ago

about:telemetry needs to be updated.

Categories

(Firefox Health Report Graveyard :: Client: Desktop, defect)

defect
Not set

Tracking

(firefox39 wontfix, firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: Dexter, Assigned: gfritzsche)

References

Details

(Whiteboard: [uplift])

Attachments

(15 files, 7 obsolete files)

108.07 KB, image/png
Details
6.27 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
1.23 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
13.29 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
3.34 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
7.45 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
9.85 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
4.63 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
9.16 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
14.47 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
12.32 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
6.62 KB, patch
rvitillo
: review+
Details | Diff | Splinter Review
1.24 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
6.37 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
60.26 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
We will need to update about:telemetry for it to keep working. We need to account for the new telemetry date migrated from FHR and the environment data.
No longer depends on: 1122479
1) Add a new "Environment" section with the data from TelemetryEnvironment.getEnvironmentData()
This could work similar to the "General Data" section here, filling the section dynamically with all the environment fields:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.js#l112
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.xhtml#l38

2) Add a "Subsession Histograms" section.
That would duplicate the histogram handling here:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.js#l1004
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.xhtml#l81
... but use Telemetry.snapshotSubsessionHistograms(false) instead of Telemetry.histogramSnapshots.

3) Add a "Subsession Keyed Histogram" section.
That would mostly duplicate the keyed histogram handling here:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.js#l1020
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/content/aboutTelemetry.xhtml#l93

... but instead of Telemetry.keyedHistogramSnapshots use the approach here to collect the keyed histogram data:
https://hg.mozilla.org/mozilla-central/annotate/df3daecd381f/toolkit/components/telemetry/TelemetrySession.jsm#l694
Whiteboard: [help]
Duplicate of this bug: 1139753
The rough plan here is to
a) keep this working for the current use as a debugging/development tool showing "current" measurements
b) allow it to also display the archived/retained ping data

So we need to:
* add the sections from comment 1
* show the new subsession sections only for the "current" data (as archived pings only contain either subsession or "classic" data)
* default to showing the "current" data
* allow users to pick older pings to display in the view
Depends on: 1150134
We should also deal with the "Disable Telemetry" button in this page. We could either:

- Remove the button
- Keep the button, making it disable FHR
- Keep the button, making it open the "Preferences" page
Assignee: nobody → gfritzsche
The most straight-forward approach i'm taking here is to make this page always work from the data of a ping.
For viewing the current data we can just collect a ping with the most recent data.
That way the handling for the current & archived data is exactly the same and we just feed it different pings.
Status: NEW → ASSIGNED
Attached image Updated settings
For updating the "toggle telemetry" button i opted for making the basic settings conveniently available.
This is how it looks now (...not planning to win design contest).

Benjamin, Vladan, does that look ok from a privacy and product collective?
I.e. making it obvious enough what the status is?
Flags: needinfo?(vdjeric)
Flags: needinfo?(benjamin)
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [help]
Version: unspecified → Trunk
Comment on attachment 8603478 [details]
Updated settings

I think we should get rid of this and just link to the prefs pane. Too many places to keep in sync with different UI is going to be a pain to maintain.
Flags: needinfo?(benjamin)
Fair point, i'll just keep the settings display for now.

Jared, i see that i can link to "about:preferences#advanced", but can i directly link so that the data choices panel opens there?
Flags: needinfo?(vdjeric) → needinfo?(jaws)
Attachment #8603836 - Flags: review?(rvitillo)
Outstanding parts coming in separate patches:
* linking to the data choices preferences
* choosing an archived ping to display
* when the current ping data is displayed, allow switching between subsession & "old style" data
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Fair point, i'll just keep the settings display for now.
> 
> Jared, i see that i can link to "about:preferences#advanced", but can i
> directly link so that the data choices panel opens there?

We don't have a way to create a URL to link to a tab within the Advanced preferences. openAdvancedPreferences() does however exist within utilityOverlay.js which can be used to open the preferences to a given advanced tab, but that is not available through a link.

In the long term we will remove preference tabboxes from the preferences.
Flags: needinfo?(jaws)
Attachment #8603829 - Flags: review?(rvitillo) → review+
Attachment #8603830 - Flags: review?(rvitillo) → review+
Attachment #8603831 - Flags: review?(rvitillo) → review+
Comment on attachment 8603832 [details] [diff] [review]
Part 4 - Make about:telemetry render functions work from a ping object

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

::: toolkit/content/aboutTelemetry.js
@@ +87,5 @@
>  let GeneralData = {
>    /**
>     * Renders the general data
>     */
> +  render: function(ping) {

The ping parameter is not used in the function.

@@ +130,5 @@
>  let TelLog = {
>    /**
>     * Renders the telemetry log
>     */
> +  render: function(ping) {

s/ping/aPing

@@ +192,5 @@
>  
>    /**
>     * Render slow SQL statistics
>     */
> +  render: function SlowSQL_render(ping) {

s/ping/aPing

@@ +197,3 @@
>      let debugSlowSql = Preferences.get(PREF_DEBUG_SLOW_SQL, false);
>      let {mainThread, otherThreads} =
> +      debugSlowSql ? Telemetry.debugSlowSQL : ping.payload.slowSQL;

Can we store debugSlowSQL within the ping?

@@ +971,1 @@
>        let req = new SymbolicationRequest("chrome-hangs",

There are multiple calls to getCurrentPingData() in the file. To parametrize aboutTelemetry on a ping object, all functions should work on the same ping object. If that would be the case one could e.g. use aboutTelemetry to display a custom/older ping, which would be helpful for debugging purposes.

@@ +976,5 @@
>  
>    document.getElementById("chrome-hangs-hide-symbols").addEventListener("click",
>      function () {
> +      let ping = TelemetryController.getCurrentPingData(false);
> +      ChromeHangs.render(ping);

See comment above.

@@ +981,5 @@
>    }, false);
>  
>    document.getElementById("late-writes-fetch-symbols").addEventListener("click",
>      function () {
> +      let ping = TelemetryController.getCurrentPingData(false);

See comment above.

@@ +992,5 @@
>    }, false);
>  
>    document.getElementById("late-writes-hide-symbols").addEventListener("click",
>      function () {
> +      let ping = TelemetryController.getCurrentPingData(false);

See comment above.
Attachment #8603832 - Flags: review?(rvitillo) → review-
Comment on attachment 8603829 [details] [diff] [review]
Part 1 - Allow collecting the current ping data from TelemetryController

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

This patch breaks the about:telemetry tab. The enable/disable telemetry button is not rendered properly, i.e. the button is empty and the labels "Telemetry is enabled" and "Telemetry is disabled" appear on top of it.
Attachment #8603829 - Flags: review+ → review-
Georg, another important part missing in comment 17 is displaying children payloads as well, possibly aggregated. Do you have already a plan for that?
Flags: needinfo?(gfritzsche)
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #21)
> Georg, another important part missing in comment 17 is displaying children
> payloads as well, possibly aggregated. Do you have already a plan for that?

I'm not handling this as part of this bug and don't think i have time for anything like that until unified Telemetry ships.
Do we have a bug for that yet? We should figure out how we want to display the data so it's actionable.
Flags: needinfo?(gfritzsche)
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #20)
> Comment on attachment 8603829 [details] [diff] [review]
> Part 1 - Allow collecting the current ping data from TelemetryController
> 
> Review of attachment 8603829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch breaks the about:telemetry tab. The enable/disable telemetry
> button is not rendered properly, i.e. the button is empty and the labels
> "Telemetry is enabled" and "Telemetry is disabled" appear on top of it.

I don't know why that is, but the patches are not supposed to work independently.
With all the patches applied, the button is gone.
Attachment #8603829 - Flags: review- → review?(rvitillo)
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #19)
> Comment on attachment 8603832 [details] [diff] [review]
> Part 4 - Make about:telemetry render functions work from a ping object
> 
> Review of attachment 8603832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/aboutTelemetry.js
> @@ +87,5 @@
> >  let GeneralData = {
> >    /**
> >     * Renders the general data
> >     */
> > +  render: function(ping) {
> 
> The ping parameter is not used in the function.

Ah, yes, that is an artifact of how i broke this into separate patches. It is used in part 6.
Let's ignore this here?

> @@ +197,3 @@
> >      let debugSlowSql = Preferences.get(PREF_DEBUG_SLOW_SQL, false);
> >      let {mainThread, otherThreads} =
> > +      debugSlowSql ? Telemetry.debugSlowSQL : ping.payload.slowSQL;
> 
> Can we store debugSlowSQL within the ping?

Not without bigger changes - in general we don't want to store that data in the ping because AFAICT its privacy sensitive and shouldn't get sent out.
So we would need to take care of only collecting that for this path here.
The easier approach for now is to just show the debug data here if we show the "current" ping data and the pref is set.
> > Can we store debugSlowSQL within the ping?
> 
> Not without bigger changes - in general we don't want to store that data in
> the ping because AFAICT its privacy sensitive and shouldn't get sent out.
> So we would need to take care of only collecting that for this path here.
> The easier approach for now is to just show the debug data here if we show
> the "current" ping data and the pref is set.

Even if the data is stored in the ping it doesn't need to be sent out. If the change requires too much effort can we add a comment to clear this up?
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #25)
> > > Can we store debugSlowSQL within the ping?
> > 
> > Not without bigger changes - in general we don't want to store that data in
> > the ping because AFAICT its privacy sensitive and shouldn't get sent out.
> > So we would need to take care of only collecting that for this path here.
> > The easier approach for now is to just show the debug data here if we show
> > the "current" ping data and the pref is set.
> 
> Even if the data is stored in the ping it doesn't need to be sent out. If
> the change requires too much effort can we add a comment to clear this up?

Sure, i will do that.
(In reply to Georg Fritzsche [:gfritzsche] from comment #23)
> (In reply to Roberto Agostino Vitillo (:rvitillo) from comment #20)
> > Comment on attachment 8603829 [details] [diff] [review]
> > Part 1 - Allow collecting the current ping data from TelemetryController
> > 
> > Review of attachment 8603829 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This patch breaks the about:telemetry tab. The enable/disable telemetry
> > button is not rendered properly, i.e. the button is empty and the labels
> > "Telemetry is enabled" and "Telemetry is disabled" appear on top of it.
> 
> I don't know why that is, but the patches are not supposed to work
> independently.
> With all the patches applied, the button is gone.

FWIW, this works fine for me with only Part 1 applied, so there is nothing odd with that patch.
Anyway, things should definitely work fine with the full stack applied.
Attachment #8603832 - Attachment is obsolete: true
Attachment #8603835 - Attachment is obsolete: true
Attachment #8603835 - Flags: review?(rvitillo)
Attachment #8603834 - Flags: review?(rvitillo) → review+
Rebased.
Attachment #8604056 - Flags: review?(rvitillo)
Attachment #8603836 - Attachment is obsolete: true
Attachment #8603836 - Flags: review?(rvitillo)
Attachment #8603837 - Attachment is obsolete: true
Attachment #8603837 - Flags: review?(rvitillo)
Attachment #8604053 - Flags: review?(rvitillo) → review+
(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> (In reply to Roberto Agostino Vitillo (:rvitillo) from comment #21)
> > Georg, another important part missing in comment 17 is displaying children
> > payloads as well, possibly aggregated. Do you have already a plan for that?
> 
> I'm not handling this as part of this bug and don't think i have time for
> anything like that until unified Telemetry ships.
> Do we have a bug for that yet? We should figure out how we want to display
> the data so it's actionable.

Afaik we don't have a bug on file yet.
Link to the data choices pane in the prefences.
Attachment #8604065 - Flags: review?(rvitillo)
Comment on attachment 8604055 [details] [diff] [review]
Part 6 - Expand the 'General Data' section to show the common ping data

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

r+ with my concern addressed

::: toolkit/content/aboutTelemetry.js
@@ +52,5 @@
>    return (documentRTLMode == "rtl");
>  }
>  
> +function isArray(arg) {
> +  return Object.prototype.toString.call(arg) === '[object Array]';

Does the Javascript standard mandate that the string representation of array and object are "[object Array]" and "object" respectively?

@@ +59,5 @@
> +function isFlatArray(obj) {
> +  if (!isArray(obj)) {
> +    return false;
> +  }
> +  return !obj.some(e => typeof(e) == "object");

See comment above.
Attachment #8604055 - Flags: review?(rvitillo) → review+
Rebased on current m-c.
Attachment #8604099 - Flags: review?(rvitillo)
Attachment #8603829 - Attachment is obsolete: true
Attachment #8603829 - Flags: review?(rvitillo)
Comment on attachment 8604056 [details] [diff] [review]
Part 7 - Add an environment data section

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

::: toolkit/content/aboutTelemetry.js
@@ +106,5 @@
> +/**
> + * This turns a JSON object into a "flat" stringified form, separated into top-level sections.
> + *
> + * For an object like {a: {b: "1"}, c: {d: "2", e: {f: "3"}}} it returns a Map
> + * of the form Map(["a", Map(["b","1"]]), ["c", Map(["d", "2"], ["e.f", "3"])]).

Parentheses don't match.
Attachment #8604056 - Flags: review?(rvitillo) → review+
Attachment #8604099 - Flags: review?(rvitillo) → review+
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #34)
> Comment on attachment 8604055 [details] [diff] [review]
> Part 6 - Expand the 'General Data' section to show the common ping data
> 
> Review of attachment 8604055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with my concern addressed
> 
> ::: toolkit/content/aboutTelemetry.js
> @@ +52,5 @@
> >    return (documentRTLMode == "rtl");
> >  }
> >  
> > +function isArray(arg) {
> > +  return Object.prototype.toString.call(arg) === '[object Array]';
> 
> Does the Javascript standard mandate that the string representation of array
> and object are "[object Array]" and "object" respectively?

Yes:
* "object": http://www.ecma-international.org/ecma-262/5.1/#sec-11.4.3
* "[object Array]" from obj.prototype: http://www.ecma-international.org/ecma-262/5.1/#sec-15.2.4.2
Comment on attachment 8604057 [details] [diff] [review]
Part 8 - Update the settings: show upload & extended recording

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

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ +9,1 @@
>  ">

In the preferences panel we explicitly mention FHR, would be nice to do the same here to be consistent.

@@ +9,4 @@
>  ">
>  
> +<!ENTITY aboutTelemetry.uploadDisabled "
> +  Data upload is <span>disabled</span>.

Same as above.
Attachment #8604057 - Flags: review?(rvitillo) → review+
Attachment #8604065 - Flags: review?(rvitillo) → review+
Keywords: leave-open
Try push with xpcshell fix (thanks Alessio) and the missing patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb35aa8c8599
Flags: needinfo?(gfritzsche)
Blocks: 1163971
(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #32)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> > (In reply to Roberto Agostino Vitillo (:rvitillo) from comment #21)
> > > Georg, another important part missing in comment 17 is displaying children
> > > payloads as well, possibly aggregated. Do you have already a plan for that?
> > 
> > I'm not handling this as part of this bug and don't think i have time for
> > anything like that until unified Telemetry ships.
> > Do we have a bug for that yet? We should figure out how we want to display
> > the data so it's actionable.
> 
> Afaik we don't have a bug on file yet.

Filed bug 1163971.
Attachment #8604598 - Flags: review?(rvitillo) → review?(alessio.placitelli)
Attachment #8604598 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8604595 [details] [diff] [review]
Part 10 - Allow switching between current and archived ping data

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

::: toolkit/content/aboutTelemetry.js
@@ +275,5 @@
> +            .addEventListener("click", () => this._movePingIndex(1), false);
> +  },
> +
> +  detachObservers: function() {
> +

Why is this function empy?

@@ +310,5 @@
> +      this._archivedPings = pingList;
> +
> +      // Collect the start dates for all the weeks we have pings for.
> +      let weekStart = (date) => {
> +        let weekDay = (date.getDay() + 13) % 7;

Please use +6 instead of +13.

@@ +344,5 @@
> +  },
> +
> +  _renderWeeks: function() {
> +    let weekSelector = document.getElementById("choose-ping-week");
> +    removeAllChildNodes(weekSelector);

removeAllChildNodes is not defined.
Attachment #8604595 - Flags: review?(rvitillo) → review+
Comment on attachment 8604596 [details] [diff] [review]
Part 11 - Make ping data renderers work properly when switching pings

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

::: toolkit/content/aboutTelemetry.js
@@ +433,5 @@
>     * Renders the general data
>     */
>    render: function(aPing) {
>      setHasData("general-data-section", true);
> +    removeAllChildNodes

What is this doing here?
Attachment #8604596 - Flags: review?(rvitillo) → review+
As discussed on IRC, after applying the full set of patches the following error appears:

TypeError: reduce of empty array with no initial value aboutTelemetry.js:998:25

after e.g. disabling a plugin and forcing a new subsession with:

(function() { Cu.import("resource://gre/modules/TelemetryEnvironment.jsm").gGlobalEnvironment._lastEnvironmentChangeDate = null; })()
Fixed histograms with no values recorded breaking the histogram rendering. We now always show them with no values displayed.
Attachment #8603834 - Attachment is obsolete: true
Attachment #8605243 - Flags: review+
Attachment #8604597 - Flags: review?(rvitillo) → review+
This looks pretty green with an Android test-fix deferred:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2cd0466ebfd
(In reply to Georg Fritzsche [:gfritzsche] from comment #51)
> This looks pretty green with an Android test-fix deferred:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2cd0466ebfd

That Android failure being the X3 here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51e1db4a0da5
Whiteboard: [uplift]
Ok, i think we have to revisit the single Android test issue later.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Depends on: 1165359
Blocks: 1165968
Depends on: 1167166
Target Milestone: --- → Firefox 41
Folded the patches here for uplift to avoid requests on 13 individual patches.
Attachment #8623877 - Flags: review+
Comment on attachment 8623877 [details] [diff] [review]
Fx40 - Update about:telemetry to the unification changes

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8623877 - Flags: approval-mozilla-aurora?
This hard-codes the strings for about:telemetry per bug 1167166, comment 5 ff.
Attachment #8623877 - Attachment is obsolete: true
Attachment #8623877 - Flags: approval-mozilla-aurora?
Comment on attachment 8624182 [details] [diff] [review]
Fx40 - Update about:telemetry to the unification changes

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page, which are hard-coded in english for the uplift.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.
Attachment #8624182 - Flags: review+
Attachment #8624182 - Flags: approval-mozilla-aurora?
Attachment #8624182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1178768
No longer depends on: 1165359
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.