Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(21)

Issue 2870163003: Log Autofill.Quality.*.ByFieldType by predicted type for unknown data. (Closed)

Created:
3 years, 7 months ago by Roger McFarlane (Chromium)
Modified:
3 years, 6 months ago
Reviewers:
privard, Mathieu, sebsg, jwd, dvadym
CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, asvitkine+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Log Autofill.Quality.*.ByFieldType by predicted type for unknown data. When the actual data type of an field cannot be determined (because it was left empty or was populated with unknown data) log the ByFieldType version of the metrics by the type predicted by autofill. This will allow analysis of the false-positive rate broken down by type. The previous version of the code was logging all of these under Empty/Unknown => AMBIGUOUS. The CL also fleshes out the ByFieldType enumeration in histograms.xml BUG=710474 R=sebsg Review-Url: https://codereview.chromium.org/2870163003 Cr-Commit-Position: refs/heads/master@{#476766} Committed: https://chromium.googlesource.com/chromium/src/+/3c6caa287dacd94ee5e64eedaf44ea987f287bf8

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : fix typo #

Patch Set 4 : fleshed out unittests #

Patch Set 5 : rebase #

Total comments: 4

Patch Set 6 : remove defunct unit-tests #

Total comments: 2

Patch Set 7 : fix comments #

Total comments: 12

Patch Set 8 : comments from mathp #

Total comments: 4

Patch Set 9 : nits and declare predicted x actual metric #

Patch Set 10 : rebase #

Patch Set 11 : remove confusing 'units' from histograms.xml #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1239 lines, -811 lines) Patch
M components/autofill/core/browser/autofill_metrics.h View 1 2 3 4 5 6 7 3 chunks +97 lines, -34 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics.cc View 1 2 3 4 5 6 7 8 5 chunks +212 lines, -50 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics_unittest.cc View 1 2 3 4 5 6 7 9 chunks +587 lines, -600 lines 0 comments Download
M components/autofill/core/browser/autofill_type.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M components/autofill/core/browser/field_types.h View 1 2 3 4 5 6 7 8 1 chunk +6 lines, -1 line 0 comments Download
M components/autofill/core/browser/form_structure.cc View 1 2 3 4 5 6 7 5 chunks +18 lines, -126 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 5 6 7 8 9 1 chunk +226 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 6 chunks +89 lines, -0 lines 0 comments Download

Messages

Total messages: 66 (46 generated)
sebsg
lgtm
3 years, 7 months ago (2017-05-10 15:07:13 UTC) #1
Roger McFarlane (Chromium)
+jwd (histograms.xml) +privard (sanity check)
3 years, 7 months ago (2017-05-11 14:48:11 UTC) #2
Roger McFarlane (Chromium)
On 2017/05/11 14:48:11, Roger McFarlane (Chromium) wrote: > +jwd (histograms.xml) > +privard (sanity check) s/histograms.xml/enums.xml/
3 years, 7 months ago (2017-05-11 14:48:34 UTC) #3
Roger McFarlane (Chromium)
add jwd and privard for realz
3 years, 7 months ago (2017-05-11 14:49:32 UTC) #6
jwd
nit: add a period at the end of CL description. LGTM, and note: you don't ...
3 years, 7 months ago (2017-05-11 14:56:56 UTC) #7
Roger McFarlane (Chromium)
Thanks. Another look? I've done a couple of things: - Rewritten the text description to ...
3 years, 7 months ago (2017-05-23 20:12:27 UTC) #24
sebsg
Two quick comments, will look more at the metrics to see if it makes sense, ...
3 years, 7 months ago (2017-05-24 14:09:02 UTC) #27
Roger McFarlane (Chromium)
Thanks! Additionally reviewed defunct Rappor metric unit-tests. https://codereview.chromium.org/2870163003/diff/80001/components/autofill/core/browser/autofill_metrics.h File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/80001/components/autofill/core/browser/autofill_metrics.h#newcode324 components/autofill/core/browser/autofill_metrics.h:324: // - ...
3 years, 7 months ago (2017-05-24 15:36:12 UTC) #31
sebsg
Read through your logging and it makes sense. Would be good to have the confirmation ...
3 years, 7 months ago (2017-05-25 20:50:16 UTC) #34
Roger McFarlane (Chromium)
+mathp privard@: ping?
3 years, 7 months ago (2017-05-25 21:01:13 UTC) #36
privard
lgtm
3 years, 7 months ago (2017-05-25 21:21:30 UTC) #39
Mathieu
I would choose a different histogram name and deprecate the old one, otherwise the analysis ...
3 years, 7 months ago (2017-05-26 18:46:45 UTC) #42
Mathieu
Looking good, just a few comments https://codereview.chromium.org/2870163003/diff/140001/components/autofill/core/browser/autofill_metrics.cc File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/core/browser/autofill_metrics.cc#newcode291 components/autofill/core/browser/autofill_metrics.cc:291: ServerFieldType GetFieldType(const ServerFieldTypeSet ...
3 years, 6 months ago (2017-05-29 14:00:19 UTC) #43
Roger McFarlane (Chromium)
Thanks. As suggested by MathP, I've deprecated the old quality metric names. This triggered a ...
3 years, 6 months ago (2017-05-30 21:43:22 UTC) #48
Roger McFarlane (Chromium)
Thanks. As suggested by MathP, I've deprecated the old quality metric names. This triggered a ...
3 years, 6 months ago (2017-05-30 21:43:23 UTC) #49
Mathieu
lgtm, sorry for the delay https://codereview.chromium.org/2870163003/diff/200001/components/autofill/core/browser/autofill_metrics.cc File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/200001/components/autofill/core/browser/autofill_metrics.cc#newcode324 components/autofill/core/browser/autofill_metrics.cc:324: // Collapse the fix ...
3 years, 6 months ago (2017-05-31 20:07:18 UTC) #52
Roger McFarlane (Chromium)
jwd@... can you take another quick look at histograms.xml? I added the Autofill.FieldPrediction.* metrics which ...
3 years, 6 months ago (2017-06-01 20:23:16 UTC) #57
jwd
still LGTM
3 years, 6 months ago (2017-06-02 16:27:59 UTC) #60
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2870163003/260001
3 years, 6 months ago (2017-06-02 18:08:09 UTC) #63
commit-bot: I haz the power
3 years, 6 months ago (2017-06-02 19:59:04 UTC) #66
Message was sent while issue was closed.
Committed patchset #11 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/3c6caa287dacd94ee5e64eedaf44...

Powered by Google App Engine
This is Rietveld 408576698