Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2849753002:
Logs all reasons card upload was not offered in UKM and UMA. (Closed)
|
Created:
3 years, 7 months ago by csashi Modified:
3 years, 7 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. |
DescriptionLogs all reasons card upload was not offered in UKM and UMA.
UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets.
Other buckets count the specific reasons.
Fixes DeveloperEngagementUkm to also use bit mask instead of separate metrics.
BUG=713842
Review-Url: https://codereview.chromium.org/2849753002
Cr-Commit-Position: refs/heads/master@{#468852}
Committed: https://chromium.googlesource.com/chromium/src/+/bc9f3b2b440eac0f15e651a9c3fd38eb40c849bf
Patch Set 1 #
Total comments: 18
Patch Set 2 : Encodes enum metrics as bitmask. #Patch Set 3 : Encodes enum metrics as bitmask. #
Total comments: 3
Patch Set 4 : Replaces CardUploadDecisionExpanded with CardUploadDecisionMetric. #
Total comments: 8
Patch Set 5 : Uses bitmask to log both UKM and UMA metrics. #Patch Set 6 : Uses bitmask to log both UKM and UMA metrics. #
Total comments: 2
Patch Set 7 : Removes redundant bits for UPLOAD_OFFERED and UPLOAD_NOT_OFFERED. #
Total comments: 3
Patch Set 8 : Formatting fix. #
Total comments: 4
Messages
Total messages: 65 (34 generated)
Description was changed from ========== Logs all reasons card upload was not offered in UKM. BUG=71342 ========== to ========== Logs all reasons card upload was not offered in UKM and UMA. BUG=71342 ==========
csashi@google.com changed reviewers: + jsaul@google.com, sebsg@chromium.org
Hi, Please take a look. Thanks, -sashi.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
On 2017/04/28 05:25:00, csashi wrote: > Hi, > Please take a look. Thanks, -sashi. Hi, I need to update the unit tests. Please take a quick first look, if you can. Thanks! -sashi.
Very glad to see this change! Some comments: https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && Do you think we could log something like "Not able to get profiles" if get_profile_succeeded is false? It feels weird to me to record no CVC if we could not get the profiles. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); Do you think you could pass the vector here too like you did for the UKM. EDIT: Actually I think this would also benefit from the bitfield I describe in another comment :P https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_metrics.cc:739: {internal::kUKMCardUploadDecisionMetricName, static_cast<int>(it)}); This works, but what do you think about a bitfield? I feel like it would better represent your data and maybe easier to manipulate? You'd be able to do something like upload_decision |= DECISION_NO_ADDRESS; then later upload_decision |= DECISION_NO_CVC etc. And then you can record as one metric :)
https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1261: bool get_profiles_succeeded = GetProfilesForCreditCardUpload( Is this new formatting the result of a tool? It's weird that it purposefully breaks the rectangle rule. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && On 2017/04/28 15:26:24, sebsg wrote: > Do you think we could log something like "Not able to get profiles" if > get_profile_succeeded is false? It feels weird to me to record no CVC if we > could not get the profiles. I think this is actually correct. In this branch, the CVC is missing. If there are OTHER errors (aka get_profile_succeeded is false) then we want to additionally log NO_CVC. If there are NOT other errors, we can still offer to save if the experiment is enabled. Additionally, "not able to get profiles" is not necessary because the specific reasons we couldn't get profiles is already populated. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); On 2017/04/28 15:26:24, sebsg wrote: > Do you think you could pass the vector here too like you did for the UKM. > > EDIT: Actually I think this would also benefit from the bitfield I describe in > another comment :P I'm definitely against doing it this way. Different users will log different numbers of failures. It might be nice from a standpoint of "NO_CVC was hit X times", but it will completely wreck the "X% of users hit NO_CVC". I think the bitfield suggestion would fix this? https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:2261: upload_decision_metrics); still nit but very confused about style formatting: the change happening here appears to be the exact opposite of the change happening at the top of the .h file
Hi, Still have to fix some unit tests and add new ones but please take a look if you can. Thanks! -sashi. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1261: bool get_profiles_succeeded = GetProfilesForCreditCardUpload( On 2017/04/28 19:21:42, Jared Saul wrote: > Is this new formatting the result of a tool? It's weird that it purposefully > breaks the rectangle rule. I am using git cl format, which is the tool recommended by git cl upload. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && On 2017/04/28 15:26:24, sebsg wrote: > Do you think we could log something like "Not able to get profiles" if > get_profile_succeeded is false? It feels weird to me to record no CVC if we > could not get the profiles. Done. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && On 2017/04/28 19:21:42, Jared Saul wrote: > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think we could log something like "Not able to get profiles" if > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > could not get the profiles. > > I think this is actually correct. In this branch, the CVC is missing. If there > are OTHER errors (aka get_profile_succeeded is false) then we want to > additionally log NO_CVC. If there are NOT other errors, we can still offer to > save if the experiment is enabled. > > Additionally, "not able to get profiles" is not necessary because the specific > reasons we couldn't get profiles is already populated. Acknowledged. I modified the conditions to perhaps make it easier to parse. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); On 2017/04/28 15:26:24, sebsg wrote: > Do you think you could pass the vector here too like you did for the UKM. > > EDIT: Actually I think this would also benefit from the bitfield I describe in > another comment :P Done. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); On 2017/04/28 19:21:42, Jared Saul wrote: > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think you could pass the vector here too like you did for the UKM. > > > > EDIT: Actually I think this would also benefit from the bitfield I describe in > > another comment :P > > I'm definitely against doing it this way. Different users will log different > numbers of failures. It might be nice from a standpoint of "NO_CVC was hit X > times", but it will completely wreck the "X% of users hit NO_CVC". I think the > bitfield suggestion would fix this? Thanks for the catch. Bit field will fix this for UKM. In theory, we could log the bit field for histogram enumeration also. For e.g. Failures X and Y will be 1 bucket, Failure X will be another bucket, Failure Y will be another bucket and so on. But that seems like an abuse of the histogram enumeration (not to mention # buckets increase as 2**N).I created a new -1 Enumerator for NOT_OFFERED so we don't conflict with existing constants. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_metrics.cc:739: {internal::kUKMCardUploadDecisionMetricName, static_cast<int>(it)}); On 2017/04/28 15:26:24, sebsg wrote: > This works, but what do you think about a bitfield? I feel like it would better > represent your data and maybe easier to manipulate? You'd be able to do > something like > > upload_decision |= DECISION_NO_ADDRESS; > > then later > > upload_decision |= DECISION_NO_CVC > > etc. > > And then you can record as one metric :) Done.
Description was changed from ========== Logs all reasons card upload was not offered in UKM and UMA. BUG=71342 ========== to ========== Logs all reasons card upload was not offered in UKM and UMA. BUG=713842 ==========
On 2017/04/28 23:47:45, csashi wrote: > Hi, > Still have to fix some unit tests and add new ones but please take a look if you > can. Thanks! > -sashi. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1261: bool > get_profiles_succeeded = GetProfilesForCreditCardUpload( > On 2017/04/28 19:21:42, Jared Saul wrote: > > Is this new formatting the result of a tool? It's weird that it purposefully > > breaks the rectangle rule. > > I am using git cl format, which is the tool recommended by git cl upload. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1272: if > (get_profiles_succeeded && > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think we could log something like "Not able to get profiles" if > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > could not get the profiles. > > Done. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1272: if > (get_profiles_succeeded && > On 2017/04/28 19:21:42, Jared Saul wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think we could log something like "Not able to get profiles" if > > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > > could not get the profiles. > > > > I think this is actually correct. In this branch, the CVC is missing. If > there > > are OTHER errors (aka get_profile_succeeded is false) then we want to > > additionally log NO_CVC. If there are NOT other errors, we can still offer to > > save if the experiment is enabled. > > > > Additionally, "not able to get profiles" is not necessary because the specific > > reasons we couldn't get profiles is already populated. > > Acknowledged. I modified the conditions to perhaps make it easier to parse. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1282: > AutofillMetrics::LogCardUploadDecisionMetric(it); > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think you could pass the vector here too like you did for the UKM. > > > > EDIT: Actually I think this would also benefit from the bitfield I describe in > > another comment :P > > Done. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1282: > AutofillMetrics::LogCardUploadDecisionMetric(it); > On 2017/04/28 19:21:42, Jared Saul wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think you could pass the vector here too like you did for the UKM. > > > > > > EDIT: Actually I think this would also benefit from the bitfield I describe > in > > > another comment :P > > > > I'm definitely against doing it this way. Different users will log different > > numbers of failures. It might be nice from a standpoint of "NO_CVC was hit X > > times", but it will completely wreck the "X% of users hit NO_CVC". I think > the > > bitfield suggestion would fix this? > > Thanks for the catch. Bit field will fix this for UKM. In theory, we could log > the bit field for histogram enumeration also. For e.g. Failures X and Y will be > 1 bucket, Failure X will be another bucket, Failure Y will be another bucket and > so on. But that seems like an abuse of the histogram enumeration (not to mention > # buckets increase as 2**N).I created a new -1 Enumerator for NOT_OFFERED so we > don't conflict with existing constants. > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > File components/autofill/core/browser/autofill_metrics.cc (right): > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_metrics.cc:739: > {internal::kUKMCardUploadDecisionMetricName, static_cast<int>(it)}); > On 2017/04/28 15:26:24, sebsg wrote: > > This works, but what do you think about a bitfield? I feel like it would > better > > represent your data and maybe easier to manipulate? You'd be able to do > > something like > > > > upload_decision |= DECISION_NO_ADDRESS; > > > > then later > > > > upload_decision |= DECISION_NO_CVC > > > > etc. > > > > And then you can record as one metric :) > > Done. Hi, Fixed unit tests. Please take a look. Thanks! -sashi.
Description was changed from ========== Logs all reasons card upload was not offered in UKM and UMA. BUG=713842 ========== to ========== Logs all reasons card upload was not offered in UKM and UMA. UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets. Other buckets count the specific reasons. BUG=713842 ==========
Description was changed from ========== Logs all reasons card upload was not offered in UKM and UMA. UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets. Other buckets count the specific reasons. BUG=713842 ========== to ========== Logs all reasons card upload was not offered in UKM and UMA. UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets. Other buckets count the specific reasons. Fixes DeveloperEngagementUkm to also use bit mask instead of separate metrics. BUG=713842 ==========
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...
csashi@google.com changed reviewers: + holte@chromium.org
Hi Steven, Please review tools/metrics. Thanks! -sashi.
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_...)
Nice improvements! Some comments, could you also add in your description that you add a pref? Thank you! https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && On 2017/04/28 23:47:45, csashi wrote: > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think we could log something like "Not able to get profiles" if > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > could not get the profiles. > > Done. Thank you! https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); On 2017/04/28 23:47:45, csashi wrote: > On 2017/04/28 15:26:24, sebsg wrote: > > Do you think you could pass the vector here too like you did for the UKM. > > > > EDIT: Actually I think this would also benefit from the bitfield I describe in > > another comment :P > > Done. Yeah that's how I did it for Payment Request, one bit per event, so there will be different buckets for X and Y VS just X like you said. However I found it to be pretty easy to split them myself when I want to query the data in the analysis tools. So I think it would be a good idea for uma data too :) https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1060: card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED; For bitmasks like this, I usually prefer when they are defined with their bit value. Since these enum values are only used in this context, I think it would be possible. For example, you could have upload offered be 0, so as soon as something else is set, you know it was not offered. Here is an example of when I use it, for the syntax. The "Event" enum in JourneyLogger: https://cs.chromium.org/chromium/src/components/payments/core/journey_logger....
https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1272: if (get_profiles_succeeded && On 2017/05/01 02:46:20, sebsg wrote: > On 2017/04/28 23:47:45, csashi wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think we could log something like "Not able to get profiles" if > > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > > could not get the profiles. > > > > Done. > > Thank you! Acknowledged. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1282: AutofillMetrics::LogCardUploadDecisionMetric(it); On 2017/05/01 02:46:20, sebsg wrote: > On 2017/04/28 23:47:45, csashi wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think you could pass the vector here too like you did for the UKM. > > > > > > EDIT: Actually I think this would also benefit from the bitfield I describe > in > > > another comment :P > > > > Done. > > Yeah that's how I did it for Payment Request, one bit per event, so there will > be different buckets for X and Y VS just X like you said. However I found it to > be pretty easy to split them myself when I want to query the data in the > analysis tools. > > So I think it would be a good idea for uma data too :) Acknowledged. https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:2261: upload_decision_metrics); On 2017/04/28 19:21:42, Jared Saul wrote: > still nit but very confused about style formatting: the change happening here > appears to be the exact opposite of the change happening at the top of the .h > file Acknowledged. https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1060: card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED; On 2017/05/01 02:46:20, sebsg wrote: > For bitmasks like this, I usually prefer when they are defined with their bit > value. Since these enum values are only used in this context, I think it would > be possible. > > For example, you could have upload offered be 0, so as soon as something else is > set, you know it was not offered. > > Here is an example of when I use it, for the syntax. The "Event" enum in > JourneyLogger: > https://cs.chromium.org/chromium/src/components/payments/core/journey_logger.... Can you confirm that I can change the enum values even though these values are being used in prior Chrome releases for UMA logging? Granted, I am logging more metrics in the new release that we did not log before but they are the same values. For Card Upload Decision metric, we cannot conclude that if value != 0, then upload was not offered. This is because we sometimes offer upload even if some fields are missing (currently only CVC, but can be expanded).
https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1060: card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED; On 2017/05/01 16:01:56, csashi wrote: > On 2017/05/01 02:46:20, sebsg wrote: > > For bitmasks like this, I usually prefer when they are defined with their bit > > value. Since these enum values are only used in this context, I think it would > > be possible. > > > > For example, you could have upload offered be 0, so as soon as something else > is > > set, you know it was not offered. > > > > Here is an example of when I use it, for the syntax. The "Event" enum in > > JourneyLogger: > > > https://cs.chromium.org/chromium/src/components/payments/core/journey_logger.... > > Can you confirm that I can change the enum values even though these values are > being used in prior Chrome releases for UMA logging? Granted, I am logging more > metrics in the new release that we did not log before but they are the same > values. > > For Card Upload Decision metric, we cannot conclude that if value != 0, then > upload was not offered. This is because we sometimes offer upload even if some > fields are missing (currently only CVC, but can be expanded). Ah I see, didn't know that you could offer upload in those cases too. And for the UMA, I think it would have to be a new histogram for the reasons you mentioned, sorry for the confusion. Creating a new one and deprecating the old one is not ideal, but I feel like the new way gives us much more valuable information so I think it's worth it. you will still be able to see the old data if you by accessing the old histogram. This will also allow you to change the enum values since they will only be used in the new UMA and UKM way of logging. Thanks you for your hard work.
On 2017/05/01 16:10:17, sebsg wrote: > https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... > components/autofill/core/browser/autofill_manager.cc:1060: > card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED; > On 2017/05/01 16:01:56, csashi wrote: > > On 2017/05/01 02:46:20, sebsg wrote: > > > For bitmasks like this, I usually prefer when they are defined with their > bit > > > value. Since these enum values are only used in this context, I think it > would > > > be possible. > > > > > > For example, you could have upload offered be 0, so as soon as something > else > > is > > > set, you know it was not offered. > > > > > > Here is an example of when I use it, for the syntax. The "Event" enum in > > > JourneyLogger: > > > > > > https://cs.chromium.org/chromium/src/components/payments/core/journey_logger.... > > > > Can you confirm that I can change the enum values even though these values are > > being used in prior Chrome releases for UMA logging? Granted, I am logging > more > > metrics in the new release that we did not log before but they are the same > > values. > > > > For Card Upload Decision metric, we cannot conclude that if value != 0, then > > upload was not offered. This is because we sometimes offer upload even if some > > fields are missing (currently only CVC, but can be expanded). > > Ah I see, didn't know that you could offer upload in those cases too. > > And for the UMA, I think it would have to be a new histogram for the reasons you > mentioned, sorry for the confusion. > > Creating a new one and deprecating the old one is not ideal, but I feel like the > new way gives us much more valuable information so I think it's worth it. you > will still be able to see the old data if you by accessing the old histogram. > > This will also allow you to change the enum values since they will only be used > in the new UMA and UKM way of logging. > > Thanks you for your hard work. I changed the histogram name because we are changing the counts. I will change the enum value if you can confirm that it does not make any difference for the histogram data structure efficiency whether the bucket values are contiguous small integers vs. values that have large separations. Thanks!
Thanks :) Could you use the same variable to store the values for both UKM and UMA? Then you can iterate on the bits in AutofillMetrics and log each reason individually. This would allow you to make the enum values be bits, which would make the code clearer. For example, you could will be able to do upload_decision_metrics |= AutofillMetrics::UPLOAD_NOT_OFFERED_NO_CVC; Thank you!
https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1087: AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED); It feels kind of flaky that if anything fails (or succeeds), the caller needs to recognize and remember that UPLOAD_NOT_OFFERED or UPLOAD_OFFERED also needs to be logged. I was going to suggest a helper method for fool-proof-ness, but I guess that would log UPLOAD_NOT_OFFERED or UPLOAD_OFFERED multiple times, which is worse. I guess my comment here is just an optional "I wish it could be safer to be extended by future developers, but I don't have any ideas offhand." https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:82: I have no idea what this is; can you please add a comment above it explaining it?
On 2017/05/01 02:46:21, sebsg wrote: > Nice improvements! > > Some comments, could you also add in your description that you add a pref? Hi, I am not adding a new pref. Can you clarify? Thanks, -sashi. > > Thank you! > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1272: if > (get_profiles_succeeded && > On 2017/04/28 23:47:45, csashi wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think we could log something like "Not able to get profiles" if > > > get_profile_succeeded is false? It feels weird to me to record no CVC if we > > > could not get the profiles. > > > > Done. > > Thank you! > > https://codereview.chromium.org/2849753002/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1282: > AutofillMetrics::LogCardUploadDecisionMetric(it); > On 2017/04/28 23:47:45, csashi wrote: > > On 2017/04/28 15:26:24, sebsg wrote: > > > Do you think you could pass the vector here too like you did for the UKM. > > > > > > EDIT: Actually I think this would also benefit from the bitfield I describe > in > > > another comment :P > > > > Done. > > Yeah that's how I did it for Payment Request, one bit per event, so there will > be different buckets for X and Y VS just X like you said. However I found it to > be pretty easy to split them myself when I want to query the data in the > analysis tools. > > So I think it would be a good idea for uma data too :) > > https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2849753002/diff/40001/components/autofill/cor... > components/autofill/core/browser/autofill_manager.cc:1060: > card_upload_decision_metrics = 1 << AutofillMetrics::UPLOAD_OFFERED; > For bitmasks like this, I usually prefer when they are defined with their bit > value. Since these enum values are only used in this context, I think it would > be possible. > > For example, you could have upload offered be 0, so as soon as something else is > set, you know it was not offered. > > Here is an example of when I use it, for the syntax. The "Event" enum in > JourneyLogger: > https://cs.chromium.org/chromium/src/components/payments/core/journey_logger....
https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1064: card_upload_decision_metrics |= 1 Are you always calling AutofillMetrics::LogCardUploadDecisionMetric for each of the individual bits you are setting in card_upload_decision_metrics? If so, maybe just loop over the bits and report them at the same point you would call LogCardUploadDecisionsUkm?
Hi, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1064: card_upload_decision_metrics |= 1 On 2017/05/02 00:36:33, Steven Holte wrote: > Are you always calling AutofillMetrics::LogCardUploadDecisionMetric for each of > the individual bits you are setting in card_upload_decision_metrics? > > If so, maybe just loop over the bits and report them at the same point you would > call LogCardUploadDecisionsUkm? Done. Sebastien made same suggestion. https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1087: AutofillMetrics::UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED); On 2017/05/01 21:05:12, Jared Saul wrote: > It feels kind of flaky that if anything fails (or succeeds), the caller needs to > recognize and remember that UPLOAD_NOT_OFFERED or UPLOAD_OFFERED also needs to > be logged. I was going to suggest a helper method for fool-proof-ness, but I > guess that would log UPLOAD_NOT_OFFERED or UPLOAD_OFFERED multiple times, which > is worse. I guess my comment here is just an optional "I wish it could be safer > to be extended by future developers, but I don't have any ideas offhand." Acknowledged. The best I could think of was to set the UPLOAD_OFFERED and UPLOAD_NOT_OFFERED bits in the consolidated LogCardUploadDecisions function - but I would need a switch to look at every other enum and set either of these 2 bits - not foolproof either. https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:82: On 2017/05/01 21:05:12, Jared Saul wrote: > I have no idea what this is; can you please add a comment above it explaining > it? Seemed self-explanatory: https://cs.chromium.org/chromium/src/components/autofill/core/common/autofill...
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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) 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...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 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...
https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:82: On 2017/05/02 00:43:56, csashi wrote: > On 2017/05/01 21:05:12, Jared Saul wrote: > > I have no idea what this is; can you please add a comment above it explaining > > it? > > Seemed self-explanatory: > https://cs.chromium.org/chromium/src/components/autofill/core/common/autofill... I think "previous" is what's throwing me off here, because I'm finding it hard to make a clear assumption on what the timeframe of "previous" is. Does Chrome actually remember what you picked last time it offered to save, even if it was like a year ago? I thought Chrome didn't have anything like that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2849753002/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:135: UPLOAD_NOT_OFFERED = 1 << 9, Does it make sense to simplify some of these bits? e.g. will you always have set one of UPLOAD_OFFERED | UPLOAD_NOT_OFFERED? It seems like you always record record some together as well, e.g. UPLOAD_NOT_OFFERED & UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED, so the second bit isn't exactly adding the UPLOAD_NOT_OFFERED information itself. Maybe you could simplify this down to a list of conditions, e.g: OFFER_DECIDED, (Not sure if you need?) OFFERED, NO_CVC, NO_ADDRESS, NO_NAME, CONFLICTING_NAMES, NO_ZIP, CONFLICTING_ZIP, GET_UPLOAD_DETAILS_FAILED
Hi, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_metrics.cc (right): https://codereview.chromium.org/2849753002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_metrics.cc:82: On 2017/05/02 01:39:45, Jared Saul wrote: > On 2017/05/02 00:43:56, csashi wrote: > > On 2017/05/01 21:05:12, Jared Saul wrote: > > > I have no idea what this is; can you please add a comment above it > explaining > > > it? > > > > Seemed self-explanatory: > > > https://cs.chromium.org/chromium/src/components/autofill/core/common/autofill... > > I think "previous" is what's throwing me off here, because I'm finding it hard > to make a clear assumption on what the timeframe of "previous" is. Does Chrome > actually remember what you picked last time it offered to save, even if it was > like a year ago? I thought Chrome didn't have anything like that. That is a good question, but is orthogonal to the documentation for this function though. All this function is doing is converting that enum to a string. I am using the same preferences that we use to remember if someone accepted a checkbox. Whatever timeframe applies to that choice, should apply to this as well. https://codereview.chromium.org/2849753002/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:135: UPLOAD_NOT_OFFERED = 1 << 9, On 2017/05/02 19:33:20, Steven Holte wrote: > Does it make sense to simplify some of these bits? e.g. will you always have > set one of UPLOAD_OFFERED | UPLOAD_NOT_OFFERED? It seems like you always record > record some together as well, e.g. UPLOAD_NOT_OFFERED & > UPLOAD_NOT_OFFERED_GET_UPLOAD_DETAILS_FAILED, so the second bit isn't exactly > adding the UPLOAD_NOT_OFFERED information itself. > > Maybe you could simplify this down to a list of conditions, e.g: > > OFFER_DECIDED, (Not sure if you need?) > OFFERED, > NO_CVC, > NO_ADDRESS, > NO_NAME, > CONFLICTING_NAMES, > NO_ZIP, > CONFLICTING_ZIP, > GET_UPLOAD_DETAILS_FAILED Done. There are no redundant bits now. Our intent to redundantly log OFFERED and NOT_OFFERED was to make it easy to quickly calculate success & failure rates. But we can move that to the analysis tools / calculations.
LGTM for autofill code with a small nit. Thanks for your perseverance! https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:889: static const int kNumCardUploadDecisionMetrics = 9; Nit: You can set this as UPLOAD_MAX or UPLOAD_DECISION_MAX in the enum. This is what we have done so far in Autofill and PaymentRequest.
https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:889: static const int kNumCardUploadDecisionMetrics = 9; On 2017/05/02 20:31:29, sebsg wrote: > Nit: You can set this as UPLOAD_MAX or UPLOAD_DECISION_MAX in the enum. This is > what we have done so far in Autofill and PaymentRequest. I considered doing that but I needed to know the max # of metrics for the histogram logging. UPLOAD_MAX or UPLOAD_DECISION_MAX will be set to (1 << number_of_metrics) and I would need to decode which bit is set. I used this constant to avoid that. Let me know if there is an easy way to define MAX enum as (1 << number_of_metrics) but still be able to easily (i.e no loops) determine which bit is set.
https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:889: static const int kNumCardUploadDecisionMetrics = 9; On 2017/05/02 20:35:19, csashi wrote: > On 2017/05/02 20:31:29, sebsg wrote: > > Nit: You can set this as UPLOAD_MAX or UPLOAD_DECISION_MAX in the enum. This > is > > what we have done so far in Autofill and PaymentRequest. > > I considered doing that but I needed to know the max # of metrics for the > histogram logging. UPLOAD_MAX or UPLOAD_DECISION_MAX will be set to (1 << > number_of_metrics) and I would need to decode which bit is set. I used this > constant to avoid that. Let me know if there is an easy way to define MAX enum > as (1 << number_of_metrics) but still be able to easily (i.e no loops) determine > which bit is set. I see, that makes sense, thanks for explaining
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.
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.
On 2017/05/02 20:36:36, sebsg wrote: > https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... > File components/autofill/core/browser/autofill_metrics.h (right): > > https://codereview.chromium.org/2849753002/diff/120001/components/autofill/co... > components/autofill/core/browser/autofill_metrics.h:889: static const int > kNumCardUploadDecisionMetrics = 9; > On 2017/05/02 20:35:19, csashi wrote: > > On 2017/05/02 20:31:29, sebsg wrote: > > > Nit: You can set this as UPLOAD_MAX or UPLOAD_DECISION_MAX in the enum. This > > is > > > what we have done so far in Autofill and PaymentRequest. > > > > I considered doing that but I needed to know the max # of metrics for the > > histogram logging. UPLOAD_MAX or UPLOAD_DECISION_MAX will be set to (1 << > > number_of_metrics) and I would need to decode which bit is set. I used this > > constant to avoid that. Let me know if there is an easy way to define MAX enum > > as (1 << number_of_metrics) but still be able to easily (i.e no loops) > determine > > which bit is set. > > I see, that makes sense, thanks for explaining Hi, FWIW, I found that we have portable wrappers around GCC's __builtin_clz which will let me define max as (1 << number_of_metrics) (see CountLeadingZeroBits32). However, not sure if the gain in elegance is worth the small additional run-time cost. -sashi.
lgtm, with or without flag changes. https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:106: UPLOAD_NOT_OFFERED_NO_ADDRESS = 1 << 2, What about the UPLOAD_NOT_OFFERED part of UPLOAD_NOT_OFFERED_NO_ADDRESS? Is that needed?
https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:106: UPLOAD_NOT_OFFERED_NO_ADDRESS = 1 << 2, On 2017/05/02 23:50:29, Steven Holte wrote: > What about the UPLOAD_NOT_OFFERED part of UPLOAD_NOT_OFFERED_NO_ADDRESS? Is > that needed? I don't follow this comment. We log this bit if the upload was not offered because we could not find an address - i.e. UPLOAD_NOT_OFFERED_NO_ADDRESS => UPLOAD_NOT_OFFERED. I removed the separate UPLOAD_NOT_OFFERED bit. Can you clarify? Thanks.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2849753002/#ps140001 (title: "Formatting fix.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:106: UPLOAD_NOT_OFFERED_NO_ADDRESS = 1 << 2, On 2017/05/02 23:54:43, csashi wrote: > On 2017/05/02 23:50:29, Steven Holte wrote: > > What about the UPLOAD_NOT_OFFERED part of UPLOAD_NOT_OFFERED_NO_ADDRESS? Is > > that needed? > > I don't follow this comment. We log this bit if the upload was not offered > because we could not find an address - i.e. > UPLOAD_NOT_OFFERED_NO_ADDRESS => UPLOAD_NOT_OFFERED. > > I removed the separate UPLOAD_NOT_OFFERED bit. > > Can you clarify? Thanks. For logging this to UKM, it seems like the bit is redundant, since the fact that you are not setting UPLOAD_OFFERED means upload was not offered. It seems like it's nice if each bit's meaning is fairly independent, even if one is the cause for another. For UMA it seems like you are not actually trying to co-occurances, like whether NO_ZIP_CODE is occuring with NO_ADDRESS, with the exception of OFFERED and NO_CVC, which is the only "recoverable" error? And it seems like you are planning to compare counts of buckets against another bucket as a baseline, rather than total counts since you recording multiple times for a single opportunity. I guess you also need to update the descriptions of these values here? There are maybe a couple other ways you might encode this: 1. If you don't actually have that many distinct combinations of errors, and it seems like some of them are exclusive/dependent, you could probably get away with using a SPARSE_HISTOGRAM and just sending the same mask to UMA as you send to UMA as your buckets, though you'd have to encode the combinations in histograms.xml 2. If there are more combinations than you care about, and you just care about co-occurence with Upload offered or not for UMA, you could probably record two histograms, with the different errors as values, one for offered one for not. It seems like that might allow you to disentangle some of these bits from a logic, but it will probably work fine as is too.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493772501968400,
"parent_rev": "cce7c0a2dcdbcb203d207baa4a01f24df5a594e5", "commit_rev":
"bc9f3b2b440eac0f15e651a9c3fd38eb40c849bf"}
Message was sent while issue was closed.
Description was changed from ========== Logs all reasons card upload was not offered in UKM and UMA. UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets. Other buckets count the specific reasons. Fixes DeveloperEngagementUkm to also use bit mask instead of separate metrics. BUG=713842 ========== to ========== Logs all reasons card upload was not offered in UKM and UMA. UPLOAD_OFFERED and UPLOAD_NOT_OFFERED are the 2 big buckets. Other buckets count the specific reasons. Fixes DeveloperEngagementUkm to also use bit mask instead of separate metrics. BUG=713842 Review-Url: https://codereview.chromium.org/2849753002 Cr-Commit-Position: refs/heads/master@{#468852} Committed: https://chromium.googlesource.com/chromium/src/+/bc9f3b2b440eac0f15e651a9c3fd... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/bc9f3b2b440eac0f15e651a9c3fd...
Message was sent while issue was closed.
https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2849753002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:106: UPLOAD_NOT_OFFERED_NO_ADDRESS = 1 << 2, On 2017/05/03 01:04:05, Steven Holte wrote: > On 2017/05/02 23:54:43, csashi wrote: > > On 2017/05/02 23:50:29, Steven Holte wrote: > > > What about the UPLOAD_NOT_OFFERED part of UPLOAD_NOT_OFFERED_NO_ADDRESS? Is > > > that needed? > > > > I don't follow this comment. We log this bit if the upload was not offered > > because we could not find an address - i.e. > > UPLOAD_NOT_OFFERED_NO_ADDRESS => UPLOAD_NOT_OFFERED. > > > > I removed the separate UPLOAD_NOT_OFFERED bit. > > > > Can you clarify? Thanks. > > For logging this to UKM, it seems like the bit is redundant, since the fact that > you are not setting UPLOAD_OFFERED means upload was not offered. It seems like > it's nice if each bit's meaning is fairly independent, even if one is the cause > for another. > > For UMA it seems like you are not actually trying to co-occurances, like whether > NO_ZIP_CODE is occuring with NO_ADDRESS, with the exception of OFFERED and > NO_CVC, which is the only "recoverable" error? And it seems like you are > planning to compare counts of buckets against another bucket as a baseline, > rather than total counts since you recording multiple times for a single > opportunity. I guess you also need to update the descriptions of these values > here? > > There are maybe a couple other ways you might encode this: > > 1. If you don't actually have that many distinct combinations of errors, and it > seems like some of them are exclusive/dependent, you could probably get away > with using a SPARSE_HISTOGRAM and just sending the same mask to UMA as you send > to UMA as your buckets, though you'd have to encode the combinations in > histograms.xml > > 2. If there are more combinations than you care about, and you just care about > co-occurence with Upload offered or not for UMA, you could probably record two > histograms, with the different errors as values, one for offered one for not. > > It seems like that might allow you to disentangle some of these bits from a > logic, but it will probably work fine as is too. Unfortunately, not setting UPLOAD_OFFERED does not mean upload was not offered. The reason is we also have UPLOAD_OFFERED_NO_CVC and we hope to imminently add additional such bits like UPLOAD_OFFERED_NAME_NORMALIZED etc. In retrospect, I should have renamed UPLOAD_OFFERED to UPLOAD_OFFERED_WITHOUT_ANY_FIX (or some such equivalent). I can do that in a follow up CL or in this CL if the commit fails anyway for some reason. I agreed that we lose the total count of failures in UMA because we are recording multiple failures for a single opportunity. Jared also pointed this out offline. But we will be able to find the total count of failures in UKM. My primary goal for this change for UMA was to find out if there are big counts in any of the UPLOAD_NOT_OFFERED bits. This will help us prioritize fixing that error. Co-occurrences are useful but we will get that from UKM. Reg. Sparse histograms - Sebastien also suggested this but we don't know if we will have many distinct combinations or not - UKM will tell us that but right now we don't know. |
