Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Description • 9 years ago

I'm not sure what the best way is to design a metric for this but I'd like to capture a telemetry count for the usage of the local addon.  

The questions I'd like to answer are:

Are people using this addon?

Possibly:
Are people using this on Nightly and is there Nightly the outdated?

	Joe Walker [:jwalker]
	Comment 1 • 9 years ago

Doing bug triage with Emma.
Filter on LOBSTER-THERMADOR

	Alexandre Poirot [:ochameau] Assignee
	Comment 2 • 9 years ago

Still need to test it.
It should log addon installation.
i.e. when we click on "Load temporary add-on" and register it.
And also reload action, when the developer hit the key shortcut.

	Lin Clark [:linclark]
	Comment 3 • 9 years ago

Just renaming to disambiguate from James's work to support hot reloading.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 4 • 9 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> Created attachment 8720388 [details] [diff] [review]
> patch v1
> 
> Still need to test it.
> It should log addon installation.
> i.e. when we click on "Load temporary add-on" and register it.
> And also reload action, when the developer hit the key shortcut.

Awesome!

	Alexandre Poirot [:ochameau] Assignee
	Comment 5 • 9 years ago

I'm wondering if userHistogram is important?

Couldn't we identify "how many users had DEVTOOLS_RELOAD_ADDON_INSTALLED_COUNT>0" on the backend side?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)
	Comment 6 • 9 years ago

Comment on attachment 8720743 [details] [diff] [review]
patch v2

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

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> I'm wondering if userHistogram is important?
> 
> Couldn't we identify "how many users had
> DEVTOOLS_RELOAD_ADDON_INSTALLED_COUNT>0" on the backend side?

I think it is possible to calculate the number of users from the basic probe if you use a more advanced query through the custom Spark pipeline, but that's a lot of work for something we'll likely want to reference regularly.

I think it's fine to "pre-compute" the user count with the flag probe as well, which also makes this similar to our other probes.

::: devtools/bootstrap.js
@@ +10,5 @@
>  
> +function actionOccurred(id) {
> +  let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> +  let Telemetry = require("devtools/client/shared/telemetry");;
> +  let telemetry = new Telemetry();

Could we cache the `telemetry` instance after the first use?  This would make it more like other usages of the modules.  It's currently okay for now, since we don't use the timer based ones (so there is no state to store), but it could be less confusing for the future that way.

::: toolkit/components/telemetry/Histograms.json
@@ +6982,5 @@
>      "description": "How many times has a custom developer tool been opened via the toolbox button?"
>    },
> +  "DEVTOOLS_RELOAD_ADDON_INSTALLED_COUNT": {
> +    "alert_emails": ["dev-developer-tools@lists.mozilla.org"],
> +    "expires_in_version": "50",

Maybe let them live a bit longer, like 55?

	Alexandre Poirot [:ochameau] Assignee
	Comment 7 • 9 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> ::: devtools/bootstrap.js
> @@ +10,5 @@
> >  
> > +function actionOccurred(id) {
> > +  let {require} = Cu.import("resource://devtools/shared/Loader.jsm", {});
> > +  let Telemetry = require("devtools/client/shared/telemetry");;
> > +  let telemetry = new Telemetry();
> 
> Could we cache the `telemetry` instance after the first use?  This would
> make it more like other usages of the modules.  It's currently okay for now,
> since we don't use the timer based ones (so there is no state to store), but
> it could be less confusing for the future that way.

I didn't do that as I'm reloading the loader and modules before/after that and wanted to avoid using a module from a old loader. I was wondering if I should even avoid using modules at all and instead use core telemetry API directly.

	Alexandre Poirot [:ochameau] Assignee
	Comment 8 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a024da44537a

	Alexandre Poirot [:ochameau] Assignee
	Comment 9 • 9 years ago

Bumped to 55.

	Bryan Clark (DevTools PM) [:clarkbw] Reporter
	Comment 10 • 9 years ago

Comment on attachment 8721986 [details] [diff] [review]
patch v3

I believe this is looking for feedback, not review, on these probes.  These are all for pre-release and we've set an expire version.

We need these to understand the funnel for activation and usage as we promote this feature over the next few releases.

	Benjamin Smedberg
	Comment 11 • 9 years ago

Comment on attachment 8721986 [details] [diff] [review]
patch v3

I couldn't reach you on IRC, so I guess I'll comment here. I promise faster reaction time next time!

>diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json

>+  "DEVTOOLS_RELOAD_ADDON_INSTALLED_COUNT": {
>+    "description": "How many times the reload addon has been installed?",

>+  "DEVTOOLS_RELOAD_ADDON_INSTALLED_PER_USER_FLAG": {
>+    "description": "How many users have installed the reload addon?",

These descriptions really shouldn't be phrased as questions, and I can't tell by reading them what the difference is or even what they are measuring. Please rephrase these as a statement of the form "This flag is recorded once per session if the reload addon is active." or something like that.

Is the reload addon a normal extension? If so, its presence should already be recorded in the telemetry environment block and you shouldn't need to record it separately here.

I have similar questions about the RELOAD_COUNT and RELOAD_PER_USER_FLAG metrics.

	Alexandre Poirot [:ochameau] Assignee
	Comment 12 • 9 years ago

With better description, similar to existing ones.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Is the reload addon a normal extension? If so, its presence should already
> be recorded in the telemetry environment block and you shouldn't need to
> record it separately here.

This is a temporary add-on. I'm not sure it is being covered by telemetry.
Developers install it temporarily via about:debugging. It is being automatically
removed on firefox restart.

	Alexandre Poirot [:ochameau] Assignee
	Comment 13 • 9 years ago

New descriptions for PER_USER histograms.

	Benjamin Smedberg
	Comment 14 • 9 years ago

Comment on attachment 8724970 [details] [diff] [review]
patch v5

data-review=me

	Alexandre Poirot [:ochameau] Assignee
	Comment 15 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f3b30b9d1be

	Alexandre Poirot [:ochameau] Assignee
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/69b8d739ddae4447e7bac8545b90fe69a6c8ff66
Bug 1248435 - Add telemetry probes for reload addon installation and reload action. r=jryans data-review=bsmedberg

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/69b8d739ddae