Add various UMA histograms for measuring GCM crypto performance

Add various UMA histograms for measuring GCM crypto performance

This adds (and tests) UMA in the GCMKeyStore, an on-disk database, and
the decryption routines part of the GCMEncryptionProvider and the
GCMMessageCryptographer.

Adding the metrics comes with an extra free refactoring adjusting the
message-received flow to make it cleaner to measure them.

BUG=486040
Committed: https://crrev.com/266a2aa40f2fb11662afe0af694b4be0cfb3efe4
Cr-Commit-Position: refs/heads/master@{#376501}

[Peter Beverloo, 2016-02-16 21:40:45]

peter@chromium.org changed reviewers:
+ johnme@chromium.org

[Peter Beverloo, 2016-02-16 21:40:45]

+johnme for first review

[johnme, 2016-02-18 17:59:38]

lgtm with nits

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/f...
File components/gcm_driver/fake_gcm_client.cc (right):

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/f...
components/gcm_driver/fake_gcm_client.cc:164:
recorder_.RecordDecryptionFailure(app_id, result);
Why does the fake record stats? Is this for a test?

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:287:
GCMEncryptionProvider::DECRYPTION_RESULT_LAST);
LAST+1

[Peter Beverloo, 2016-02-18 21:22:58]

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

[Peter Beverloo, 2016-02-18 21:22:59]

Thanks John!

+isherman

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/f...
File components/gcm_driver/fake_gcm_client.cc (right):

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/f...
components/gcm_driver/fake_gcm_client.cc:164:
recorder_.RecordDecryptionFailure(app_id, result);
On 2016/02/18 17:59:38, johnme wrote:
> Why does the fake record stats? Is this for a test?

Yes. See GCMDriverFunctionalTest.EncryptedMessageReceivedError.

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1707513002/diff/40001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:287:
GCMEncryptionProvider::DECRYPTION_RESULT_LAST);
On 2016/02/18 17:59:38, johnme wrote:
> LAST+1

Done.

[Ilya Sherman, 2016-02-18 21:54:40]

Metrics LGTM

https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:287:
GCMEncryptionProvider::DECRYPTION_RESULT_LAST + 1);
Optional nit: I generally find it's a little easier to have a _COUNT entry
rather than a _LAST entry, as it lets you drop the "+ 1" here, and it's easier
to keep the _COUNT entry correctly up-to-date.

[johnme, 2016-02-19 10:24:13]

https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:287:
GCMEncryptionProvider::DECRYPTION_RESULT_LAST + 1);
On 2016/02/18 21:54:40, Ilya Sherman wrote:
> Optional nit: I generally find it's a little easier to have a _COUNT entry
> rather than a _LAST entry, as it lets you drop the "+ 1" here, and it's easier
> to keep the _COUNT entry correctly up-to-date.

Do you mean as a separate integer, outside the enum (named
DECRYPTION_RESULT_COUNT to match)? Having it in the enum would make it slightly
awkward to use the enum in switch statements.

[Peter Beverloo, 2016-02-19 10:28:43]

On 2016/02/19 10:24:13, johnme wrote:
>
https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
> File components/gcm_driver/gcm_driver.cc (right):
> 
>
https://codereview.chromium.org/1707513002/diff/60001/components/gcm_driver/g...
> components/gcm_driver/gcm_driver.cc:287:
> GCMEncryptionProvider::DECRYPTION_RESULT_LAST + 1);
> On 2016/02/18 21:54:40, Ilya Sherman wrote:
> > Optional nit: I generally find it's a little easier to have a _COUNT entry
> > rather than a _LAST entry, as it lets you drop the "+ 1" here, and it's
easier
> > to keep the _COUNT entry correctly up-to-date.
> 
> Do you mean as a separate integer, outside the enum (named
> DECRYPTION_RESULT_COUNT to match)? Having it in the enum would make it
slightly
> awkward to use the enum in switch statements.

No, an entry in the enum, i.e. a diff of the following on
gcm_encryption_provider.h.

-DECRYPTION_RESULT_LAST = DECRYPTION_RESULT_INVALID_PAYLOAD
+DECRYPTION_RESULT_COUNT

I actually tried this in response to John's feedback, but also preferred this
over an empty entry in the switch statement.

I'll keep as-is. Thanks, both! :)

[Peter Beverloo, 2016-02-19 10:29:01]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-02-19 10:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/1707513002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-19 10:29:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707513002/60001

[commit-bot: I haz the power, 2016-02-19 10:51:49]

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

[commit-bot: I haz the power, 2016-02-19 10:51:50]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[Peter Beverloo, 2016-02-19 11:18:14]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-02-19 11:18:15]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1707513002/#ps80001
(title: "android fix")

[commit-bot: I haz the power, 2016-02-19 11:19:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707513002/80001

[commit-bot: I haz the power, 2016-02-19 15:16:22]

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

[commit-bot: I haz the power, 2016-02-19 15:16:23]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Beverloo, 2016-02-19 17:22:04]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2016-02-19 17:24:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707513002/80001

[commit-bot: I haz the power, 2016-02-19 18:51:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-19 18:53:13]

Description was changed from

==========
Add various UMA histograms for measuring GCM crypto performance

This adds (and tests) UMA in the GCMKeyStore, an on-disk database, and
the decryption routines part of the GCMEncryptionProvider and the
GCMMessageCryptographer.

Adding the metrics comes with an extra free refactoring adjusting the
message-received flow to make it cleaner to measure them.

BUG=486040
==========

to

==========
Add various UMA histograms for measuring GCM crypto performance

This adds (and tests) UMA in the GCMKeyStore, an on-disk database, and
the decryption routines part of the GCMEncryptionProvider and the
GCMMessageCryptographer.

Adding the metrics comes with an extra free refactoring adjusting the
message-received flow to make it cleaner to measure them.

BUG=486040

Committed: https://crrev.com/266a2aa40f2fb11662afe0af694b4be0cfb3efe4
Cr-Commit-Position: refs/heads/master@{#376501}
==========

[commit-bot: I haz the power, 2016-02-19 18:53:14]

Patchset 5 (id:??) landed as
https://crrev.com/266a2aa40f2fb11662afe0af694b4be0cfb3efe4
Cr-Commit-Position: refs/heads/master@{#376501}