Closed Bug 1211164 Opened 4 years ago Closed 4 years ago

Collect JS deprecated language extension telemetry for Add-ons.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(1 file)

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.
English is difficult :P
Summary: Correct JS deprecated language extension telemetry for Add-ons. → 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.
Assignee: nobody → arai.unmht
Attachment #8669356 - Flags: review?(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)))
Attachment #8669356 - Flags: review?(till)
Attachment #8669356 - Flags: review?(benjamin)
Attachment #8669356 - Flags: review+
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?
Flags: needinfo?(arai.unmht)
(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?
Flags: needinfo?(arai.unmht)
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?
Flags: needinfo?(nihsanullah)
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 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": ["jdemooij@mozilla.com"] into Histograms.json for these metrics so that we know who owns monitoring them.

data-collection-review+ with that change
Attachment #8669356 - Flags: review?(benjamin) → feedback+
https://hg.mozilla.org/integration/mozilla-inbound/rev/01cdd3e5bd7153dfdc47d865538e15e4754dc0cf
Bug 1211164 - Collect JS deprecated language extension telemetry for Add-ons. r=till,bsmedberg
https://hg.mozilla.org/mozilla-central/rev/01cdd3e5bd71
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee: arai.unmht → emanuel.hoogeveen
Flags: needinfo?(nihsanullah)
Oops changed Assignee
Assignee: emanuel.hoogeveen → arai.unmht
You need to log in before you can comment on or make changes to this bug.