Add metrics for certificate report uploads

Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters, firewalls and
captive portals delay the response indefinitely without sending back any response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a histogram
to understand how often this happens. In particular, a difference between submitted 
reports and the sum of failed and successful reports will indicate that some reports simply
get lost. The histogram also provides insight about how often queued reports are dropped,
which will help us fine tune the queue size and report time-to-live.

A question that came up while discussing this histogram was whether it would be useful
since UMA uploads can also be blocked in a similar way. The answer seems to be yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics uploads whereas
cert reports are not persisted.

BUG=682933
Review-Url: https://codereview.chromium.org/2862453002
Cr-Commit-Position: refs/heads/master@{#469883}
Committed: https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddbe6209362ade5

[meacer, 2017-05-03 21:53:20]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 21:54:02]

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-03 22:46:46]

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

[commit-bot: I haz the power, 2017-05-03 22:46:46]

Dry run: This issue passed the CQ dry run.

[meacer, 2017-05-04 23:39:12]

Description was changed from

==========
Add UMA for report retries

BUG=
==========

to

==========
Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters,
firewalls and
captive portals delay the response indefinitely without sending back any
response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a
histogram
to understand how often this happens. In particular, a difference between
submitted 
reports and the sum of failed and successful reports will indicate that some
reports simply
get lost.

A question that came up while discussing this histogram was whether it would be
useful
since UMA uploads can also be blocked in a similar way. The answer seems to be
yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics
uploads whereas
cert reports are not persisted.

BUG=682933
==========

[meacer, 2017-05-04 23:41:22]

Description was changed from

==========
Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters,
firewalls and
captive portals delay the response indefinitely without sending back any
response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a
histogram
to understand how often this happens. In particular, a difference between
submitted 
reports and the sum of failed and successful reports will indicate that some
reports simply
get lost.

A question that came up while discussing this histogram was whether it would be
useful
since UMA uploads can also be blocked in a similar way. The answer seems to be
yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics
uploads whereas
cert reports are not persisted.

BUG=682933
==========

to

==========
Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters,
firewalls and
captive portals delay the response indefinitely without sending back any
response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a
histogram
to understand how often this happens. In particular, a difference between
submitted 
reports and the sum of failed and successful reports will indicate that some
reports simply
get lost. The histogram also provides insight about how often queued reports are
dropped,
which will help us fine tune the queue size and report time-to-live.

A question that came up while discussing this histogram was whether it would be
useful
since UMA uploads can also be blocked in a similar way. The answer seems to be
yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics
uploads whereas
cert reports are not persisted.

BUG=682933
==========

[meacer, 2017-05-04 23:43:10]

meacer@chromium.org changed reviewers:
+ estark@google.com, jialiul@chromium.org

[meacer, 2017-05-04 23:43:10]

jialiul, estark: PTAL?

We discussed this during enamel sync, but I think there might still be some
value in this histogram.

[meacer, 2017-05-04 23:43:32]

meacer@chromium.org changed reviewers:
+ estark@chromium.org
- estark@google.com

[meacer, 2017-05-04 23:43:32]

-estark@google
+estark@chromium

[Jialiu Lin, 2017-05-05 00:25:43]

LGTM

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:60: enum UMAEvent {
nit: how about "ReportOutcome"?  "UMAEvent" sounds a little bit too general.

https://codereview.chromium.org/2862453002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2862453002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:69399: +<histogram
name="SSL.CertificateErrorReportEvent"
nit: SSL.CertificateErrorReportOutcome?

[estark, 2017-05-05 16:21:14]

lgtm with nits and a question: can you remind me (and/or document somewhere if
it isn't already) how we ensure that the EventHistogramTester is destroyed
*after* all reports are sent?

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:60: enum UMAEvent {
On 2017/05/05 00:25:43, Jialiu Lin wrote:
> nit: how about "ReportOutcome"?  "UMAEvent" sounds a little bit too general.

+1

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:63:
REPORT_SUBMITTED,
Not sure but I think you're supposed to explicitly index (i.e. add " = 0" here)

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:66: // A report
submission was successful.
nit: perhaps define successful (HTTP 200 status code?)

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:174: static const
char kReportEventHistogram[];
nit: also "Public for testing"

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
(right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:449: if
(submitted_) {
Do you need these conditionals? Seems like the bucket counts ought to be 0 if
any of the expected counts is set  to 0.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:259: int
submitted_;
initialize all to 0

[meacer, 2017-05-05 19:24:20]

Thanks, PTAL?

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service.h (right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:60: enum UMAEvent {
On 2017/05/05 00:25:43, Jialiu Lin wrote:
> nit: how about "ReportOutcome"?  "UMAEvent" sounds a little bit too general.

Done.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:63:
REPORT_SUBMITTED,
On 2017/05/05 16:21:13, estark wrote:
> Not sure but I think you're supposed to explicitly index (i.e. add " = 0"
here)

Done.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:66: // A report
submission was successful.
On 2017/05/05 16:21:13, estark wrote:
> nit: perhaps define successful (HTTP 200 status code?)

Done.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service.h:174: static const
char kReportEventHistogram[];
On 2017/05/05 16:21:13, estark wrote:
> nit: also "Public for testing"

Done.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc
(right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.cc:449: if
(submitted_) {
On 2017/05/05 16:21:13, estark wrote:
> Do you need these conditionals? Seems like the bucket counts ought to be 0 if
> any of the expected counts is set  to 0.

Unfortunately yes, base::HistogramTester complains if you look up nonexistent
buckets.

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h
(right):

https://codereview.chromium.org/2862453002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h:259: int
submitted_;
On 2017/05/05 16:21:13, estark wrote:
> initialize all to 0

Done.

https://codereview.chromium.org/2862453002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2862453002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:69399: +<histogram
name="SSL.CertificateErrorReportEvent"
On 2017/05/05 00:25:43, Jialiu Lin wrote:
> nit: SSL.CertificateErrorReportOutcome?

Done, this and the enum.

[meacer, 2017-05-05 19:25:13]

> can you remind me (and/or document somewhere if
it isn't already) how we ensure that the EventHistogramTester is destroyed
*after* all reports are sent?

I added comments to the tests where we declare EventHistogramTester. tl;dr is
it's checked after CleanUpOnIOThread.

[Jialiu Lin, 2017-05-05 20:12:38]

Thanks! LGTM

[meacer, 2017-05-05 21:23:54]

meacer@chromium.org changed reviewers:
+ isherman@chromium.org

[meacer, 2017-05-05 21:23:55]

isherman: Can you PTAL at tools/metrics? Thanks.

[Ilya Sherman, 2017-05-05 22:43:33]

Metrics LGTM, thanks.

[estark, 2017-05-05 22:49:38]

lgtm

[meacer, 2017-05-05 22:53:47]

The CQ bit was checked by meacer@chromium.org

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

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-06 00:00:28]

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

[commit-bot: I haz the power, 2017-05-06 00:00:30]

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_...)

[meacer, 2017-05-06 00:01:47]

The CQ bit was checked by meacer@chromium.org

[commit-bot: I haz the power, 2017-05-06 00:03:23]

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-06 19:21:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494028907942300,
"parent_rev": "32b0510d7d81ee6bb988d9283336320306debbdf", "commit_rev":
"19c49d0f04233c3a406d8171eddbe6209362ade5"}

[commit-bot: I haz the power, 2017-05-06 19:21:58]

Description was changed from

==========
Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters,
firewalls and
captive portals delay the response indefinitely without sending back any
response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a
histogram
to understand how often this happens. In particular, a difference between
submitted 
reports and the sum of failed and successful reports will indicate that some
reports simply
get lost. The histogram also provides insight about how often queued reports are
dropped,
which will help us fine tune the queue size and report time-to-live.

A question that came up while discussing this histogram was whether it would be
useful
since UMA uploads can also be blocked in a similar way. The answer seems to be
yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics
uploads whereas
cert reports are not persisted.

BUG=682933
==========

to

==========
Add metrics for certificate report uploads

When sending certificate reports, we suspect that some content filters,
firewalls and
captive portals delay the response indefinitely without sending back any
response.
Certificate uploads rely on a response to be received in order to decide whether
to retry uploading the report, so this breaks those assumptions. This CL adds a
histogram
to understand how often this happens. In particular, a difference between
submitted 
reports and the sum of failed and successful reports will indicate that some
reports simply
get lost. The histogram also provides insight about how often queued reports are
dropped,
which will help us fine tune the queue size and report time-to-live.

A question that came up while discussing this histogram was whether it would be
useful
since UMA uploads can also be blocked in a similar way. The answer seems to be
yes, as UMA
retries metrics uploads with a different frequency, and it persists metrics
uploads whereas
cert reports are not persisted.

BUG=682933

Review-Url: https://codereview.chromium.org/2862453002
Cr-Commit-Position: refs/heads/master@{#469883}
Committed:
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...
==========

[commit-bot: I haz the power, 2017-05-06 19:22:00]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...

[Avi (use Gerrit), 2017-05-06 22:37:29]

On 2017/05/06 19:22:00, commit-bot: I haz the power wrote:
> Committed patchset #4 (id:60001) as
>
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...

This breaks on multiple bots; reverting in
https://codereview.chromium.org/2861323002

[Avi (use Gerrit), 2017-05-06 23:16:12]

On 2017/05/06 22:37:29, Avi (ping after 24h) wrote:
> On 2017/05/06 19:22:00, commit-bot: I haz the power wrote:
> > Committed patchset #4 (id:60001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...
> 
> This breaks on multiple bots; reverting in
> https://codereview.chromium.org/2861323002

Fails on
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...
https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu...
https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/bu...

[Avi (use Gerrit), 2017-05-06 23:24:42]

On 2017/05/06 23:16:12, Avi (ping after 24h) wrote:
> On 2017/05/06 22:37:29, Avi (ping after 24h) wrote:
> > On 2017/05/06 19:22:00, commit-bot: I haz the power wrote:
> > > Committed patchset #4 (id:60001) as
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...
> > 
> > This breaks on multiple bots; reverting in
> > https://codereview.chromium.org/2861323002
> 
> Fails on
>
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...
>
https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu...
>
https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/bu...

FYI it seems flaky; it's not a consistent failure, just all over the place.

[Avi (use Gerrit), 2017-05-07 17:00:47]

On 2017/05/06 23:24:42, Avi (ping after 24h) wrote:
> On 2017/05/06 23:16:12, Avi (ping after 24h) wrote:
> > On 2017/05/06 22:37:29, Avi (ping after 24h) wrote:
> > > On 2017/05/06 19:22:00, commit-bot: I haz the power wrote:
> > > > Committed patchset #4 (id:60001) as
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/19c49d0f04233c3a406d8171eddb...
> > > 
> > > This breaks on multiple bots; reverting in
> > > https://codereview.chromium.org/2861323002
> > 
> > Fails on
> >
>
https://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%2...
> >
>
https://build.chromium.org/p/chromium.win/builders/Win7%20%2832%29%20Tests/bu...
> >
>
https://build.chromium.org/p/chromium.memory/builders/Linux%20MSan%20Tests/bu...
> 
> FYI it seems flaky; it's not a consistent failure, just all over the place.

Reverted.