UKM collection for Autofill is pretty busted due to AutofillManager resets. |
|||||||||
Issue descriptionWhat is the issue? I think AutofillManager resets almost always before logging Autofill_FormSubmitted. When that happens we create new FormInteractionsUkmLogger with empty url_ and -1 source_id_. It was assumed in the design earlier that the value of of url_ and source_ will be correctly set during form parsing (i.e OnFormsParsed) which it does currently. However after the manager is reset, the logger also resets to empty url. Almost all the metric in FormInteractionsUkmLogger, is gated by CanLog() which checks if url_ is not empty (Btw this check is redundant). Since the calls to recording metrics happens after the manager has reset, these values get recorded from the new logger which doesn't contain url_ hence skips recording the UKMs and we miss the data. This is different from crbug.com/892299 as this happens way more frequently. Autofill manager resets lot more often. Note that metrics collection will be impacted by crbug.com/892299 also because without AutofillManager, there is not FormInteractionLogger as well. But this issue will happen just by resets. How to reproduce this? Fairly consistent to reproduce - Go to - https://dump-truck.appspot.com/usecase-formless_addr_and_cc_on_same_page/addr_and_cc.html Fill default value and submit. I added printfs in AutofillManger.Reset() and AutofillManager.OnFormsParsed() and AutofillMetrics::FormInteractionsUkmLogger::LogFormSubmitted() and this is the result - Created New Form Interaction Logger with SourceID: 73 Parsed Form SourceID:- 77 Created New Form Interaction Logger with SourceID: 77 FormSubmitted SourceID:- 77 The source ID is recorded from AutofillClient::GetUkmSourceId(). Note that the created new Form Interaction Logger was triggered right before form submitted. So, the url_ value set during the Parsed From get cleared.
,
Nov 14
The fix is very small and without this most of our UKM metrics are busted. So, it would be great if can get this merged to M71.
,
Nov 14
Autofill UKM metrics across all the OS are busted.
,
Nov 14
This change is not in canary yet, pls update bug with canary result tomorrow.
,
Nov 14
This bug requires manual review: Less than 16 days to go before AppStore submit on M71 Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Nov 15
The NextAction date has arrived: 2018-11-15
,
Nov 15
How is the change looking in canary?
,
Nov 15
The change is looking okay in the canary. Verified the UKM metrics are collected for https://dump-truck.appspot.com/usecase-formless_addr_and_cc_on_same_page/addr_and_cc.html
,
Nov 15
Approving merge to M71 branch 3578 based on comment #8. Please merge ASAP.
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/833f45c840d5c684badc20791d1a2cad3c886c4a commit 833f45c840d5c684badc20791d1a2cad3c886c4a Author: Nikunj Bhagat <nikunjb@chromium.org> Date: Fri Nov 16 01:00:46 2018 AutofillManager reset shouldn't cause the metrics to be dropped TBR=nikunjb@chromium.org (cherry picked from commit feb4f0d0edb3027d6b1be034aafa38f5fcea8cc0) Bug: 904953 Change-Id: I763d48881143645102fdaf5d85cb2ab812e8b8c0 Reviewed-on: https://chromium-review.googlesource.com/c/1334327 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Commit-Queue: Nik Bhagat <nikunjb@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#607856} Reviewed-on: https://chromium-review.googlesource.com/c/1338384 Reviewed-by: Jared Saul <jsaul@google.com> Cr-Commit-Position: refs/branch-heads/3578@{#711} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034} [modify] https://crrev.com/833f45c840d5c684badc20791d1a2cad3c886c4a/components/autofill/core/browser/autofill_manager.cc [modify] https://crrev.com/833f45c840d5c684badc20791d1a2cad3c886c4a/components/autofill/core/browser/autofill_metrics.cc [modify] https://crrev.com/833f45c840d5c684badc20791d1a2cad3c886c4a/components/autofill/core/browser/autofill_metrics.h [modify] https://crrev.com/833f45c840d5c684badc20791d1a2cad3c886c4a/components/autofill/core/browser/test_autofill_manager.cc
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/833f45c840d5c684badc20791d1a2cad3c886c4a Commit: 833f45c840d5c684badc20791d1a2cad3c886c4a Author: nikunjb@chromium.org Commiter: jsaul@google.com Date: 2018-11-16 01:00:46 +0000 UTC AutofillManager reset shouldn't cause the metrics to be dropped TBR=nikunjb@chromium.org (cherry picked from commit feb4f0d0edb3027d6b1be034aafa38f5fcea8cc0) Bug: 904953 Change-Id: I763d48881143645102fdaf5d85cb2ab812e8b8c0 Reviewed-on: https://chromium-review.googlesource.com/c/1334327 Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org> Commit-Queue: Nik Bhagat <nikunjb@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#607856} Reviewed-on: https://chromium-review.googlesource.com/c/1338384 Reviewed-by: Jared Saul <jsaul@google.com> Cr-Commit-Position: refs/branch-heads/3578@{#711} Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
,
Jan 2
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 14