Open Bug 1177960 Opened 9 years ago Updated 2 years ago

Regularly submit a ping with extended addons data

Categories

(Toolkit :: Add-ons Manager, defect, P4)

defect

Tracking

()

Tracking Status
firefox42 --- affected

People

(Reporter: gfritzsche, Unassigned)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

From bug 1124128:

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> (In reply to Dave Townsend [:mossop] from comment #14)
> > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> > > Mossop's use-case is reasonable, but I suspect we can solve it this way:
> > > 
> > > Periodically submit a separate "addons" ping which lists all addons, enabled
> > > or not, including the signing status and all that good stuff.
> > > 
> > > Suggest that "periodically" be no more than once per week.
> > > 
> > > Dave, I also suggest that you consider a separate ping submitted at the time
> > > you disable an addon because it doesn't meet signing requirements: we could
> > > graph those in realtime and count them more easily than the normal session
> > > tracking pings.
> > 
> > Sounds somewhat similar to bug 1062386, I'm not sure how this helps though
> > (mind you I have only the basic idea of getting data from FHR). Getting info
> > on the number of users that see add-ons disabled when we first flip on
> > signing is one thing, but after that we want to track how many users are
> > still missing their add-ons.
> 
> Do we need to know which specific addons are affected or is it enough to
> keep track of the numbers per user?
> 
> If we go with a separate ping, this is a minimal example:
> https://hg.mozilla.org/integration/fx-team/rev/c418add0c07f
Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Moves the code that generates the telemetry data for an add-on to TelemetryUtils
and has AddonManagerInternal use it to send a ping once a week with a list of
all installed add-ons.
Attachment #8656249 - Flags: review?(benjamin)
The ping currently includes the environment. This is a bit wasteful as the environment contains all enabled add-ons too so maybe we shouldn't include it. But the environment does contain lots of useful info that might help understand why the add-ons manager is disabling certain add-ons. An alternative might be just duplicating that data into the ping's payload.
Assignee: nobody → dtownsend
https://reviewboard.mozilla.org/r/18123/#review16269

::: toolkit/components/telemetry/docs/addon-ping.rst:6
(Diff revision 1)
> +add-ons.

Please add whether this ping includes the environment data & the client id.

Can you be more detailed re "periodically"?
Ideally these docs are complete enough that people doing analysis can answer most of their questions from it.

::: toolkit/components/telemetry/docs/environment.rst:181
(Diff revision 1)
> +            id: <string>,

Why always add an id? We already have it as the key here.

::: toolkit/components/telemetry/docs/environment.rst:207
(Diff revision 1)
> +          type: <string>,

Adding a type here is redundant.

::: toolkit/mozapps/extensions/AddonManager.jsm:54
(Diff revision 1)
> +const TELEMETRY_REPORT_PERIOD         = 604800;

Can we make this more readable? E.g. something like:
// Send a full addon Telemetry ping every 7 days.
const TELEMETRY_REPORT_PERIOD_SEC = 7 * 24 * 60 * 60;

::: toolkit/mozapps/extensions/AddonManager.jsm:1037
(Diff revision 1)
> +  sendTelemetryPing: function AMI_sendTelemetryPing() {

AFAIK we don't need those function names anymore since the error & debugging tooling all show it now properly in stacks etc.

::: toolkit/mozapps/extensions/AddonManager.jsm:1041
(Diff revision 1)
> +      Cu.import("resource://gre/modules/TelemetryController.jsm");
> +      Cu.import("resource://gre/modules/TelemetryUtils.jsm");

Is there any benefit to importing those modules locally instead of using lazyModuleGetter?

::: toolkit/mozapps/extensions/AddonManager.jsm:1044
(Diff revision 1)
> +      this.getAddonsByTypes(["extension", "service", "theme"], (aAddons) => {

Can the async task in the AddonManager be cancelled (e.g. for shutdown etc.)?
What does that mean here, does this still get called?
Would we get an incomplete list of addons?
Could we tell that it was cancelled and not submit the ping?
(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #3)
> ::: toolkit/components/telemetry/docs/environment.rst:181
> (Diff revision 1)
> > +            id: <string>,
> 
> Why always add an id? We already have it as the key here.
> 
> ::: toolkit/components/telemetry/docs/environment.rst:207
> (Diff revision 1)
> > +          type: <string>,
> 
> Adding a type here is redundant.

The code takes the two places where we generate add-on info and moves them somewhere shared, which means that by default all users include the same info. I could make it hide the id or the type depending on the caller but I didn't know if it was worth the complexity, personally as someone who has written custom analysis I'd rather see all cases of an "addon" have the same fields regardless of where it came from. Your call though.

> ::: toolkit/mozapps/extensions/AddonManager.jsm:54
> (Diff revision 1)
> > +const TELEMETRY_REPORT_PERIOD         = 604800;
> 
> Can we make this more readable? E.g. something like:
> // Send a full addon Telemetry ping every 7 days.
> const TELEMETRY_REPORT_PERIOD_SEC = 7 * 24 * 60 * 60;
> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm:1037
> (Diff revision 1)
> > +  sendTelemetryPing: function AMI_sendTelemetryPing() {
> 
> AFAIK we don't need those function names anymore since the error & debugging
> tooling all show it now properly in stacks etc.

We don't but I added it for consistency with the rest of the module. One day I'll get round to just removing them all.

> ::: toolkit/mozapps/extensions/AddonManager.jsm:1041
> (Diff revision 1)
> > +      Cu.import("resource://gre/modules/TelemetryController.jsm");
> > +      Cu.import("resource://gre/modules/TelemetryUtils.jsm");
> 
> Is there any benefit to importing those modules locally instead of using
> lazyModuleGetter?

Less code to run on startup.

> ::: toolkit/mozapps/extensions/AddonManager.jsm:1044
> (Diff revision 1)
> > +      this.getAddonsByTypes(["extension", "service", "theme"], (aAddons) => {
> 
> Can the async task in the AddonManager be cancelled (e.g. for shutdown etc.)?
> What does that mean here, does this still get called?
> Would we get an incomplete list of addons?
> Could we tell that it was cancelled and not submit the ping?

I'd need to check what happens on shutdown but it certainly won't get an incomplete list. It will either be called with a full list or not called at all.
(In reply to Dave Townsend [:mossop] from comment #4)
> (In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #3)
> > ::: toolkit/components/telemetry/docs/environment.rst:181
> > (Diff revision 1)
> > > +            id: <string>,
> > 
> > Why always add an id? We already have it as the key here.
> > 
> > ::: toolkit/components/telemetry/docs/environment.rst:207
> > (Diff revision 1)
> > > +          type: <string>,
> > 
> > Adding a type here is redundant.
> 
> The code takes the two places where we generate add-on info and moves them
> somewhere shared, which means that by default all users include the same
> info. I could make it hide the id or the type depending on the caller but I
> didn't know if it was worth the complexity, personally as someone who has
> written custom analysis I'd rather see all cases of an "addon" have the same
> fields regardless of where it came from. Your call though.

It sounds like you'd probably want to look at the full addon list from the custom ping for analysis like that?
Because otherwise you always have to piece the data together from the different properties etc.
But i don't care too strongly about it.

Note that i'm away from a bit later today, bsmedberg or vladan should be good for review.
Comment on attachment 8656249 [details]
MozReview Request: Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Moves the code that generates the telemetry data for an add-on to TelemetryUtils
and has AddonManagerInternal use it to send a ping once a week with a list of
all installed add-ons.
Comment on attachment 8656249 [details]
MozReview Request: Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Moves the code that generates the telemetry data for an add-on to TelemetryUtils
and has AddonManagerInternal use it to send a ping once a week with a list of
all installed add-ons.
Attachment #8656249 - Flags: review?(benjamin) → review?(vladan.bugzilla)
I addressed all of Georg's comments.
Attachment #8656249 - Flags: review+
Comment on attachment 8656249 [details]
MozReview Request: Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

https://reviewboard.mozilla.org/r/18123/#review16985

::: toolkit/components/telemetry/TelemetryUtils.jsm:136
(Diff revision 2)
> +      data.signedState = aAddon.signedState;

Nit: In the Telemetry JS we tried to stick with always bracing the if-body.

::: toolkit/mozapps/extensions/test/xpcshell/test_telemetry.js:12
(Diff revision 2)
> +function test_telemetry(ping, addon) {

Nit: check_addon_data()?
Stole the review, but needinfo for vladan for looking over the data.
This should effectively be a straight port of the FHR data: we report the active addons in the environment, this adds submitting all addons (extensions, services, themes), even inactive ones, which FHR does too.
Flags: needinfo?(vladan.bugzilla)
I think I'm opposed to having a new ping type for addons:

1) There is no need to receive reports on addon state changes in real-time 
2) We do want the inactive addon info in regular pings (see below)
3) Sending weekly reports of addon state adds more complexity than necessary (see below for an alternative)


In reply to Benjamin Smedberg  [:bsmedberg])
> I really don't want to be in a position where upgrading, disabling, or uninstalling an inactive addon causes a new subsession.

Disabling an addon *should* start a new subsession (because the active ping list changes). It's true we don't need a new session when we uninstall an addon. Do we really upgrade disabled addons?

In reply to Benjamin Smedberg  [:bsmedberg])
>Really I don't want to record data about inactive addons at all:  I don't think anybody is actually going to use it and it will just bloat our data storage for no good reason.

We do need this information in regular pings. For example, inactive addons affect startup time (bug 810149)

Proposal:

I think we just need to add a new behaviour for Environment block changes, similar to how we no longer start a new subsession when certain watched prefs change (bug 1196219). 

Similar to bug 1196219, we should just treat changes to the "inactiveAddons" subsection differently.
Flags: needinfo?(vladan.bugzilla) → needinfo?(benjamin)
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> I think I'm opposed to having a new ping type for addons:
> 
> 1) There is no need to receive reports on addon state changes in real-time 
> 2) We do want the inactive addon info in regular pings (see below)
> 3) Sending weekly reports of addon state adds more complexity than necessary
> (see below for an alternative)
> 
> 
> In reply to Benjamin Smedberg  [:bsmedberg])
> > I really don't want to be in a position where upgrading, disabling, or uninstalling an inactive addon causes a new subsession.
> 
> Disabling an addon *should* start a new subsession (because the active ping
> list changes). It's true we don't need a new session when we uninstall an
> addon. Do we really upgrade disabled addons?

Yes, disabled add-ons are automatically upgraded so if the user chooses to enable they already have the latest version.

> In reply to Benjamin Smedberg  [:bsmedberg])
> >Really I don't want to record data about inactive addons at all:  I don't think anybody is actually going to use it and it will just bloat our data storage for no good reason.
> 
> We do need this information in regular pings. For example, inactive addons
> affect startup time (bug 810149)
> 
> Proposal:
> 
> I think we just need to add a new behaviour for Environment block changes,
> similar to how we no longer start a new subsession when certain watched
> prefs change (bug 1196219). 
> 
> Similar to bug 1196219, we should just treat changes to the "inactiveAddons"
> subsection differently.

What are the consequences of this? Does it mean that the data in the inactiveAddons section can be stale? Does it get updated when the user happens to restart their browser or could it be stale to the point that if nothing else in the environment changed then this data could be weeks old?
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #11)
> I think I'm opposed to having a new ping type for addons:
> 
> 1) There is no need to receive reports on addon state changes in real-time 

Yes, this patch wouldn't do this. It just submits the data roughly every 7 days.

> 2) We do want the inactive addon info in regular pings (see below)
> 3) Sending weekly reports of addon state adds more complexity than necessary
> (see below for an alternative)

This would be about submitting the full addon details, which are probably only interesting for certain specific analyses.

> In reply to Benjamin Smedberg  [:bsmedberg])
> >Really I don't want to record data about inactive addons at all:  I don't think anybody is actually going to use it and it will just bloat our data storage for no good reason.
> 
> We do need this information in regular pings. For example, inactive addons
> affect startup time (bug 810149)

This sounds like specific data that we want in the session payload, not necessarily the environment data (which is submitted with other pings too).
Also, do we need the full details to answer this or just say the addon id?
Comment on attachment 8656249 [details]
MozReview Request: Bug 1177960: Regularly submit a telemetry ping with information about all installed add-ons.

Clearing needinfo temporarily (while I wait for Ben's reply) to stop the Bugzilla reminders
Attachment #8656249 - Flags: review?(vladan.bugzilla)
Priority: -- → P3
Whiteboard: [measurement:client]
Kev and Andy are on point for figuring out any further details here. Happy to do patch work once we have nailed down what it is we want.
Assignee: dtownsend → nobody
I really do not think we should collect inactive addon data in the main pings. It's potentially a lot of data for relatively little gain. Knowing that addon scanning slows us down doesn't actually translate into much action for us.
Flags: needinfo?(benjamin)
Priority: P3 → P4
Component: Telemetry → Add-ons Manager
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: