Gather telemetry for isindex usage

RESOLVED FIXED in Firefox 54

Status

()

Core
HTML: Form Submission
RESOLVED FIXED
2 months ago
18 days ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
Find out what proportion of sessions has isindex submitted at least once.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8857895 - Flags: review?(alessio.placitelli)
(Assignee)

Comment 2

2 months ago
mozreview-review
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

2 months ago
mozreview-review
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:
> +      - hsivonen@mozilla.com
> +    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.
Attachment #8857895 - Flags: review?(alessio.placitelli)
As this is currently not feasible using TMO, can the analysis here use re:dash or a custom analysis job instead?
(Assignee)

Comment 5

a month ago
(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 hidden (mozreview-request)
(Assignee)

Comment 7

a month ago
mozreview-review-reply
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`.
(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!
Attachment #8857895 - Flags: review?(alessio.placitelli)
(Assignee)

Updated

a month ago
Attachment #8857895 - Flags: review?(francois)
Attachment #8857895 - Flags: review?(ehsan)

Comment 9

a month ago
mozreview-review
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": ["hsivonen@mozilla.com"],
> +    "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."
Attachment #8857895 - Flags: review?(francois) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a month ago
mozreview-review-reply
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

a month ago
mozreview-review
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": ["hsivonen@mozilla.com"],
> +    "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 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!
Attachment #8857895 - Flags: review?(ehsan) → review+
(Assignee)

Comment 14

a month ago
mozreview-review-reply
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.
(Assignee)

Comment 15

a month ago
mozreview-review-reply
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

a month ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/648169abf928
Gather telemetry about isindex usage in hope of justifying removal. r=Ehsan,francois
(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.
https://hg.mozilla.org/mozilla-central/rev/648169abf928
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(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.  :-)
(Assignee)

Comment 20

26 days ago
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.
Attachment #8857895 - Flags: approval-mozilla-beta?

Updated

26 days ago
status-firefox54: --- → affected
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.
Attachment #8857895 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

25 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/bfcbbd92d6e6
status-firefox54: affected → fixed
(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.
Flags: qe-verify-
(Assignee)

Comment 24

19 days ago
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?
Flags: needinfo?(gfritzsche)
Frank, can you take a look?
Flags: needinfo?(gfritzsche) → needinfo?(fbertsch)
We had a minor issue with some dependencies. Everything is now in order for TMO.
Flags: needinfo?(fbertsch)
You need to log in before you can comment on or make changes to this bug.