1117883 - Explicitly report in Telemetry whether asyncPluginInit feature is enabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Vladan Djeric (:vladan)]

Telemetry pings should report the value of the dom.ipc.plugins.asyncInit preference. This will make it determine the benefit of the async plugin init feature (bug 998863) by analyzing Telemetry. This analysis will be done in a different bug

How to get started:

1. Take a look at about:telemetry to get an idea of the type of data reported by Telemetry
2. Familiarize yourself with the "getMetadata" function in TelemetryPing.jsm. This is where the change should be made:
http://hg.mozilla.org/mozilla-central/annotate/912036eeb024/toolkit/components/telemetry/TelemetryPing.jsm#l933
3. Learn how preferences are read from Javascript by getting a basic understanding of Preferences.jsm or looking at examples of its usage in TelemetryPing.jsm

http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm

Comment 1

[Abhishek Bhattacharya [:babhishek21]]

(In reply to Vladan Djeric (:vladan) from comment #0)
> Telemetry pings should report the value of the dom.ipc.plugins.asyncInit
> preference. This will make it determine the benefit of the async plugin init
> feature (bug 998863) by analyzing Telemetry. This analysis will be done in a
> different bug

I would like to work on this bug. I am new to FOSS, so need some help and mentoring.

> How to get started:
> 
> 1. Take a look at about:telemetry to get an idea of the type of data
> reported by Telemetry
Done!
> 2. Familiarize yourself with the "getMetadata" function in
> TelemetryPing.jsm. This is where the change should be made:
> http://hg.mozilla.org/mozilla-central/annotate/912036eeb024/toolkit/
> components/telemetry/TelemetryPing.jsm#l933
I understand the `getMetadata` function. However I don't understand what exactly is required. 
Am I supposed to `let` a local variable that uses Preferences.get()? Or am I required to write a separate method?
> 3. Learn how preferences are read from Javascript by getting a basic
> understanding of Preferences.jsm or looking at examples of its usage in
> TelemetryPing.jsm
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Preferences.jsm
Basic understanding. This looks like a JS module for all Preference manipulation operations.

PS: Quick question. Why don't http://hg.mozilla.org/mozilla-central/annotate/912036eeb024/toolkit/components/telemetry/TelemetryPing.jsm#l933 and https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.jsm codebases match? Both sources seem to be missing a few lines.

Comment 2

[Abhishek Bhattacharya [:babhishek21]]

Request for mentoring.

Comment 3

[Sparsh Paliwal [:discoman]]

I read asynchronous initialization of plugins.I got idea of the metadata function.I read about Preferences,dom.ipc.plugins.asyncInit exists in the Preferences(about:config),so we have to check if that is enabled or not and represent it in the telemetry send from user to mozilla.So we have to add that particular part for getting information about it is enabled or not.Am I right?...and for that we will use Preferences.has functions from preference.jsm to get the "value" of this particular preference.

Comment 4

[Sparsh Paliwal [:discoman]]

I want to work on this bug.This would be my second bug to work on.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Abhishek, are you still interested in working on this?

(Sparsh, Abhishek was the first to ask to work on this bug so I am asking Abhishek first.)

(In reply to Abhishek Bhattacharya from comment #1)
> I understand the `getMetadata` function. However I don't understand what
> exactly is required. 
> Am I supposed to `let` a local variable that uses Preferences.get()? Or am I
> required to write a separate method?
You will need to add a new property to the ret object that contains the value of the preference. You may do that in the body of getMetadata. Of course, if the additional code was complicated enough, you could wrote it in a separate function, but I do not think that applies in the case of this bug; you can just call Preferences.get() from within getMetadata.

> Basic understanding. This looks like a JS module for all Preference
> manipulation operations.
> 
Correct.

> PS: Quick question. Why don't
> http://hg.mozilla.org/mozilla-central/annotate/912036eeb024/toolkit/
> components/telemetry/TelemetryPing.jsm#l933 and
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryPing.jsm codebases match? Both sources seem to be missing a few
> lines.
My guess is that DXR had probably analyzed a different revision of the source.

Comment 6

[Abhishek Bhattacharya [:babhishek21]]

Thanks Aaaron for the reply. (I had forgotten all about it).
I'm sorry. I'm busy with 3 assigned bugs right now. 
Sparsh, feel free to to work on this bug. You have more experience, I guess.

To Aaron: If this bug remains NEW/OPEN by the time I finish up some of the bugs assigned to me (ETC: 1 week), I'll be happy to return and assist in this bug.

Thanks for the reply again.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

OK Sparsh, it's yours if you still want it...

Comment 8

[Sparsh Paliwal [:discoman]]

Yay...I still wanna work on it..thanks Abhishek and Aaron.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Done!

Comment 10

[Sparsh Paliwal [:discoman]]

I added 
asyncPluginInit: Preferences.get("dom.ipc.plugins.asyncInit")
in ret object
This added the feature and it is working and showing in about:telemetry whether it is on or not.But I am confused about the process of sending this data to the server.
I felt..that the code for sending the data in the ret object to the server as a whole is already present...so there is no need to write any code for it...am I right?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Sparsh Paliwal from comment #10)
> I added 
> asyncPluginInit: Preferences.get("dom.ipc.plugins.asyncInit")
> in ret object
> This added the feature and it is working and showing in about:telemetry
> whether it is on or not.But I am confused about the process of sending this
> data to the server.
> I felt..that the code for sending the data in the ret object to the server
> as a whole is already present...so there is no need to write any code for
> it...am I right?
That's right, everything inside that ret object will be added to the telemetry ping.

Comment 12

[Sparsh Paliwal [:discoman]]

Attachment: Added the asycInitPlugin in the telemetry ping

I hope it does the work. I was having issues with mercurial but now got it to work. Please review the patch.

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8555144 [details] [diff] [review]
Added the asycInitPlugin in the telemetry ping

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +540,5 @@
>        appUpdateChannel: UpdateChannel.get(),
>        platformBuildID: ai.platformBuildID,
>        revision: HISTOGRAMS_FILE_VERSION,
> +      locale: getLocale(),
> +      asyncPluginInit: Preferences.get("dom.ipc.plugins.asyncInit")

Can you please declare the pref name as a constant? See PREF_FHR_UPLOAD_ENABLED on line 31.

Also, please include the second parameter to Preferences.get which is a default value. In this case it should be false.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Sparsh, would you mind updating your patch to address my remarks in comment 13?

Comment 15

[Sparsh Paliwal [:discoman]]

I am really sorry for the delay in reply ,I was working with something else ,the TelemetryPing.jsm has completely changed no ret object, where we placed the Preference.get .I guess now it will be placed here,
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.jsm#91

Comment 16

[Vladan Djeric (:vladan)]

Sparsh, Telemetry is being unified with another data reporting system called "Firefox Health Report", so things are moving around.

The preference should be stored in TelemetrySession now:

https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/TelemetrySession.jsm#l548

Comment 17

[Sparsh Paliwal [:discoman]]

Attachment: Bug1117883_fix2.diff

Changes made as per the requirement.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8563601 [details] [diff] [review]
Bug1117883_fix2.diff

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

Looks good!

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/fx-team/rev/3fe14625f954

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fe14625f954