Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2800853004:
UKM that threads together multiple form interaction events. (Closed)
|
Created:
3 years, 8 months ago by csashi Modified:
3 years, 8 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, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUKM that threads together multiple form interaction events.
BUG=709530
Review-Url: https://codereview.chromium.org/2800853004
Cr-Commit-Position: refs/heads/master@{#464565}
Committed: https://chromium.googlesource.com/chromium/src/+/de1410d69989d28e98ca3bc1d31a18d74dae5352
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moves UkmLogger as parameter to LogAutofillFormSubmittedState. #
Total comments: 4
Patch Set 3 : Updates unit tests. #
Total comments: 14
Patch Set 4 : Renames UkmLogger->FormInteractionsUkmLogger, LogUkm->SubmitUkm. #
Total comments: 6
Patch Set 5 : Keeps metric names instead of metric enums in metrics_ vector. #Patch Set 6 : Adds new metrics to ukm.xml. #Patch Set 7 : Combines metrics for UPI VPA and autocomplete markup hints in a single entry. #Patch Set 8 : Logs each form interaction event in a separate UkmEntry, includes time_delta as a metric. #
Total comments: 10
Patch Set 9 : Moves away from UMA style metrics. Logs more metadata for events. #Patch Set 10 : Logs count of local cards/profiles and server cards/profiles. #Patch Set 11 : Updates autofill_metrics_unittest.cc. #Patch Set 12 : Resolves merge conflicts. Adds 'Autofill' prefix to UKM entry names. #Patch Set 13 : Resolves merge errors. #Patch Set 14 : Uses size_t for local and server record type count. #Messages
Total messages: 45 (23 generated)
csashi@google.com changed reviewers: + sebsg@chromium.org
Hi Sebastien, Please take a first look. I will refine it if this was similar to what you had visualized. Thanks, -sashi.
rogerm@chromium.org changed reviewers: + rogerm@chromium.org
Why not add the ukm logger to the signatures of the existing log methods and update them to log both the old and ukm versions of the metrics? https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:226: ukm_logger_(new AutofillMetrics::UkmLogger(client->GetUkmService())), prefer base::MakeUnique<Foo>(bar, baz, boz) over naked new. ukm_logger_(base:MakeUnique<AutofillMetrics::UkmLogger>(client->GetUkmService())),
On 2017/04/07 02:34:01, Roger McFarlane (Chromium) wrote: > Why not add the ukm logger to the signatures of the existing log methods and > update them to log both the old and ukm versions of the metrics? > Done for |AutofillFormSubmittedState|. For |FormEvent|, UKM logger is embedded inside |FormEventLogger| and effectively a parameter to all the logging functions (via |this|). I initially started adding UKM logger as an explicit parameter, but switched to making it a private member variable because it seemed cleaner. For |UserHappinessMetric|, UMA logs both the metric and the metric_ONCE. Because we are threading events, I do not need the _ONCE. Further, it seems better to log every occurrence of the event (ideally, we could log the timestamps also but perhaps that is too much logging). For |UnmaskPromptEvent|, all logging, except for the first log that we have shown the prompt is happening in the view destructor. It was unwieldy to pass the UKM logger through to the views (there may be some #include restrictions when I cross components/ui too), so I deferred it for now. > https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:226: ukm_logger_(new > AutofillMetrics::UkmLogger(client->GetUkmService())), > prefer base::MakeUnique<Foo>(bar, baz, boz) over naked new. > > ukm_logger_(base:MakeUnique<AutofillMetrics::UkmLogger>(client->GetUkmService())),
Hi, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:226: ukm_logger_(new AutofillMetrics::UkmLogger(client->GetUkmService())), On 2017/04/07 02:34:01, Roger McFarlane (Chromium) wrote: > prefer base::MakeUnique<Foo>(bar, baz, boz) over naked new. > > ukm_logger_(base:MakeUnique<AutofillMetrics::UkmLogger>(client->GetUkmService())), Done.
Hi Sashi, I like the general idea of how you keep the state, I realized I started to do the real review, but I think you just wanted me to take a look at the general idea. Let me know when you want the real review :) https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1925: ukm_logger_->LogUkm(); DCHECK(ukm_logger_)? https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:977: LogUkm(); Wouldn't this cause us to log twice if LogAutofillFormSubmittedState is called and then the object is disposed?
Description was changed from ========== UKM that threads together multiple form interaction events. BUG= ========== to ========== UKM that threads together multiple form interaction events. BUG=709530 ==========
csashi@google.com changed reviewers: + holte@chromium.org
Hi, I have updated the tests and is ready for review. Hi Steven, Can you review changes to components/ukm/test_ukm_service* Thanks, -sashi. https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1925: ukm_logger_->LogUkm(); On 2017/04/07 13:17:24, sebsg wrote: > DCHECK(ukm_logger_)? I am creating ukm_logger_ in the constructor, so I removed all the DCHECK(ukm_logger_) https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:977: LogUkm(); On 2017/04/07 13:17:24, sebsg wrote: > Wouldn't this cause us to log twice if LogAutofillFormSubmittedState is called > and then the object is disposed? I meant to clear |metrics_| after logging but forgot. Fixed now.
The CQ bit was checked by csashi@google.com 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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by csashi@google.com 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: This issue passed the CQ dry run.
Some comments, nice work! https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1398: if (state < AutofillMetrics::AUTOFILL_FORM_SUBMITTED_STATE_ENUM_SIZE) { Do we expect that this is ever bigger than max? If not, I would put a DCHECK instead. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1914: // If user submits a form later, we set the key to the |submitted_form|'s I would reformulate to something like, setup the url for the future metrics for this form. WDYT? https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1004: LogUkm(); Please add a comment to explain why it's useful to log here. Thanks! https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1036: void AutofillMetrics::UkmLogger::LogUkm() { I would change either this one or the others to reflect that this one actually submits the metrics? maybe this one should be SubmitUkm? Or the others RecordUserHappinessMetrics or something? https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:556: class UkmLogger { Do you think we could rename to something like FormInteractionsUkmLogger? This one feels very broad :) https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:364: EXPECT_THAT(entry_proto.metrics(), very nice! https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/form_structure.cc:794: AutofillMetrics::AutofillFormSubmittedState state = I find it weird to return this value, but the function is very very messy, so I understand why you did it. Could you open a but and add a TODO comment here to clean this? leave it available for the autofill team. Thanks!
Hi, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1398: if (state < AutofillMetrics::AUTOFILL_FORM_SUBMITTED_STATE_ENUM_SIZE) { On 2017/04/10 19:20:19, sebsg wrote: > Do we expect that this is ever bigger than max? If not, I would put a DCHECK > instead. Done. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1914: // If user submits a form later, we set the key to the |submitted_form|'s On 2017/04/10 19:20:19, sebsg wrote: > I would reformulate to something like, setup the url for the future metrics for > this form. > > WDYT? Done. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1004: LogUkm(); On 2017/04/10 19:20:19, sebsg wrote: > Please add a comment to explain why it's useful to log here. Thanks! Done. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1036: void AutofillMetrics::UkmLogger::LogUkm() { On 2017/04/10 19:20:19, sebsg wrote: > I would change either this one or the others to reflect that this one actually > submits the metrics? maybe this one should be SubmitUkm? Or the others > RecordUserHappinessMetrics or something? Done. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.h:556: class UkmLogger { On 2017/04/10 19:20:19, sebsg wrote: > Do you think we could rename to something like FormInteractionsUkmLogger? This > one feels very broad :) Done. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics_unittest.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_metrics_unittest.cc:364: EXPECT_THAT(entry_proto.metrics(), On 2017/04/10 19:20:19, sebsg wrote: > very nice! Credit to Zhanyong Wan. https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2800853004/diff/40001/components/autofill/cor... components/autofill/core/browser/form_structure.cc:794: AutofillMetrics::AutofillFormSubmittedState state = On 2017/04/10 19:20:19, sebsg wrote: > I find it weird to return this value, but the function is very very messy, so I > understand why you did it. Could you open a but and add a TODO comment here to > clean this? leave it available for the autofill team. > > Thanks! I noticed that LogQualityMetrics was already taking in a Rappor (another kind of logger), so I added the new logger as an additional parameter. I did not create a bug but I created a TODO to perhaps modify the function name or split it up.
The CQ bit was checked by csashi@google.com 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: This issue passed the CQ dry run.
LGTM, thanks!
Please document your entries in tools/metrics/ukm/ukm.xml https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:742: bool AutofillMetrics::LogUkm( This appears to be logging a source for every entry, which you probably shouldn't do. Instead, you should log the source once and record multiple entries to it. Once we switch to having a global source id, we can just stop adding the source and start using the global one. https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1049: AUTOFILL_FORM_SUBMITTED_STATE: Why store all of this in multiple maps before putting it in UkmEntry? It seems like it would be much simpler to just store this information in UkmLogger members, and then just build the Entry from the members?
Hi Steven, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:742: bool AutofillMetrics::LogUkm( On 2017/04/10 22:42:32, Steven Holte wrote: > This appears to be logging a source for every entry, which you probably > shouldn't do. Instead, you should log the source once and record multiple > entries to it. Once we switch to having a global source id, we can just stop > adding the source and start using the global one. FWIW, I was following the logging pattern that had been committed prior to my change. I can make this change, by keeping the source_id_ in the FormInteractionsUkmLogger (similar to UkmPageLoadMetricsObserver). However, I see 3 issues: 1. FormInteractions has a lifetime that is different from both the CardUploadDecisionMetric and the DeveloperEngagement. CardUploadDecisionMetric is logged after the user's form interaction is complete and the user has submitted the form. DeveloperEngagement is logged when the form is parsed - i.e. even before user has started interacting with the form. 2. DeveloperEngagement is logged for *all* forms, CardUploadDecisionMetric is logged with URL key from form that is submitted, and FormInteractions is logged for with URL key from form that user interacts with (usually same as submitted form). 3. If we adopt this principle of logging the source only once, should we extend it to UkmPageLoadMetricsObserver also? I can make the change such that all logging from autofill_manager log the source just once with some additional state in autofill_manager. Please confirm. https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1049: AUTOFILL_FORM_SUBMITTED_STATE: On 2017/04/10 22:42:31, Steven Holte wrote: > Why store all of this in multiple maps before putting it in UkmEntry? > > It seems like it would be much simpler to just store this information in > UkmLogger members, and then just build the Entry from the members? I am not using 'maps' - I have a vector of metrics. The same metric name could be repeated. For example, we may show a suggestion, the user may edit, then we show a suggestion again and so on. - we want to capture this 'session'. In case you were referring to this switch statement as the 'map', I have removed the switch.
On 2017/04/11 00:40:23, csashi wrote: > Hi Steven, > Please take a look. Thanks! > -sashi. > > https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... > File components/autofill/core/browser/autofill_metrics.cc (right): > > https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics.cc:742: bool > AutofillMetrics::LogUkm( > On 2017/04/10 22:42:32, Steven Holte wrote: > > This appears to be logging a source for every entry, which you probably > > shouldn't do. Instead, you should log the source once and record multiple > > entries to it. Once we switch to having a global source id, we can just stop > > adding the source and start using the global one. > > FWIW, I was following the logging pattern that had been committed prior to my > change. I can make this change, by keeping the source_id_ in the > FormInteractionsUkmLogger (similar to UkmPageLoadMetricsObserver). However, I > see 3 issues: > > 1. FormInteractions has a lifetime that is different from both the > CardUploadDecisionMetric and the DeveloperEngagement. CardUploadDecisionMetric > is logged after the user's form interaction is complete and the user has > submitted the form. DeveloperEngagement is logged when the form is parsed - i.e. > even before user has started interacting with the form. > > 2. DeveloperEngagement is logged for *all* forms, CardUploadDecisionMetric is > logged with URL key from form that is submitted, and FormInteractions is logged > for with URL key from form that user interacts with (usually same as submitted > form). > > 3. If we adopt this principle of logging the source only once, should we extend > it to UkmPageLoadMetricsObserver also? > > I can make the change such that all logging from autofill_manager log the source > just once with some additional state in autofill_manager. Please confirm. > > https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... > components/autofill/core/browser/autofill_metrics.cc:1049: > AUTOFILL_FORM_SUBMITTED_STATE: > On 2017/04/10 22:42:31, Steven Holte wrote: > > Why store all of this in multiple maps before putting it in UkmEntry? > > > > It seems like it would be much simpler to just store this information in > > UkmLogger members, and then just build the Entry from the members? > > I am not using 'maps' - I have a vector of metrics. The same metric name could > be repeated. For example, we may show a suggestion, the user may edit, then we > show a suggestion again and so on. - we want to capture this 'session'. > > In case you were referring to this switch statement as the 'map', I have removed > the switch. Hi, Per comment in https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor..., I have combined the metrics for UPI Virtual-Payment-Address and autocomplete markup type hints into a single entry. Please take a look. Thanks! -sashi.
https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1049: AUTOFILL_FORM_SUBMITTED_STATE: On 2017/04/11 00:40:23, csashi wrote: > On 2017/04/10 22:42:31, Steven Holte wrote: > > Why store all of this in multiple maps before putting it in UkmEntry? > > > > It seems like it would be much simpler to just store this information in > > UkmLogger members, and then just build the Entry from the members? > > I am not using 'maps' - I have a vector of metrics. The same metric name could > be repeated. For example, we may show a suggestion, the user may edit, then we > show a suggestion again and so on. - we want to capture this 'session'. > > In case you were referring to this switch statement as the 'map', I have removed > the switch. If I understand this right, you intending to infer the order of edits and fills from the order of metrics within an entry? I don't think we have an intention to preserve that order during processing of this data. It may make more sense for you to send separate entries for each of your events and have something to order them by, either a timestamp or a counter. (though you can still batch them if they are only important for the context of the next event). E.g. for some form, you might record AutofillEvent { FormType: #CreditCard EventCounter: 0 HadTypedData: 0 } AutofillEvent { FormType: #CreditCard EventCounter: 1 HadEdits: 1 } AutofillSubmitEvent { FormType: #CreditCard EventCounter: 2 HadEdits: 1 }
Hi, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:1049: AUTOFILL_FORM_SUBMITTED_STATE: On 2017/04/11 17:37:57, Steven Holte wrote: > On 2017/04/11 00:40:23, csashi wrote: > > On 2017/04/10 22:42:31, Steven Holte wrote: > > > Why store all of this in multiple maps before putting it in UkmEntry? > > > > > > It seems like it would be much simpler to just store this information in > > > UkmLogger members, and then just build the Entry from the members? > > > > I am not using 'maps' - I have a vector of metrics. The same metric name could > > be repeated. For example, we may show a suggestion, the user may edit, then we > > show a suggestion again and so on. - we want to capture this 'session'. > > > > In case you were referring to this switch statement as the 'map', I have > removed > > the switch. > > If I understand this right, you intending to infer the order of edits and fills > from the order of metrics within an entry? I don't think we have an intention > to preserve that order during processing of this data. It may make more sense > for you to send separate entries for each of your events and have something to > order them by, either a timestamp or a counter. (though you can still batch them > if they are only important for the context of the next event). > > E.g. for some form, you might record > AutofillEvent { > FormType: #CreditCard > EventCounter: 0 > HadTypedData: 0 > } > AutofillEvent { > FormType: #CreditCard > EventCounter: 1 > HadEdits: 1 > } > AutofillSubmitEvent { > FormType: #CreditCard > EventCounter: 2 > HadEdits: 1 > } Done. Your understanding is correct. I was intending to infer the order of events from the order of the metrics. If we don't intend to preserve the order, it may be worthwhile to document that explicitly because https://developers.google.com/protocol-buffers/docs/encoding#optional says: """ The order of the elements with respect to each other is preserved when parsing, though the ordering with respect to other fields is lost. """
https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:837: DCHECK(is_for_credit_card_); This seems like a much more natural place to log the event from, since most of this seems to be to try to flatten / multiply things for UMA, which is probably not helpful for UKM, since you can just add the information to the event. e.g. std::unique_ptr<ukm::UkmEntryBuilder> builder = ukm_service_->GetEntryBuilder(source_id, "FormEvent"); builder->AddMetric("EventType", FILL_SUGGESTION); builder->AddMetric(internal::kUKMMillisecondsSinceFormLoadedMetricName, GetTimeSinceFormLoaded()); builder->AddMetric("RecordType", credit_card.record_type()); // You could pack builder->AddMetric("IsContextSecure", is_context_secure_); builder->AddMetric("FilledBefore", has_logged_suggestion_filled_); has_logged_suggestion_filled_=true; https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:1044: const int32_t source_id = ukm_service_->GetNewSourceID(); Maybe call GetNewSourceID() in your constructor, Call UpdateSourceURL when you set url_, and GetEntryBuilder in AddEvent? There doesn't seem to be a reason to wait for SubmitUkm() https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:82: <metric name="UnmaskPromptEvent"/> It seems like this is always AutoFill::UNMASK_PROMPT_SHOWN. Is this needed? https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:85: <event name="UserHappinessMetric"> A lot of these metric/events names seem pretty ambiguous. It would be helpful if you describe these in terms of how they tell you what has happened on the client.
Hi Steven, Thanks for the detailed comments. When I started this change, my objective was to simply thread together UMA events that occur in a single session. I agree with your recommendations and have made an initial pass. I will update the tests once you review the changes. Thanks, -sashi. https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:837: DCHECK(is_for_credit_card_); On 2017/04/12 02:26:30, Steven Holte wrote: > This seems like a much more natural place to log the event from, since most of > this seems to be to try to flatten / multiply things for UMA, which is probably > not helpful for UKM, since you can just add the information to the event. > > e.g. > > std::unique_ptr<ukm::UkmEntryBuilder> builder = > ukm_service_->GetEntryBuilder(source_id, "FormEvent"); > builder->AddMetric("EventType", FILL_SUGGESTION); > builder->AddMetric(internal::kUKMMillisecondsSinceFormLoadedMetricName, > GetTimeSinceFormLoaded()); > builder->AddMetric("RecordType", credit_card.record_type()); > // You could pack > builder->AddMetric("IsContextSecure", is_context_secure_); > builder->AddMetric("FilledBefore", has_logged_suggestion_filled_); > has_logged_suggestion_filled_=true; > > > > Done. https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:1044: const int32_t source_id = ukm_service_->GetNewSourceID(); On 2017/04/12 02:26:30, Steven Holte wrote: > Maybe call GetNewSourceID() in your constructor, > Call UpdateSourceURL when you set url_, > and GetEntryBuilder in AddEvent? > > There doesn't seem to be a reason to wait for SubmitUkm() Done. I wasn't sure of when the logs will get "flushed"? Is it possible that logs will be sent before we have updated with the correct URL? https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:82: <metric name="UnmaskPromptEvent"/> On 2017/04/12 02:26:31, Steven Holte wrote: > It seems like this is always AutoFill::UNMASK_PROMPT_SHOWN. Is this needed? Acknowledged. I was planning on adding other UnmaskPromptEvents too but they get logged at different locations. To limit the scope of this change, I just logged UNMASK_PROMPT_SHOWN, but it would be better to add it back when I have all the unmask events. https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:85: <event name="UserHappinessMetric"> On 2017/04/12 02:26:31, Steven Holte wrote: > A lot of these metric/events names seem pretty ambiguous. It would be helpful > if you describe these in terms of how they tell you what has happened on the > client. Acknowledged. I will update up this after you review the changes to the metrics/entries.
Hi Steven, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.xml File tools/metrics/ukm/ukm.xml (right): https://codereview.chromium.org/2800853004/diff/140001/tools/metrics/ukm/ukm.... tools/metrics/ukm/ukm.xml:85: <event name="UserHappinessMetric"> On 2017/04/12 05:53:59, csashi wrote: > On 2017/04/12 02:26:31, Steven Holte wrote: > > A lot of these metric/events names seem pretty ambiguous. It would be helpful > > if you describe these in terms of how they tell you what has happened on the > > client. > > Acknowledged. I will update up this after you review the changes to the > metrics/entries. Done.
Thanks for updating this! The UKM logging generally looks good now. Please update the descriptions before submitting (in particular, for fields that use some enum coding, reference where the value meanings can be found), but otherwise lgtm https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2800853004/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.cc:1044: const int32_t source_id = ukm_service_->GetNewSourceID(); On 2017/04/12 05:53:58, csashi wrote: > On 2017/04/12 02:26:30, Steven Holte wrote: > > Maybe call GetNewSourceID() in your constructor, > > Call UpdateSourceURL when you set url_, > > and GetEntryBuilder in AddEvent? > > > > There doesn't seem to be a reason to wait for SubmitUkm() > > Done. I wasn't sure of when the logs will get "flushed"? Is it possible that > logs will be sent before we have updated with the correct URL? It is possible that the Entry and Source will be sent in different uploads, but that's expected to be a typical case with UKM logs. When analyzing the logs, the first step should usually be to join by session_id + source_id across multiple logs. This is pretty quick and simple to do with the raw data that we have currently, and we are currently building a sessionized logs source where this step will be done already.
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2800853004/#ps240001 (title: "Resolves merge errors.")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, holte@chromium.org Link to the patchset: https://codereview.chromium.org/2800853004/#ps260001 (title: "Uses size_t for local and server record type count.")
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": 1492117855295820,
"parent_rev": "8f979492a4af4fd07a0a10e614dc2e0b7abea91a", "commit_rev":
"de1410d69989d28e98ca3bc1d31a18d74dae5352"}
Message was sent while issue was closed.
Description was changed from ========== UKM that threads together multiple form interaction events. BUG=709530 ========== to ========== UKM that threads together multiple form interaction events. BUG=709530 Review-Url: https://codereview.chromium.org/2800853004 Cr-Commit-Position: refs/heads/master@{#464565} Committed: https://chromium.googlesource.com/chromium/src/+/de1410d69989d28e98ca3bc1d31a... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/de1410d69989d28e98ca3bc1d31a... |
