1211164 - Collect JS deprecated language extension telemetry for Add-ons.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 1208835.

Currently we're collecting telemetry for deprecated language extension only for web content (bug 1133900).

If we're going to remove some deprecated feature, we'll also need to know how much it's used in Add-ons, to reduce addon-compat issue after removing it.  Of course it should be corrected separately from web content.

Comment 1

[Tooru Fujisawa [:arai]]

English is difficult :P

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Collect JS deprecated language extension telemetry for Add-ons.

If we're going to collect all values, it could be done in JSCompartment::addTelemetry and JSCompartment::reportTelemetry.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8669356 [details] [diff] [review]
Collect JS deprecated language extension telemetry for Add-ons.

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

Nice, thank you. r=me with nit addressed. However, I asked for a telemetry peer's review, too, so please wait for that before landing.

@bsmedberg, we have exactly this probe for web content already. We now want to also introduce it for addons, so we know when it is safe to remove deprecated features not only from the web but also from chrome JS. (We explicitly didn't include addons or chrome code for the first probe because we had lots of usages through the addons SDK and didn't want web usage to be swamped out. Now things have changed somewhat in the SDK and we want to learn about addons, too.)

::: js/src/jscompartment.cpp
@@ +1094,5 @@
>  void
>  JSCompartment::addTelemetry(const char* filename, DeprecatedLanguageExtension e)
>  {
> +    // Only report telemetry for web content and add-ons, not chrome JS.
> +    if (!addonId && (isSystem_ || !filename || strncmp(filename, "http", 4) != 0))

In practice it might not make a difference, but this check does something slightly different than the above: the way it is now, if there is an addonId it doesn't make a difference if isSystem_ is set or not. Before, and in JSCompartment::reportTelemetry, if isSystem_ is set we always bail. Perhaps change it to
if (isSystem_ || (!addonId && (!filename || strncmp(filename, "http", 4) != 0)))

Comment 4

[Benjamin Smedberg]

Comment on attachment 8669356 [details] [diff] [review]
Collect JS deprecated language extension telemetry for Add-ons.

Do you also want to record the addon ID in a keyed histogram, or is counting them across all addons sufficient?

I don't think this should be expires_in_version: never unless you have a product manager or QA lead who is committed to monitoring this permanently. Do you?

Comment 5

[Tooru Fujisawa [:arai]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8669356 [details] [diff] [review]
> Collect JS deprecated language extension telemetry for Add-ons.
> 
> Do you also want to record the addon ID in a keyed histogram, or is counting
> them across all addons sufficient?

Counting across all addons will be sufficient, as this is the complementary for JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT, that we're also monitoring only the total number.  Telemetry probes for add-ons are once added by bug 1054630 as part of JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT, and excluded from it by bug 1133900.

I'm not going to contact each add-on's developer for now.  Just to figure out how much those deprecated features are used in add-ons, and when we can remove it safely.

I think we should collect data per each add-ons only when the number stop decreasing at high point.

> I don't think this should be expires_in_version: never unless you have a
> product manager or QA lead who is committed to monitoring this permanently.
> Do you?

I think this should be treated in same way as JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT.
so... who is the product manager currently working on it?

Comment 6

[Benjamin Smedberg]

In case you're asking me that last question: I don't know. Naveed, do you have embedded QA on the JS team to help with this kind of thing?

Comment 7

[Jan de Mooij [:jandem]]

Not sure what QA means in this context, but personally I check this Telemetry probe regularly to see which features we can potentially remove from the engine...

Comment 8

[Benjamin Smedberg]

Comment on attachment 8669356 [details] [diff] [review]
Collect JS deprecated language extension telemetry for Add-ons.

If Jan is signed up to monitor these, please add "alert_emails": ["[e-mail]"] into Histograms.json for these metrics so that we know who owns monitoring them.

data-collection-review+ with that change

Comment 9

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01cdd3e5bd7153dfdc47d865538e15e4754dc0cf
Bug 1211164 - Collect JS deprecated language extension telemetry for Add-ons. r=till,bsmedberg

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/01cdd3e5bd71

Comment 11

[Naveed Ihsanullah [:naveed]]

Oops changed Assignee