[Form Autofill] Collect information on how much time users spent on page with forms (w/wo form autofill)

NEW
Assigned to

Status

()

Toolkit
Form Manager
2 months ago
2 days ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [story][form autofill:M3][autofill-metrics])

User Story

Product Owner should be able to see how long does the users spent on the form under form autofill feature enabled/disabled.

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 months ago
Product Owner should be able to see how long does the users spent on the form under form autofill feature enabled/disabled.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a month ago
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

Hi Matt, I'm not sure if there's other way to access the telemetry, but adding a new probe in  Histogram.json directly is one of the solution I know by far. Please let me know if there's other alternative solution for system addon.

BTW we might need basic heuristic for collecting data, otherwise we'll collect nothing since our heuristic could only work for autocomplete property.
Attachment #8846567 - Flags: review?(MattN+bmo) → feedback?(MattN+bmo)
(Assignee)

Comment 4

a month ago
...Or only collect the data from target website
(Assignee)

Comment 5

a month ago
Hi Joe, talking about where to collect the data before the feature enabled, do you have any preference? Should we only record the results from 12 target websites, or record data from all the websites that could apply our heuristic checking?
Flags: needinfo?(jcheng)
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review122724

::: commit-message-0ec22:1
(Diff revision 1)
> +Bug 1341569 - Part 2: Add the from created time and telemetry probe for form filling duration. r?MattN

Did you mean "form" created time?

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

This won't handle "forms" without a <form>. I guess that would be fine for an initial landing but it would be good to refactor/share the formless logic out of pwmgr eventually.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

In my opinion we should only start the timer when the user first interacts with the form e.g. a `click` or `input` event otherwise we will be basically measuring from page load time which won't be accurate especially when things like session restore are involved.

::: toolkit/components/telemetry/Histograms.json:11065
(Diff revision 1)
>      "kind": "categorical",
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {

I think integer seconds isn't granular enough. I suspect we will only move the needle by less than 10 seconds so with the rounding of seconds it may be noisy to see a difference.

::: toolkit/components/telemetry/Histograms.json:11066
(Diff revision 1)
>      "bug_numbers": [1307689],
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],

Nit: I think you can add our public list too: autofill@…

::: toolkit/components/telemetry/Histograms.json:11067
(Diff revision 1)
>      "description": "Records a value each time a tab that is showing the loading throbber is interrupted. Desktop only.",
>      "labels": ["stop", "back", "forward", "historyNavigation", "reload", "tabClosed", "newURI"]
> +  },
> +  "FORM_FILLING_REQUIRED_TIME_S": {
> +    "alert_emails": ["schung@mozilla.com"],
> +    "expires_in_version": "60",

IIRC I think we want this probe to be opt-out on the release channel.

::: toolkit/components/telemetry/Histograms.json:11068
(Diff revision 1)
> +    "kind": "linear",
> +    "high": 600,
> +    "n_buckets": 20,

I think https://telemetry.mozilla.org/histogram-simulator/#low=0&high=600&n_buckets=22&kind=linear&generate=uniform would be better to have round numbers for the buckets.
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

::: browser/extensions/formautofill/FormAutofillParent.jsm:54
(Diff revision 1)
>  this.log = null;
>  FormAutofillUtils.defineLazyLogGetter(this, this.EXPORTED_SYMBOLS[0]);
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  const ENABLED_PREF = "browser.formautofill.enabled";
> +const EXPERIMENT_PREF="browser.formautofill.experimental";

Nit: spaces around operators

::: browser/extensions/formautofill/bootstrap.js:43
(Diff revision 1)
>  function startup() {
>    // Besides this pref, we'll need dom.forms.autocomplete.experimental enabled
>    // as well to make sure form autocomplete works correctly.
> -  if (!Services.prefs.getBoolPref("browser.formautofill.experimental")) {
> +  if (Services.prefs.getBoolPref("browser.formautofill.experimental")) {

I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.

Have you considered putting this telemetry in Satchel instead?
Attachment #8846566 - Flags: review?(MattN+bmo)
Attachment #8846567 - Flags: feedback?(MattN+bmo)
(Assignee)

Comment 8

a month ago
mozreview-review-reply
Comment on attachment 8846566 [details]
Bug 1341569 - Part 1: Make the formfill executable for metrics when exerimental pref is off,

https://reviewboard.mozilla.org/r/119616/#review122722

> I'm not sure it's a good idea to put this telemetry in F.A.P. Our extension doesn't even get built on beta/release (see browser/extensions/moz.build) so this won't actually get us data. I mostly worry about having to "ship" our system add-on without proper QA just to get the baseline numbers. I would feel better if the telemetry was more self-contained.
> 
> Have you considered putting this telemetry in Satchel instead?

If we put telemetry in Satchel, that means we could only record the data from target websites.  There's no heuristic logic in Satchel to know whether the website is suitable for profile filling. I'm fine with that if Joe wants to narrow down the focus to limited websites.
(Assignee)

Comment 9

a month ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review123280

::: browser/extensions/formautofill/FormAutofillContent.jsm:351
(Diff revision 1)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.createdTime);
> +      });

Thanks for pointing out this issue, I'll create a bug for it since we'll have lots of post-submitting tasks in Q2

::: browser/extensions/formautofill/FormAutofillHandler.jsm:57
(Diff revision 1)
> +   * The created time of the handler instance.
> +   */
> +  createdTime: null,

Yeah I think that would be more accurate.

Updated

a month ago
Assignee: nobody → schung
Whiteboard: [story] [form autofill][autofill-metrics] → [story][form autofill:M2][autofill-metrics]
(Assignee)

Comment 10

a month ago
Hi Joe, except for the question in comment 5, I have more questions for you:
- In your imagination, would you expect that the result is displayed like these histograms[1] that each histogram will represent the submitted time while feature disabled and enabled?
- In autofill enabled case, should we record the time only when feature is enabled and users actually apply the stored profile?

[1] https://mzl.la/2mvvRgl : Dashboard about the ADDON_CONTENT_POLICY_SHIM_BLOCKING_LOADING_MS
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

21 days ago
Attachment #8846566 - Flags: review?(MattN+bmo)
(Assignee)

Updated

21 days ago
Attachment #8846567 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Updated

20 days ago
Attachment #8846566 - Attachment is obsolete: true
(Assignee)

Comment 14

20 days ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review129880

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js
(Diff revision 3)
>    handleEvent(evt) {
>      if (!evt.isTrusted) {
>        return;
>      }
>  
> -    if (!Services.prefs.getBoolPref("browser.formautofill.enabled")) {

I removed this part in order to trigger identifyAutofillFields and create cache. I think it's easier to maintain the form status even when feature is disabled, and it won't affect the search result since it should fallback to history at startSearch.

Updated

9 days ago
Whiteboard: [story][form autofill:M2][autofill-metrics] → [story][form autofill:M3][autofill-metrics]
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135088

::: browser/extensions/formautofill/FormAutofillContent.jsm:413
(Diff revision 3)
> +      form.rootElement.addEventListener("submit", (evt) => {
> +        this._addFillFormTimeProbe(formHandler.inputStartedTime);
> +      });

Change this to use the form submit observer from your other bug.

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

This is a great place to use https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryStopwatch.jsm

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

I think this should be "input" since "change" only fires when you blur the field IIRC so we wouldn't be measuring time spent in the first field. Also, it would be great for memory and CPU usage to use event bubbling and only have a listener on the whole document for each handler and it gets removed for that handler by looking up the event.target's handler.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:88
(Diff revision 3)
>          // A field with the same identifier already exists.
>          log.debug("Not collecting a field matching another with the same info:", info);
>          continue;
>        }
>  
> +      element.addEventListener("change", () => {

For any event listener from untrusted pages we should be returning early if `event.isTrusted` is false.

::: toolkit/components/telemetry/Histograms.json:11333
(Diff revision 3)
> +    "kind": "exponential",
> +    "high": 300000,
> +    "n_buckets": 22,
> +    "bug_numbers": [1341569],
> +    "releaseChannelCollection": "opt-out",
> +    "description": "Collect information on how much time users spent on page with forms"

Nit: "Collect information on " isn't necessary.

"Milliseconds between focusing an autofill-eligible field and submitting its form"
Attachment #8846567 - Flags: review?(MattN+bmo)
(Assignee)

Comment 16

5 days ago
mozreview-review
Comment on attachment 8846567 [details]
Bug 1341569 - Add the form created time and telemetry probe for form filling duration.

https://reviewboard.mozilla.org/r/119618/#review135128

::: browser/extensions/formautofill/FormAutofillContent.jsm:442
(Diff revision 3)
> +    let histogram = Services.telemetry.getHistogramById("FORM_FILLING_REQUIRED_TIME_MS");
> +    histogram.add(Date.now() - startedTime);
> +  },

I knew we had this API for time recording, but my main concern is that user might fill multiple forms at the same time. We could only store one of the record if that happened(or we skip this time recording since it should be a rare case). WDYT?
The API already handles this with the aObj argument: https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/toolkit/components/telemetry/TelemetryStopwatch.jsm#124-128
(Assignee)

Updated

2 days ago
Duplicate of this bug: 990201
(Assignee)

Comment 19

2 days ago
(In reply to Steve Chung [:steveck] from comment #10)
> Hi Joe, except for the question in comment 5, I have more questions for you:
> - In your imagination, would you expect that the result is displayed like
> these histograms[1] that each histogram will represent the submitted time
> while feature disabled and enabled?

After some discussion with Joe, he is satisfied with the basic dashboards that brought by main pings.

> - In autofill enabled case, should we record the time only when feature is
> enabled and users actually apply the stored profile?

He would like to know the form submitting time even the feature is disabled, that means we'll need to identify forms and listen to earlysubmit event all the time. We might need to beware of this since the overhead wont be reduced even user disable the feature.
(Assignee)

Updated

2 days ago
Flags: needinfo?(jcheng)
You need to log in before you can comment on or make changes to this bug.