Uses fine grained histogram buckets for CVC errors.

Uses fine grained histogram buckets for CVC errors.

This will help us debug why CVC is not detected often.

BUG=713842
Review-Url: https://codereview.chromium.org/2863913003
Cr-Commit-Position: refs/heads/master@{#469692}
Committed: https://chromium.googlesource.com/chromium/src/+/93bddd53f3519c31a055cd8d27e2be2606e6eda0

[csashi, 2017-05-05 04:13:54]

csashi@google.com changed reviewers:
+ jsaul@google.com, sebsg@chromium.org

[csashi, 2017-05-05 04:13:54]

Hi,
Please take a look. Thanks!
-sashi.

[csashi, 2017-05-05 04:15:14]

Description was changed from

==========
Uses fine grained histogram buckets for CVC errors.

BUG=713842
==========

to

==========
Uses fine grained histogram buckets for CVC errors.

This will help us debug why CVC is not detected often.

BUG=713842
==========

[csashi, 2017-05-05 04:15:23]

The CQ bit was checked by csashi@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 04:15:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 05:11:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 05:11:16]

Dry run: This issue passed the CQ dry run.

[sebsg, 2017-05-05 15:15:31]

The code looks good, a comment about the histograms.

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.h:103: CVC_FIELD_NOT_FOUND = 1
<< 1,
Please see the comment I wrote in the histograms page.

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
tools/metrics/histograms/enums.xml:1617: -  <int value="1" label="Upload not
offered, no CVC detected"/>
You should keep the ones that don't change like "no address available" to the
same value. The reason being that when you'll analyze the metrics, you'll filter
by bucket. In the case, between two versions you will change the bucket meaning,
which is bad.

What I suggest is 

1) Either deprecate the old bucket 1 (Put name as deprecated) and put the others
at the bottom.

2) if you feel like "CVC field not detected" is equivalent to "... no CVC
detected" you can change the name of that bucket, but still put the others at
the bottom.

I lean towards solution 1)

[csashi, 2017-05-05 16:29:55]

Hi Sebastien,
Please take a look. Thanks,
-sashi.

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.h:103: CVC_FIELD_NOT_FOUND = 1
<< 1,
On 2017/05/05 15:15:31, sebsg wrote:
> Please see the comment I wrote in the histograms page.

Acknowledged. That comment looks correct to me even with this change.

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
tools/metrics/histograms/enums.xml:1617: -  <int value="1" label="Upload not
offered, no CVC detected"/>
On 2017/05/05 15:15:31, sebsg wrote:
> You should keep the ones that don't change like "no address available" to the
> same value. The reason being that when you'll analyze the metrics, you'll
filter
> by bucket. In the case, between two versions you will change the bucket
meaning,
> which is bad.
> 
> What I suggest is 
> 
> 1) Either deprecate the old bucket 1 (Put name as deprecated) and put the
others
> at the bottom.
> 
> 2) if you feel like "CVC field not detected" is equivalent to "... no CVC
> detected" you can change the name of that bucket, but still put the others at
> the bottom.
> 
> I lean towards solution 1)

Acknowledged. I thought it was safe to change this because we don't yet have a
stable release with this change and the one before that. We only look at stable
metrics. Please let me know if you still need me to keep the same numbers.

[sebsg, 2017-05-05 16:46:31]

LGTM

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_metrics.h (right):

https://codereview.chromium.org/2863913003/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_metrics.h:103: CVC_FIELD_NOT_FOUND = 1
<< 1,
On 2017/05/05 16:29:55, csashi wrote:
> On 2017/05/05 15:15:31, sebsg wrote:
> > Please see the comment I wrote in the histograms page.
> 
> Acknowledged. That comment looks correct to me even with this change.

Oh sorry it was not about the comment, it was about the values :)

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
File tools/metrics/histograms/enums.xml (left):

https://codereview.chromium.org/2863913003/diff/1/tools/metrics/histograms/en...
tools/metrics/histograms/enums.xml:1617: -  <int value="1" label="Upload not
offered, no CVC detected"/>
On 2017/05/05 16:29:55, csashi wrote:
> On 2017/05/05 15:15:31, sebsg wrote:
> > You should keep the ones that don't change like "no address available" to
the
> > same value. The reason being that when you'll analyze the metrics, you'll
> filter
> > by bucket. In the case, between two versions you will change the bucket
> meaning,
> > which is bad.
> > 
> > What I suggest is 
> > 
> > 1) Either deprecate the old bucket 1 (Put name as deprecated) and put the
> others
> > at the bottom.
> > 
> > 2) if you feel like "CVC field not detected" is equivalent to "... no CVC
> > detected" you can change the name of that bucket, but still put the others
at
> > the bottom.
> > 
> > I lean towards solution 1)
> 
> Acknowledged. I thought it was safe to change this because we don't yet have a
> stable release with this change and the one before that. We only look at
stable
> metrics. Please let me know if you still need me to keep the same numbers.

Oh I forgot you changed the whole histogram name from CardUploadDecisionExpanded
to CardUploadDecisionMetric. Normally I would still advocate to keep it
consistent in case you want to analyze the old data, but the old data is pretty
useless since it recorded only the first abort reason. 

So I think it's fine :)

[csashi, 2017-05-05 17:06:07]

The CQ bit was checked by csashi@google.com

[commit-bot: I haz the power, 2017-05-05 17:06:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 17:12:46]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1494003967246100, "parent_rev":
"5742c3808b947b73c4e279c2f753e59e284a1910", "commit_rev":
"93bddd53f3519c31a055cd8d27e2be2606e6eda0"}

[commit-bot: I haz the power, 2017-05-05 17:13:35]

Description was changed from

==========
Uses fine grained histogram buckets for CVC errors.

This will help us debug why CVC is not detected often.

BUG=713842
==========

to

==========
Uses fine grained histogram buckets for CVC errors.

This will help us debug why CVC is not detected often.

BUG=713842

Review-Url: https://codereview.chromium.org/2863913003
Cr-Commit-Position: refs/heads/master@{#469692}
Committed:
https://chromium.googlesource.com/chromium/src/+/93bddd53f3519c31a055cd8d27e2...
==========

[commit-bot: I haz the power, 2017-05-05 17:13:37]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/93bddd53f3519c31a055cd8d27e2...

[Jared Saul, 2017-05-05 18:26:38]

lgtm