Closed Bug 1117883 Opened 5 years ago Closed 5 years ago

Explicitly report in Telemetry whether asyncPluginInit feature is enabled

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: vladan, Assigned: discoman, Mentored)

References

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files)

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
Summary: Explicitly report whether asyncPluginInit feature is enabled in Telemetry pings → Explicitly report in Telemetry whether asyncPluginInit feature is enabled
(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.
Request for mentoring.
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.
I want to work on this bug.This would be my second bug to work on.
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.
Flags: needinfo?(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.
Flags: needinfo?(babhishek21)
OK Sparsh, it's yours if you still want it...
Flags: needinfo?(sparshpaliwal123)
Yay...I still wanna work on it..thanks Abhishek and Aaron.
Flags: needinfo?(sparshpaliwal123)
Done!
Assignee: nobody → sparshpaliwal123
Status: NEW → ASSIGNED
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?
(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.
I hope it does the work. I was having issues with mercurial but now got it to work. Please review the patch.
Attachment #8555144 - Flags: review?(aklotz)
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.
Attachment #8555144 - Flags: review?(aklotz) → feedback+
Sparsh, would you mind updating your patch to address my remarks in comment 13?
Flags: needinfo?(sparshpaliwal123)
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
Flags: needinfo?(sparshpaliwal123)
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
Flags: needinfo?(sparshpaliwal123)
Changes made as per the requirement.
Flags: needinfo?(sparshpaliwal123)
Attachment #8563601 - Flags: review?(aklotz)
Comment on attachment 8563601 [details] [diff] [review]
Bug1117883_fix2.diff

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

Looks good!
Attachment #8563601 - Flags: review?(aklotz) → review+
https://hg.mozilla.org/mozilla-central/rev/3fe14625f954
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.