Make MetricsService save compressed logs to local state.

Make MetricsService save compressed logs to local state. 

Previously, it would persist logs in uncompressed form. 
This CL changes the code to compress logs before they're 
saved. A new pair of prefs is introduced for storing 
these logs, while reading from the old pref is still 
maintained to not lose old logs. 

Additionally, this makes the metrics log discard limit 
(currently 50k) be checked against the compressed size 
rather than the uncompressed size. 

Simplifies the format used to store logs, now simply 
storing the compressed log bytes and corresponding hash 
for each log. The size and checksum fields are removed. 
(Size was redundant while the checksum is not no longer 
necessary now that we store the log hash.) 

Deletes some tests that inspected the actual pref 
values are removed, in favor of tests that check that 
serializing and de-serializing works as expected. 

Finally, also introduces GzipUncompress() that is 
needed for tests that inspect log manager logs. 

BUG=382076
TBR=agl@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276429

[Alexei Svitkine (slow), 2014-06-07 08:54:20]

Oops, somehow sent this out with the wrong title. Fixed now.

[Alexei Svitkine (slow), 2014-06-09 20:42:19]

friendly ping

[Ilya Sherman, 2014-06-09 23:17:00]

Thanks, Alexei!

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils.cc:79: const Bytef *source,
nit: " *" -> "* "

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils.cc:80: uLong sourceLen) {
nit: Please use Chromium style for the variable names, same as in the function
right above this one.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:38: #endif
What was this about, and why is it appropriate to remove it?

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:61: std::string data =
base::RandBytesAsString(kSize);
Please log the random input, so that failures are reproducible.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/net/n...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/net/n...
components/metrics/net/net_metrics_log_uploader.cc:26: bool
NetMetricsLogUploader::UploadLog(const std::string& compressed_log_data,
Please update the header file as well.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:211: DCHECK_EQ(0U, list_value.GetSize() %
2);
nit: IMO swap this line to come before the previous line.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.h:149: // Raw log text, typically a serialized
protobuf.
nit: Please update this comment.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.h:162: void Swap(LogHashPair* input);
nit: Member variables should come below methods.  (I know this isn't newly
introduced by your CL, but might as well clean this up while we're here.)

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:288: // Corrupt checksum of stored
list_value.
It's possible to corrupt the hash, right?  Should we still test for that?

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:41: }
Please make sure to log random inputs on failure, so that failures are
reproducible.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:66: min_log_bytes, 0) {
nit: I'd prefer that we have just a single constructor, and that callers who
want kLogByteLimit pass that in.

[Alexei Svitkine (slow), 2014-06-10 17:01:16]

Thanks, comments addressed. PTAL.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils.cc:79: const Bytef *source,
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: " *" -> "* "

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils.cc:80: uLong sourceLen) {
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: Please use Chromium style for the variable names, same as in the function
> right above this one.

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:38: #endif
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> What was this about, and why is it appropriate to remove it?

It's this warning:

http://msdn.microsoft.com/en-us/library/sz5z1byt.aspx

I think it was some left-over crud that got committed accidentally. My guess is
in a previous version the arrays above were being initialized with decimal
values and were thus triggering the warning. So there was another pragma that
disabled this warning around them and this one was supposed to re-enable it.

If my guess above is correct, then it's not needed anymore. We'll see what the
trybots say.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:61: std::string data =
base::RandBytesAsString(kSize);
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> Please log the random input, so that failures are reproducible.

Changed this test to use deterministic input instead.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/net/n...
File components/metrics/net/net_metrics_log_uploader.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/net/n...
components/metrics/net/net_metrics_log_uploader.cc:26: bool
NetMetricsLogUploader::UploadLog(const std::string& compressed_log_data,
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> Please update the header file as well.

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.cc:211: DCHECK_EQ(0U, list_value.GetSize() %
2);
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: IMO swap this line to come before the previous line.

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.h:149: // Raw log text, typically a serialized
protobuf.
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs.h:162: void Swap(LogHashPair* input);
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: Member variables should come below methods.  (I know this isn't newly
> introduced by your CL, but might as well clean this up while we're here.)

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:288: // Corrupt checksum of stored
list_value.
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> It's possible to corrupt the hash, right?  Should we still test for that?

The new code doesn't check for this. The hash is computed based on the
uncompressed data. If we want to check for it, we'd need to unserialized it and
recompute the hash which would be extra overhead and I'm not sure we want to do
that.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:41: }
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> Please make sure to log random inputs on failure, so that failures are
> reproducible.

Done.

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:66: min_log_bytes, 0) {
On 2014/06/09 23:17:00, Ilya Sherman wrote:
> nit: I'd prefer that we have just a single constructor, and that callers who
> want kLogByteLimit pass that in.

Done.

[Ilya Sherman, 2014-06-10 23:38:39]

LGTM % nits and a question.  Thanks, Alexei!

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:288: // Corrupt checksum of stored
list_value.
On 2014/06/10 17:01:15, Alexei Svitkine wrote:
> On 2014/06/09 23:17:00, Ilya Sherman wrote:
> > It's possible to corrupt the hash, right?  Should we still test for that?
> 
> The new code doesn't check for this. The hash is computed based on the
> uncompressed data. If we want to check for it, we'd need to unserialized it
and
> recompute the hash which would be extra overhead and I'm not sure we want to
do
> that.

So it's just checked on the server?  I guess that's fine.  I assume we'd drop
the log if the server rejected it due to a bad hash?

https://codereview.chromium.org/318203004/diff/40001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:65: data[i] =
static_cast<char>(kSize & 0xFF);
Hmm, did you mean "i" rather than "kSize"?  Otherwise, there's easier ways to
build a string that just has a single character repeated a bunch.

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
components/metrics/persisted_logs.h:158: // Compressed log data, typically a
serialized protobuf that's been gzipped.
nit: What does "typically" mean, in this case?  Are there still other possible
cases?

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:43: LOG(INFO) << "Using random
data " << base64_data_for_logging;
nit: SCOPED_TRACE is better, as it only logs the output on failure.

[Alexei Svitkine (slow), 2014-06-11 00:16:28]

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (left):

https://codereview.chromium.org/318203004/diff/20001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:288: // Corrupt checksum of stored
list_value.
On 2014/06/10 23:38:39, Ilya Sherman wrote:
> On 2014/06/10 17:01:15, Alexei Svitkine wrote:
> > On 2014/06/09 23:17:00, Ilya Sherman wrote:
> > > It's possible to corrupt the hash, right?  Should we still test for that?
> > 
> > The new code doesn't check for this. The hash is computed based on the
> > uncompressed data. If we want to check for it, we'd need to unserialized it
> and
> > recompute the hash which would be extra overhead and I'm not sure we want to
> do
> > that.
> 
> So it's just checked on the server?  I guess that's fine.  I assume we'd drop
> the log if the server rejected it due to a bad hash?

Yes that's the plan. We're not dropping just yet (only recording stats about %
mismatched which is tiny), but I plan to make this change probably next week.

https://codereview.chromium.org/318203004/diff/40001/components/metrics/compr...
File components/metrics/compression_utils_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/compr...
components/metrics/compression_utils_unittest.cc:65: data[i] =
static_cast<char>(kSize & 0xFF);
On 2014/06/10 23:38:39, Ilya Sherman wrote:
> Hmm, did you mean "i" rather than "kSize"?  Otherwise, there's easier ways to
> build a string that just has a single character repeated a bunch.

Indeed, good catch - done!

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
File components/metrics/persisted_logs.h (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
components/metrics/persisted_logs.h:158: // Compressed log data, typically a
serialized protobuf that's been gzipped.
On 2014/06/10 23:38:39, Ilya Sherman wrote:
> nit: What does "typically" mean, in this case?  Are there still other possible
> cases?

Changed to not say typically. (There's no other case.)

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
File components/metrics/persisted_logs_unittest.cc (right):

https://codereview.chromium.org/318203004/diff/40001/components/metrics/persi...
components/metrics/persisted_logs_unittest.cc:43: LOG(INFO) << "Using random
data " << base64_data_for_logging;
On 2014/06/10 23:38:39, Ilya Sherman wrote:
> nit: SCOPED_TRACE is better, as it only logs the output on failure.

Done.

[Alexei Svitkine (slow), 2014-06-11 00:16:33]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-11 00:17:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/318203004/60001

[Alexei Svitkine (slow), 2014-06-11 01:16:06]

+agl for components/metrics/DEPS review for the +third_party/zlib line (which
requires approval from third_party/zlib/OWNERS)

[Alexei Svitkine (slow), 2014-06-11 04:47:46]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-11 04:49:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/318203004/80001

[Alexei Svitkine (slow), 2014-06-11 05:59:32]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-11 06:00:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/318203004/100001

[commit-bot: I haz the power, 2014-06-11 12:02:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-11 12:09:06]

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

[commit-bot: I haz the power, 2014-06-11 12:09:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Alexei Svitkine (slow), 2014-06-11 15:13:12]

The CQ bit was checked by asvitkine@chromium.org

[commit-bot: I haz the power, 2014-06-11 15:17:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asvitkine@chromium.org/318203004/100001

[commit-bot: I haz the power, 2014-06-11 16:31:26]

Change committed as 276429