|
|
Created:
3 years, 7 months ago by Roger McFarlane (Chromium) Modified:
3 years, 6 months ago 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. |
DescriptionLog 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 #Messages
Total messages: 66 (46 generated)
lgtm
+jwd (histograms.xml) +privard (sanity check)
On 2017/05/11 14:48:11, Roger McFarlane (Chromium) wrote: > +jwd (histograms.xml) > +privard (sanity check) s/histograms.xml/enums.xml/
Description was changed from ========== 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 ========== to ========== 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 ==========
rogerm@chromium.org changed reviewers: + jwd@chromium.org, privard@google.com
add jwd and privard for realz
nit: add a period at the end of CL description. LGTM, and note: you don't need our metrics owners for enums.xml changes in the future!
Description was changed from ========== 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 ========== to ========== 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 ==========
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rogerm@chromium.org changed reviewers: + dvadym@chromium.org
Thanks. Another look? I've done a couple of things: - Rewritten the text description to use confusion matrix terminology. The numeric values still correspond to the original semantics, so the order is a bit wonky. - Added a false negative mismatch enum value - Removed the rappor metrics for "too many mismatches". These are not being watched and can be replaced with UKM versions (in a follow-on CL). - Added better unit-testing of all of the confusion matrix categories.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Two quick comments, will look more at the metrics to see if it makes sense, but it looks good https://codereview.chromium.org/2870163003/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:324: // - the crowd-sourced (server) prediction; and, nit: remove the and? https://codereview.chromium.org/2870163003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2870163003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:1852: + <int value="0" label="Ambiguous: False Negative (Unknown)"/> The issue with this is that the previous data collides with these new values. It means you won't be able to use data from <60 for analysis.
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:100001) has been deleted
Thanks! Additionally reviewed defunct Rappor metric unit-tests. https://codereview.chromium.org/2870163003/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:324: // - the crowd-sourced (server) prediction; and, On 2017/05/24 14:09:01, sebsg wrote: > nit: remove the and? Done. https://codereview.chromium.org/2870163003/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/enums.xml (right): https://codereview.chromium.org/2870163003/diff/80001/tools/metrics/histogram... tools/metrics/histograms/enums.xml:1852: + <int value="0" label="Ambiguous: False Negative (Unknown)"/> On 2017/05/24 14:09:02, sebsg wrote: > The issue with this is that the previous data collides with these new values. It > means you won't be able to use data from <60 for analysis. Indeed. I forgot to discuss this in the review request mail. We currently aren't looking at these metrics (they let us answer recall per field type group, but nothing else). So, I'm comfortable abandoning the old data. The values for AutofillTypeQuality on the other hand, are renamed but mapped to the same numeric values, so the old data remains valid, though incomplete (no true negatives of false positives pre 60-ish).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Read through your logging and it makes sense. Would be good to have the confirmation that the logic is sound from some metric people though. LGTM! :) https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:420: // predicted_point != UNKNOWN. predicted_type?
rogerm@chromium.org changed reviewers: + mathp@chromium.org
+mathp privard@: ping?
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I would choose a different histogram name and deprecate the old one, otherwise the analysis becomes very difficult. I'm continuing the review!
Looking good, just a few comments https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:291: ServerFieldType GetFieldType(const ServerFieldTypeSet field_types, GetFieldType is a little vague, can we find a more specific name? https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:308: // TODO(isherman): Improve this logic once we add support for hmm... https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:342: ServerFieldType actual_type = GetFieldType(possible_types, predicted_type); GetActualFieldType perhaps? https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:427: LogUMAHistogramEnumeration( Is it weird two log the same histograms for both reference points (actual and predicted)? Will it be harder to analyze? https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:327: // For each of these prediction types, these metrics are also logger by *logged https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:366: // The field type is UNKNOWN, but autofill predicted it to be of type T. should this say EMPTY
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:160001) has been deleted
Patchset #8 (id:180001) has been deleted
Thanks. As suggested by MathP, I've deprecated the old quality metric names. This triggered a bunch of additional changes to the unittests. I've also added an ambiguous type (for logging purposes only) that captures the difference between us not recognizing the data and us not being able to disambiguate the data. Another look? https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:420: // predicted_point != UNKNOWN. On 2017/05/25 20:50:16, sebsg wrote: > predicted_type? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:291: ServerFieldType GetFieldType(const ServerFieldTypeSet field_types, On 2017/05/29 14:00:19, Mathieu wrote: > GetFieldType is a little vague, can we find a more specific name? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:308: // TODO(isherman): Improve this logic once we add support for On 2017/05/29 14:00:19, Mathieu wrote: > hmm... Indeed. It's not clear to me that we need to do this collapsing thing at all. I wasn't intending to change the counting behaviour in this CL, so I retained the original logic; but, this function could probably be reduced to returning the first item if there's only one, else predicted_type if it's in the set, else UNKNOWN_TYPE. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:342: ServerFieldType actual_type = GetFieldType(possible_types, predicted_type); On 2017/05/29 14:00:19, Mathieu wrote: > GetActualFieldType perhaps? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:427: LogUMAHistogramEnumeration( On 2017/05/29 14:00:19, Mathieu wrote: > Is it weird two log the same histograms for both reference points (actual and > predicted)? Will it be harder to analyze? It's impossible to do some analyses if you don't (because these histograms lose information, retaining either actual or predicted, but not both). https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:327: // For each of these prediction types, these metrics are also logger by On 2017/05/29 14:00:19, Mathieu wrote: > *logged Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:366: // The field type is UNKNOWN, but autofill predicted it to be of type T. On 2017/05/29 14:00:19, Mathieu wrote: > should this say EMPTY Done.
Thanks. As suggested by MathP, I've deprecated the old quality metric names. This triggered a bunch of additional changes to the unittests. I've also added an ambiguous type (for logging purposes only) that captures the difference between us not recognizing the data and us not being able to disambiguate the data. Another look? https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:420: // predicted_point != UNKNOWN. On 2017/05/25 20:50:16, sebsg wrote: > predicted_type? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:291: ServerFieldType GetFieldType(const ServerFieldTypeSet field_types, On 2017/05/29 14:00:19, Mathieu wrote: > GetFieldType is a little vague, can we find a more specific name? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:308: // TODO(isherman): Improve this logic once we add support for On 2017/05/29 14:00:19, Mathieu wrote: > hmm... Indeed. It's not clear to me that we need to do this collapsing thing at all. I wasn't intending to change the counting behaviour in this CL, so I retained the original logic; but, this function could probably be reduced to returning the first item if there's only one, else predicted_type if it's in the set, else UNKNOWN_TYPE. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:342: ServerFieldType actual_type = GetFieldType(possible_types, predicted_type); On 2017/05/29 14:00:19, Mathieu wrote: > GetActualFieldType perhaps? Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:427: LogUMAHistogramEnumeration( On 2017/05/29 14:00:19, Mathieu wrote: > Is it weird two log the same histograms for both reference points (actual and > predicted)? Will it be harder to analyze? It's impossible to do some analyses if you don't (because these histograms lose information, retaining either actual or predicted, but not both). https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:327: // For each of these prediction types, these metrics are also logger by On 2017/05/29 14:00:19, Mathieu wrote: > *logged Done. https://codereview.chromium.org/2870163003/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:366: // The field type is UNKNOWN, but autofill predicted it to be of type T. On 2017/05/29 14:00:19, Mathieu wrote: > should this say EMPTY Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, sorry for the delay https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:324: // Collapse the fix plz :) https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... File components/autofill/core/browser/field_types.h (right): https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... components/autofill/core/browser/field_types.h:168: AMBIGUOUS_TYPE = 96, you'll need to modify the server version according to the top level comment here
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by rogerm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jwd@... can you take another quick look at histograms.xml? I added the Autofill.FieldPrediction.* metrics which are an un-enumerated cross of predicted type and actual type. https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:324: // Collapse the On 2017/05/31 20:07:18, Mathieu wrote: > fix plz :) Done. https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... File components/autofill/core/browser/field_types.h (right): https://codereview.chromium.org/2870163003/diff/200001/components/autofill/co... components/autofill/core/browser/field_types.h:168: AMBIGUOUS_TYPE = 96, On 2017/05/31 20:07:18, Mathieu wrote: > you'll need to modify the server version according to the top level comment here Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
still LGTM
The CQ bit was checked by rogerm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, privard@google.com, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2870163003/#ps260001 (title: "remove confusing 'units' from histograms.xml")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 260001, "attempt_start_ts": 1496426859965570, "parent_rev": "69735df5263dc92d1a3759d4c8e39e50db1a9a2b", "commit_rev": "3c6caa287dacd94ee5e64eedaf44ea987f287bf8"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/3c6caa287dacd94ee5e64eedaf44... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3c6caa287dacd94ee5e64eedaf44... |