[autofill] Add UKM for field type prediction quality and autofill outcome.

[autofill] Add UKM for field type prediction quality and autofill outcome.

Add UKM entries for predicted/actual field types and for filled/edited
status.

These metrics allow us to identify which field types are being confused
by the type prediction systems (heuristics, server, overall) as well as
to understand for which fields users accept autofilled values vs editing
them.

BUG=733708
R=mathp@chromium.org, sebsg@chromium.org

Review-Url: https://codereview.chromium.org/2940983003
Cr-Commit-Position: refs/heads/master@{#483159}
Committed: https://chromium.googlesource.com/chromium/src/+/a1755c304ab86df45ca7796be7adff070cd61d1a

[Roger McFarlane (Chromium), 2017-06-15 21:21:42]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 21:22:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roger McFarlane (Chromium), 2017-06-15 21:22:40]

PTAL?

[Roger McFarlane (Chromium), 2017-06-15 21:23:28]

rogerm@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Roger McFarlane (Chromium), 2017-06-15 21:23:29]

+rkaplow

[commit-bot: I haz the power, 2017-06-15 22:26:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-15 22:26:07]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-06-19 14:48:09]

Nice! Small comments

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.h:703: class UkmTimestampPin;
As discussed, please remove or add comment.

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics_unittest.cc:335: const
ukm::TestUkmRecorder& ukm_recorder,
What's the advantage of putting it first?

[rkaplow, 2017-06-19 17:03:28]

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml
File tools/metrics/ukm/ukm.xml (right):

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:41: Recorded when Chrome validates field type
predictions (on submit, on leaving
is this wrong?

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:46: The signature of the field
.

(also can you give more details on what signatures are?)

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:59: <metric name="MillisecondsSinceFormParsed">
prefer putting MS as a suffix

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:93: <metric name="FieldSignature">
similar comments

[Roger McFarlane (Chromium), 2017-06-21 21:01:45]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[Roger McFarlane (Chromium), 2017-06-21 21:01:55]

Thanks. Another look?

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.h:703: class UkmTimestampPin;
On 2017/06/19 14:48:09, sebsg wrote:
> As discussed, please remove or add comment.

Done.

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics_unittest.cc:335: const
ukm::TestUkmRecorder& ukm_recorder,
On 2017/06/19 14:48:09, sebsg wrote:
> What's the advantage of putting it first?

nothing in particular. mostly just a preference to put things in this order:

  1. The context being verified.
  2. Parameters to index into the context.
  3. output.

The initial change was to pass the recorder by const ref instead of const ptr
(not sure why that was)... then I just "rationalized" the param order at the
same time.

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml
File tools/metrics/ukm/ukm.xml (right):

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:41: Recorded when Chrome validates field type
predictions (on submit, on leaving
On 2017/06/19 17:03:28, rkaplow wrote:
> is this wrong?

No, but the constants used to refer to them are poorly named.

  AutofillMetrids::QualityMetricType::{
    TYPE_SUBMISSION,
    TYPE_NO_SUBMISSION,
    TYPE_AUTOCOMPLETE_BASED
  }

I plan to go back and rename these... but not in this CL; it's big enough
already.

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:46: The signature of the field
On 2017/06/19 17:03:28, rkaplow wrote:
> .
> 
> (also can you give more details on what signatures are?)

Done.

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:59: <metric name="MillisecondsSinceFormParsed">
On 2017/06/19 17:03:28, rkaplow wrote:
> prefer putting MS as a suffix

This string is used throughout the autofill UKM events. I'd prefer to be
consistent and either leave it as it, or change it everywhere.

As the UKMs are temporarily on hold, this might be a good opportunity for a
rename, but that would require this CL to be merged into M60... for which we're
a bit late.

https://codereview.chromium.org/2940983003/diff/1/tools/metrics/ukm/ukm.xml#n...
tools/metrics/ukm/ukm.xml:93: <metric name="FieldSignature">
On 2017/06/19 17:03:28, rkaplow wrote:
> similar comments

Done.

[commit-bot: I haz the power, 2017-06-21 21:02:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 21:07:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-21 21:07:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sebsg, 2017-06-21 21:08:03]

lgtm

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics_unittest.cc (right):

https://codereview.chromium.org/2940983003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics_unittest.cc:335: const
ukm::TestUkmRecorder& ukm_recorder,
On 2017/06/21 21:01:54, Roger McFarlane (Chromium) wrote:
> On 2017/06/19 14:48:09, sebsg wrote:
> > What's the advantage of putting it first?
> 
> nothing in particular. mostly just a preference to put things in this order:
> 
>   1. The context being verified.
>   2. Parameters to index into the context.
>   3. output.
> 
> The initial change was to pass the recorder by const ref instead of const ptr
> (not sure why that was)... then I just "rationalized" the param order at the
> same time.
>   

SGTM

[rkaplow, 2017-06-23 20:22:33]

lgtm

[Mathieu, 2017-06-27 21:02:28]

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.cc:372: // possible and
predicted field types (|possible_types| and |predicted_type|,
can you fix up the comment

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.cc:1476:
pinned_timestamp_.is_null() ? base::TimeTicks::Now() : pinned_timestamp_;
AutofillClock::Now would remove the need for pinned timestamp?

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.h:769:
logger->set_pinned_timestamp(base::TimeTicks::Now());
AutofillClock::Now? Would allow you to test

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:700:
AutofillMetrics::UkmTimestampPin timestamp_pin(form_interactions_ukm_logger);
Could you use AutofillClock::Now in the code? That way you could use
TestAutofillClock to achieve the same effect.

https://codereview.chromium.org/2940983003/diff/20001/tools/metrics/ukm/ukm.xml
File tools/metrics/ukm/ukm.xml (right):

https://codereview.chromium.org/2940983003/diff/20001/tools/metrics/ukm/ukm.x...
tools/metrics/ukm/ukm.xml:39: <owner>rogerm@google.com</owner>
@chromium.org

[Roger McFarlane (Chromium), 2017-06-28 17:33:34]

The CQ bit was checked by rogerm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 17:33:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Roger McFarlane (Chromium), 2017-06-28 17:41:01]

Thanks! Another look?

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics.cc (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.cc:372: // possible and
predicted field types (|possible_types| and |predicted_type|,
On 2017/06/27 21:02:27, Mathieu wrote:
> can you fix up the comment

Done.

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.cc:1476:
pinned_timestamp_.is_null() ? base::TimeTicks::Now() : pinned_timestamp_;
On 2017/06/27 21:02:27, Mathieu wrote:
> AutofillClock::Now would remove the need for pinned timestamp?

No, pinned timestamp is to give all events generated under the pin the same
timestamp. This is used during the quality metrics logging.

Also AutofillClock captures wall-clock time. We're using TimeTicks here, to
sequence events, not map them back to a real time.

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_metrics.h:769:
logger->set_pinned_timestamp(base::TimeTicks::Now());
On 2017/06/27 21:02:27, Mathieu wrote:
> AutofillClock::Now? Would allow you to test

AutofillClock captures wall time, not TimeTicks.

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/form_structure.cc (right):

https://codereview.chromium.org/2940983003/diff/20001/components/autofill/cor...
components/autofill/core/browser/form_structure.cc:700:
AutofillMetrics::UkmTimestampPin timestamp_pin(form_interactions_ukm_logger);
On 2017/06/27 21:02:27, Mathieu wrote:
> Could you use AutofillClock::Now in the code? That way you could use
> TestAutofillClock to achieve the same effect.

the pin isn't for testing, it's to pin the events to the same timestamp in
production.

https://codereview.chromium.org/2940983003/diff/20001/tools/metrics/ukm/ukm.xml
File tools/metrics/ukm/ukm.xml (right):

https://codereview.chromium.org/2940983003/diff/20001/tools/metrics/ukm/ukm.x...
tools/metrics/ukm/ukm.xml:39: <owner>rogerm@google.com</owner>
On 2017/06/27 21:02:27, Mathieu wrote:
> @chromium.org

Done.

[Mathieu, 2017-06-28 20:44:35]

lgtm

[Roger McFarlane (Chromium), 2017-06-28 20:50:47]

The CQ bit was checked by rogerm@chromium.org

[Roger McFarlane (Chromium), 2017-06-28 20:50:48]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2940983003/#ps60001
(title: "fix a comment")

[commit-bot: I haz the power, 2017-06-28 20:51:26]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 21:58:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498683047912330,
"parent_rev": "30ce8a5ef772032f32fcb53d39612aca2529c891", "commit_rev":
"a1755c304ab86df45ca7796be7adff070cd61d1a"}

[commit-bot: I haz the power, 2017-06-28 21:58:30]

Description was changed from

==========
[autofill] Add UKM for field type prediction quality and autofill outcome.

Add UKM entries for predicted/actual field types and for filled/edited
status.

These metrics allow us to identify which field types are being confused
by the type prediction systems (heuristics, server, overall) as well as
to understand for which fields users accept autofilled values vs editing
them.

BUG=733708
R=mathp@chromium.org, sebsg@chromium.org
==========

to

==========
[autofill] Add UKM for field type prediction quality and autofill outcome.

Add UKM entries for predicted/actual field types and for filled/edited
status.

These metrics allow us to identify which field types are being confused
by the type prediction systems (heuristics, server, overall) as well as
to understand for which fields users accept autofilled values vs editing
them.

BUG=733708
R=mathp@chromium.org, sebsg@chromium.org

Review-Url: https://codereview.chromium.org/2940983003
Cr-Commit-Position: refs/heads/master@{#483159}
Committed:
https://chromium.googlesource.com/chromium/src/+/a1755c304ab86df45ca7796be7ad...
==========

[commit-bot: I haz the power, 2017-06-28 21:58:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/a1755c304ab86df45ca7796be7ad...