1356181 - Gather telemetry for isindex usage

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Henri Sivonen (:hsivonen)]

Find out what proportion of sessions has isindex submitted at least once.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

Review commit: https://reviewboard.mozilla.org/r/129932/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/129932/

Comment 2

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review132488

::: dom/html/HTMLFormSubmission.cpp:192
(Diff revision 1)
>    nsresult rv = URLEncode(aValue, convValue);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Append data to string
>    if (mQueryString.IsEmpty()) {
> +    Telemetry::ScalarSet(Telemetry::ScalarID::WEB_ISINDEX_USED, true);

The intent is to be able to see what proportion of Firefox sessions sees at least one legacy `isindex` submission. I'm not sure if the telemetry usage allows compasison with the `true` case with all submissions considering that the non-`true` case of this scalar doesn't get submitted.

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review132506

::: dom/html/HTMLFormSubmission.cpp:192
(Diff revision 1)
>    nsresult rv = URLEncode(aValue, convValue);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Append data to string
>    if (mQueryString.IsEmpty()) {
> +    Telemetry::ScalarSet(Telemetry::ScalarID::WEB_ISINDEX_USED, true);

Since there seems not to be an easy way to do this on TMO, I've filed bug 1356232 to add support for default values for scalars.

I'm not sure I can get to that before next week so, if you need to land this soon-ish, you might consider using a flag histogram and add it as an exception in the histogram-whitelist.json file.

::: toolkit/components/telemetry/Scalars.yaml:534
(Diff revision 1)
> +    expires: "56"
> +    kind: boolean
> +    notification_emails:
> +      - [e-mail]
> +    record_in_processes:
> +      - 'content'

You would want to make this record both in the 'content' and in the 'main' process, otherwise you would not record anything in non-e10s.

Comment 4

[Georg Fritzsche [:gfritzsche]]

As this is currently not feasible using TMO, can the analysis here use re:dash or a custom analysis job instead?

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Georg Fritzsche [:gfritzsche] [away Apr 13 - 18] from comment #4)
> As this is currently not feasible using TMO, can the analysis here use
> re:dash or a custom analysis job instead?

I don't know what those are.

(In reply to Alessio Placitelli [:Dexter] from comment #3)
> I'm not sure I can get to that before next week so, if you need to land this
> soon-ish, you might consider using a flag histogram and add it as an
> exception in the histogram-whitelist.json file.

I'll do that. Thanks.

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129932/diff/1-2/

Comment 7

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review132506

> Since there seems not to be an easy way to do this on TMO, I've filed bug 1356232 to add support for default values for scalars.
> 
> I'm not sure I can get to that before next week so, if you need to land this soon-ish, you might consider using a flag histogram and add it as an exception in the histogram-whitelist.json file.

Addressed via using legacy `flag`.

Comment 8

[Alessio Placitelli [:Dexter]]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] [away Apr 13 - 18] from comment
> #4)
> > As this is currently not feasible using TMO, can the analysis here use
> > re:dash or a custom analysis job instead?
> 
> I don't know what those are.

re:dash -> http://sql.telemetry.mozilla.org/
custom analysis job -> http://analysis.telemetry.mozilla.org/

> (In reply to Alessio Placitelli [:Dexter] from comment #3)
> > I'm not sure I can get to that before next week so, if you need to land this
> > soon-ish, you might consider using a flag histogram and add it as an
> > exception in the histogram-whitelist.json file.
> 
> I'll do that. Thanks.

The legacy usage looks correct from the code, however I don't think I'm the right person to review this code:

- Somebody else needs to review the probe to make sure it's measuring the right thing
- You need a data-review for this from one of the data peers

Thanks!

Comment 9

[François Marier [:francois]]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review133960

datareview+ (with the small changes below)

::: toolkit/components/telemetry/Histograms.json:5944
(Diff revision 2)
>    "INNERWINDOWS_WITH_MUTATION_LISTENERS": {
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "description": "Deleted or to-be-reused innerwindow which has had mutation event listeners."
>    },
> +  "ISINDEX_USED": {

I would suggest `FORM_ISINDEX_USED` to be a little more specific.

::: toolkit/components/telemetry/Histograms.json:5949
(Diff revision 2)
> +  "ISINDEX_USED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "56",
> +    "kind": "flag",
> +    "bug_numbers": [1356181],
> +    "description": "Whether there has been an isindex submission in this session."

Can you please add the word "form" in there to make it clear what it refers to?

For example: "Whether there has been an isindex form submission in this session."

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/129932/diff/2-3/

Comment 11

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review133960

> I would suggest `FORM_ISINDEX_USED` to be a little more specific.

Changed name as requested.

> Can you please add the word "form" in there to make it clear what it refers to?
> 
> For example: "Whether there has been an isindex form submission in this session."

Added the word "form" as requested.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review134368

::: toolkit/components/telemetry/Histograms.json:5944
(Diff revision 3)
>    "INNERWINDOWS_WITH_MUTATION_LISTENERS": {
>      "expires_in_version": "never",
>      "kind": "boolean",
>      "description": "Deleted or to-be-reused innerwindow which has had mutation event listeners."
>    },
> +  "FORM_ISINDEX_USED": {

This is an opt-in probe, which means you effectively only collect this data from pre-release channels:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Data_collection_levels

Is that sufficient or do you need data from the release population?

::: toolkit/components/telemetry/Histograms.json:5947
(Diff revision 3)
>      "description": "Deleted or to-be-reused innerwindow which has had mutation event listeners."
>    },
> +  "FORM_ISINDEX_USED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "56",
> +    "kind": "flag",

We can make this a boolean scalar.

Talking to frank, bug 1353105 should make this queryable from next week on or so.
We can help you write a query for that, e.g. by catching us in #telemetry.

Comment 13

[(no longer active)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review134694

::: dom/html/HTMLFormSubmission.cpp:192
(Diff revision 3)
>    nsresult rv = URLEncode(aValue, convValue);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    // Append data to string
>    if (mQueryString.IsEmpty()) {
> +    Telemetry::Accumulate(Telemetry::FORM_ISINDEX_USED, true);

Hmm, so we are collecting the number of sessions that encounter a form where isindex is used, so at least we won't double count cases where it happens several times per session, which is good.  I can't really think of something better to measure, so this is probably fine.

Thanks for the patch!

Comment 14

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review134694

> Hmm, so we are collecting the number of sessions that encounter a form where isindex is used, so at least we won't double count cases where it happens several times per session, which is good.  I can't really think of something better to measure, so this is probably fine.
> 
> Thanks for the patch!

This comment looks like it wasn't meant to be an "issue", so dropping the issueness.

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

https://reviewboard.mozilla.org/r/129932/#review134368

> This is an opt-in probe, which means you effectively only collect this data from pre-release channels:
> https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Data_collection_levels
> 
> Is that sufficient or do you need data from the release population?

What I'm looking for is "practically zero usage" vs. "more than practically zero usage", so opt-in should be enough.

> We can make this a boolean scalar.
> 
> Talking to frank, bug 1353105 should make this queryable from next week on or so.
> We can help you write a query for that, e.g. by catching us in #telemetry.

I started with a boolean scalar and was advised to make it a legacy flag instead. Since this issue has taken excessive (relative to the expected benefit) time already, I'm going to land as-is with the current reviews.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/648169abf928
Gather telemetry about isindex usage in hope of justifying removal. r=Ehsan,francois

Comment 17

[Georg Fritzsche [:gfritzsche]]

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> > We can make this a boolean scalar.
> > 
> > Talking to frank, bug 1353105 should make this queryable from next week on or so.
> > We can help you write a query for that, e.g. by catching us in #telemetry.
> 
> I started with a boolean scalar and was advised to make it a legacy flag
> instead. Since this issue has taken excessive (relative to the expected
> benefit) time already, I'm going to land as-is with the current reviews.

I think part of this was that the communication was a bit scattered on multiple bugs here (plus people being out over easter).
We are happy to help through the usage of tools in #telemetry etc., ideally by starting from the specific problem that you want to solve next time.

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/648169abf928

Comment 19

[(no longer active)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Comment on attachment 8857895 [details]
> Bug 1356181 - Gather telemetry about isindex usage in hope of justifying
> removal.
> 
> https://reviewboard.mozilla.org/r/129932/#review134694
> 
> > Hmm, so we are collecting the number of sessions that encounter a form where isindex is used, so at least we won't double count cases where it happens several times per session, which is good.  I can't really think of something better to measure, so this is probably fine.
> > 
> > Thanks for the patch!
> 
> This comment looks like it wasn't meant to be an "issue", so dropping the
> issueness.

Oh no sorry, I have never really understood this MozReview issue business, so I just ignore it.  This was mostly documenting my thoughts during the review on the bug.  :-)

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

Approval Request Comment
> [Feature/Bug causing the regression]:

Not a regression.

> [User impact if declined]:

Isindex will be removed later, which will lead to users not receiving something nice that developers could work on instead sooner, because non-zero amount of developer time goes into working around the fact that isindex continues to exist in our code.

> [Is this code covered by automated tests?]:

No.

> [Has the fix been verified in Nightly?]:

There seem to be telemetry submissions.

> [Needs manual test from QE? If yes, steps to reproduce]: 

No.

> [List of other uplifts needed for the feature/fix]:

None.

> [Is the change risky?]:

No.

> [Why is the change risky/not risky?]:

It's just a telemetry probe.

> [String changes made/needed]:

None.

Note the sheriff:
The changes to toolkit/components/telemetry/histogram-whitelists.json don't apply to beta and need to be simply ignored when uplifting to beta.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8857895 [details]
Bug 1356181 - Gather telemetry about isindex usage in hope of justifying removal.

Get telemetry for usage of isindex. Beta54+. Should be in 54 beta 5.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bfcbbd92d6e6

Comment 23

[Andrei Vaida [:avaida]]

(In reply to Henri Sivonen (:hsivonen) from comment #20)
> > [Is this code covered by automated tests?]:
> 
> No.
> 
> > [Has the fix been verified in Nightly?]:
> 
> There seem to be telemetry submissions.
> 
> > [Needs manual test from QE? If yes, steps to reproduce]: 
> 
> No.

Setting qe-verify- based on Henri's assessment on manual testing needs.

Comment 24

[Henri Sivonen (:hsivonen)]

When I wrote comment 20 (8 days ago, says Bugzilla), I saw FORM_ISINDEX_USED data on Measurement Dashboard for Nightly. Now it's gone. gfritzsche, any idea what's going on? Does the dashboard get confused if a telemetry bucket exists on beta and nightly but not Aurora?

Comment 25

[Georg Fritzsche [:gfritzsche]]

Frank, can you take a look?

Comment 26

[Frank Bertsch [:frank]]

We had a minor issue with some dependencies. Everything is now in order for TMO.