1120408 - about:telemetry should display the TelemetryLog data

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

Bug 981851 added Telemetry logging, but we don't show that data in about:telemetry yet.

Comment 1

[Manraj Singh [:manrajsingh]]

Hi

I would like to this this up. Please assign this to me and help me in going about it.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Hello Manraj, thanks for your interest in working on this bug.

To get started, you will need to download the source code using Mercurial, and then compile Firefox. Which operating system are you using?

This page here has instructions on how to do that: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

If you have any trouble on doing this, let me know and I can help.


After you have successfully compiled Firefox and run it (with `mach build` and `mach run`), you're ready to start modifying the source code to implement this feature.

The 3 relevant files that are most involved with this bug are:
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.xhtml
https://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryLog.jsm

I'll help you further on figuring out how and what to modify once you finish the initial setup and compilation.

Comment 3

[Manraj Singh [:manrajsingh]]

Hi sir :)

Sir I have already set up the build environment.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Ok great!  First, you will need to learn how to get the data out of TelemetryLog, and in which format it is.
To experiment with JS code in Firefox, you can use the Browser Console to do that. Some instructions:
https://developer.mozilla.org/en-US/docs/Tools/Browser_Console

From the browser console, if you import the TelemetryLog.jsm file like so:
Cu.import("resource://gre/modules/TelemetryLog.jsm")

You'll have access to the TelemetryLog object. You will see that it is quite simple, and the function you need to call is entries(), which will return an array of entries (each entry itself is an array of strings and numbers).

After you get familiar with it, you'll need to add a new section in the aboutTelemetry.xhtml file, a function to fill this section in aboutTelemetry.js, and a click listener to call that function, to be added here: https://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js#922

After you make changes, you should be able to recompile with `mach build toolkit/content`, which will be a bit faster than re-doing the full build

Comment 5

[Manraj Singh [:manrajsingh]]

Hi sir :)

Okay sir I was able to extract the array and was looking at it. Its a 2D array with currently 3 columns. I would like to know if this would get extended to further columns in future both in rows and columns or if its static in size and dynamic in content.Also this gets added as another option right?

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Hello.

Actually the logging API supports an arbitrary number of columns, it just depends on whoever logs the messages. So you need to support displaying an arbitrary number of columns too. The API is like this: the first column is the "name" of the message, and all other columns are random data related to it. For example, it's used as:

TelemetryLog.log("MyEvent", ["install", 1000, "foo", "bar"]);

If you're creating a table, what you could do is to first iterate through all messages to discover what is the largest number of columns, and then go from there.

I'm not sure what you mean by "this gets added as another option". But if you're asking if this should have an option in about:config, the answer is no, it's not needed. This section should always be visible in about:telemetry.

Comment 7

[Manraj Singh [:manrajsingh]]

Hi sir :)

Okay sir, but just adding arbitrary number of columns to table might not look good on the page, unless we make the table scrollable.

No sir, what I meant is current about:telemetry has options/sections of "General Data", "Slow SQL statements" etc. We need to add another option/section for Telemetry log, right?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

(In reply to Manraj Singh [:manrajsingh] from comment #7)
> Hi sir :)
> 
> Okay sir, but just adding arbitrary number of columns to table might not
> look good on the page, unless we make the table scrollable.

This is no problem. It's unlikely that someone will be logging an unreasonable number of entries, and this is a page meant for debugging. Besides, the section comes closed by default. So it's not necessary to write any more complicated code to handle the case for many columns.

> 
> No sir, what I meant is current about:telemetry has options/sections of
> "General Data", "Slow SQL statements" etc. We need to add another
> option/section for Telemetry log, right?

Yeah, that is correct. You'll need to add this section in aboutTelemetry.xhtml

Comment 9

[Manraj Singh [:manrajsingh]]

Attachment: Need help. Telemetry Log not showing up.

Hi Sir,

First of all sorry for being late and delaying this. I got caught up in my exams hence couldn't work on it. Now that I am free, I have tried fixing it but I am not able to reproduce the results. I notice "TelemetryLog" is not working since it is not even getting "has-data" class. Am I missing something? I am attaching the patch file. Please help.

Comment 10

[Manraj Singh [:manrajsingh]]

Please ignore the "console.log"s

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Manraj Singh [:manrajsingh | Exams Away: March 5-13] from comment #9)
> First of all sorry for being late and delaying this. I got caught up in my
> exams hence couldn't work on it. Now that I am free, I have tried fixing it
> but I am not able to reproduce the results. I notice "TelemetryLog" is not
> working since it is not even getting "has-data" class. Am I missing
> something?

You are not triggering |TelemetryLog.render()| anywhere. See e.g. |GeneralData.render()| for an example.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8579908 [details] [diff] [review]
Need help. Telemetry Log not showing up.

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

::: toolkit/content/aboutTelemetry.js
@@ +11,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/TelemetryTimestamps.jsm");
>  Cu.import("resource://gre/modules/TelemetryPing.jsm");
>  Cu.import("resource://gre/modules/TelemetrySession.jsm");
> +Cu.import("resource://gre/modules/TelemetryLog.jsm")

This is missing a ";" at the end of the line.

@@ +115,5 @@
>     * Renders the general data
>     */
>    render: function() {
>      setHasData("general-data-section", true);
> +	

This whitespace change is unneeded.

@@ +171,5 @@
> +    let headings = document.createElement("tr");
> +    this.appendColumn(headings, "th", bundle.GetStringFromName("telemetryLogHeadingName") + "\t");
> +    table.appendChild(headings);
> +	console.log("reached finally");
> +    for (let i = 0; i < entriesCount; ++i) {

You can just use something like:
for (let entry of entries) {
  ...
  for (let elem of entries) {
    ...

Comment 13

[Manraj Singh [:manrajsingh]]

Attachment: Fixed bug 112048 Show TelemetryLog in about:telemetry

Sir

I have made the changes and fixed the bug. Please review and let me know the changes, if any.

Comment 14

[Manraj Singh [:manrajsingh]]

Bug 1120408* Sorry for the typo.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Great - if you think the patch is done, you can set the review? flag on the patch instead of needinfo:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 16

[Manraj Singh [:manrajsingh]]

Attachment: Fixed bug 1120408 Show TelemetryLog in about:telemetry

Sir,

Please review the patch.Thank you.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8579987 [details] [diff] [review]
Fixed bug 1120408 Show TelemetryLog in about:telemetry

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

Awesome! I tested your patch and it's working really well! I just have one request for change, and then it's done :)

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

this blank line was removed accidentally.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +14,5 @@
>  generalDataHeadingValue = Value
>  
> +telemetryLogTitle = Telemetry Log
> +
> +telemetryLogHeadingName = Name

To follow the naming convention of TelemetryLog.jsm, can you make 3 caption columns (instead of two), and call them: Id, Timestamp, Data?

Comment 18

[Manraj Singh [:manrajsingh]]

Attachment: Fixed bug 1120408 Show TelemetryLog in about:telemetry. Final Changes.

Hi Sir, :)

Made the final changes. Please review.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8580305 [details] [diff] [review]
Fixed bug 1120408 Show TelemetryLog in about:telemetry. Final Changes.

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

Thank you!

Comment 20

[Manraj Singh [:manrajsingh]]

Hi Sir,

Please land this fix. Thank you. :)

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Yep, the trees are closed at the moment, but I've got the patch ready to land once it reopens

Comment 22

[Manraj Singh [:manrajsingh]]

A request sir. Could you please vouch for me so that I can access Mozillians.org content? This is my second bug fix. Thank you. :)

https://mozillians.org/en-US/u/manrajsingh/

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Vouched!

https://hg.mozilla.org/integration/fx-team/rev/6f2e247fb7b7

Comment 24

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

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

Comment 25

[Manraj Singh [:manrajsingh]]

Thank you sir. :)