Closed Bug 942280 Opened 7 years ago Closed 3 years ago

Add section for UI Telemetry

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: liuche, Unassigned)

References

Details

Attachments

(2 files, 2 obsolete files)

Add a section in about:telemetry for displaying UI Telemetry.
This is actually useful if you apply patches that add probes (like from bug 942279).
Attachment #8337144 - Flags: review?(mark.finkle)
Screenshot of the section.
Attachment #8337144 - Attachment is patch: true
Comment on attachment 8337144 [details] [diff] [review]
Patch: add UI Telemetry section to about:telemetry

Again, this looks OK. I'd rename UiMeasurements -> UIMeasurements before I land, but I want a second set of eyes on it.
Attachment #8337144 - Flags: review?(vdjeric)
Attachment #8337144 - Flags: review?(mark.finkle)
Attachment #8337144 - Flags: review+
Comment on attachment 8337144 [details] [diff] [review]
Patch: add UI Telemetry section to about:telemetry

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

I took a look at your screenshot, are you recording epoch times in these probes? Do we really need those? We might need to get an opinion from privacy people

::: toolkit/content/aboutTelemetry.js
@@ +647,5 @@
> +  render: function UiMeasurements_render(aData) {
> +    let uiSection = document.getElementById("ui-measurements");
> +    let measurements = [];
> +
> +    aData.forEach(function(item) {

I think these forEach iterations would be a bit more readable with a for..of loop:

for ([key, value] of Iterator(aData)) {
  ...
}

or

for (key of Object.keys(aData)) {
  ...
}
Attachment #8337144 - Flags: review?(vdjeric)
Flags: needinfo?(nobody)
Flags: needinfo?(liuche)
Flags: needinfo?(nobody)
(In reply to Vladan Djeric (:vladan) from comment #4)
> Comment on attachment 8337144 [details] [diff] [review]
> Patch: add UI Telemetry section to about:telemetry
> 
> Review of attachment 8337144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I took a look at your screenshot, are you recording epoch times in these
> probes? Do we really need those? We might need to get an opinion from
> privacy people

With UI telemetry we need to know the sequence of events. We need to know if an event occurred during a specific session.

We'd be open to other forms of collecting the information, but we need to collect enough information to be able to perform some useful analysis.

> ::: toolkit/content/aboutTelemetry.js
> @@ +647,5 @@
> > +  render: function UiMeasurements_render(aData) {
> > +    let uiSection = document.getElementById("ui-measurements");
> > +    let measurements = [];
> > +
> > +    aData.forEach(function(item) {
> 
> I think these forEach iterations would be a bit more readable with a for..of
> loop:
> 
> for ([key, value] of Iterator(aData)) {
>   ...
> }
> 
> or
> 
> for (key of Object.keys(aData)) {
>   ...
> }

I can make that change and rename the object
Flags: needinfo?(liuche)
Did you have cycles to make those changes and land the patch, mfinkle? If not, let me know, and I can finish it.
Flags: needinfo?(mark.finkle)
(In reply to Mike Conley (:mconley) from comment #6)
> Did you have cycles to make those changes and land the patch, mfinkle? If
> not, let me know, and I can finish it.

Mike - Please take this if you can. I am working on the Java probe side in bug 932092.
Flags: needinfo?(mark.finkle)
Sure, on it.
Assignee: nobody → mconley
Attached patch Patch v1.1 (r+'d by mfinkle) (obsolete) — Splinter Review
I've updated UiTelemetry -> UITelemetry, and I've switched to good ol' for...of loops instead of forEach.

What do you think of this, vladan?
Attachment #8337144 - Attachment is obsolete: true
Attachment #8339512 - Flags: review?(vdjeric)
Comment on attachment 8339512 [details] [diff] [review]
Patch v1.1 (r+'d by mfinkle)

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

Drive-by review, at the request of mconley.

::: toolkit/content/aboutTelemetry.js
@@ +642,5 @@
> +  tableValueTitle: bundle.GetStringFromName("uiTableValue"),
> +
> +  /**
> +   * Render the UI measurements section as a single JSON blob.
> +   */

Please document the type and format of aData.

@@ +648,5 @@
> +    let uiSection = document.getElementById("ui-measurements");
> +    let measurements = [];
> +
> +    for (let item of aData) {
> +      let aKey = item.type;

Please keep the "a" prefix for arguments.

@@ +659,5 @@
> +          aValue[property] = item[property];
> +        }
> +      }
> +
> +      measurements.push([aKey, JSON.stringify(aValue)]);

How large do you expect |aData| to be? If it's very large, you might wish to delay stringification to a later ping.

@@ +663,5 @@
> +      measurements.push([aKey, JSON.stringify(aValue)]);
> +    }
> +
> +    // Use a custom generator because our types have redundancies.
> +    function MeasurementIterator() {};

Defining a new constructor/prototype for a single instance looks a bit overkill.

@@ +664,5 @@
> +    }
> +
> +    // Use a custom generator because our types have redundancies.
> +    function MeasurementIterator() {};
> +    MeasurementIterator.prototype.__iterator__ = function() {

__iterator__ is deprecated, you should now use @@iterator

@@ +667,5 @@
> +    function MeasurementIterator() {};
> +    MeasurementIterator.prototype.__iterator__ = function() {
> +      for (let i in measurements) {
> +        yield measurements[i];
> +      }

I don't understand why you cannot simply pass |measurements|.
Attachment #8339512 - Flags: review?(vdjeric) → feedback+
With bug 945499 fixed, the Australis team no longer requires the UI Telemetry section for the event logs.

The Fennec team might still want it, but I'll let them push it through. Unassigning self.
Assignee: mconley → nobody
Comment on attachment 8339512 [details] [diff] [review]
Patch v1.1 (r+'d by mfinkle)

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

::: toolkit/content/aboutTelemetry.js
@@ +664,5 @@
> +    }
> +
> +    // Use a custom generator because our types have redundancies.
> +    function MeasurementIterator() {};
> +    MeasurementIterator.prototype.__iterator__ = function() {

I'm not sure what @@iterator is - can you point me to a reference?

@@ +667,5 @@
> +    function MeasurementIterator() {};
> +    MeasurementIterator.prototype.__iterator__ = function() {
> +      for (let i in measurements) {
> +        yield measurements[i];
> +      }

renderBody creates an Iterator of aMeasurements, which returns the indices of the measurements array as keys - perhaps I could change it to only take an Iterable object?
Flags: needinfo?(dteller)
Addressed some feedback comments, left questions on previous review for others.
Attachment #8339512 - Attachment is obsolete: true
(In reply to Chenxia Liu [:liuche] from comment #12)
> Comment on attachment 8339512 [details] [diff] [review]
> Patch v1.1 (r+'d by mfinkle)
> 
> Review of attachment 8339512 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/content/aboutTelemetry.js
> @@ +664,5 @@
> > +    }
> > +
> > +    // Use a custom generator because our types have redundancies.
> > +    function MeasurementIterator() {};
> > +    MeasurementIterator.prototype.__iterator__ = function() {
> 
> I'm not sure what @@iterator is - can you point me to a reference?

Tutorial: http://domenic.me/2013/09/06/es6-iterators-generators-and-iterables/
Reference: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-iterator-interface

> @@ +667,5 @@
> > +    function MeasurementIterator() {};
> > +    MeasurementIterator.prototype.__iterator__ = function() {
> > +      for (let i in measurements) {
> > +        yield measurements[i];
> > +      }
> 
> renderBody creates an Iterator of aMeasurements, which returns the indices
> of the measurements array as keys - perhaps I could change it to only take
> an Iterable object?

I'm not sure I understand why you cannot simply rewrite your code to make |measurements| into a non-array object. Is it because you could have several instances of each |key|?
Flags: needinfo?(dteller)
> Tutorial:
> http://domenic.me/2013/09/06/es6-iterators-generators-and-iterables/
> Reference:
> http://people.mozilla.org/~jorendorff/es6-draft.html#sec-iterator-interface

Thanks!

> I'm not sure I understand why you cannot simply rewrite your code to make
> |measurements| into a non-array object. Is it because you could have several
> instances of each |key|?

Correct - the keys currently are the type of message. However, I think it might be worthwhile to revisit the intent of this section - all this contortion points to KeyValueTable not being the right way to display UI Telemetry events.

Something like what the "Slow SQL Statements" table does actually seems way, way better.
This isn't really a particularly useful section to have on about:telemetry, especially since most of the questions we'd like to answer will require correlation between sessions and events.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
So what's going on with this bug, exactly? What part is WONTFIXed?
Flags: needinfo?(nobody)
We decided that looking at a raw stream of event/session data was not useful. The entire bug is WONTFIX.
Flags: needinfo?(nobody)
IMO we should reopen this bug. Having a section in about:telemetry may not be useful to devs, but it's important for letting users know what kind of information is being sent through telemetry. In the interest of full disclosure, we shouldn't leave out UI telemetry.

Mark, what do you think?
Flags: needinfo?(mark.finkle)
(In reply to Jim Chen [:jchen :nchen] from comment #19)
> IMO we should reopen this bug. Having a section in about:telemetry may not
> be useful to devs, but it's important for letting users know what kind of
> information is being sent through telemetry. In the interest of full
> disclosure, we shouldn't leave out UI telemetry.
> 
> Mark, what do you think?

Yeah. That's a good point. I'll reopen.

If the current idea of a raw dump becomes a problem for other reasons (performance or memory) we can re-evaluate the format of the data being displayed.
Status: RESOLVED → REOPENED
Flags: needinfo?(mark.finkle)
Resolution: WONTFIX → ---
UI Telemetry has been supplanted by Event Telemetry which is displayed by the redesigned about:telemetry.
Status: REOPENED → RESOLVED
Closed: 7 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.