MetricsService: Send a hash of the UMA log in a header.

MetricsService: Send a hash of the UMA log in a header. 

Add a new X-Chrome-UMA-Log-SHA1 header that is transmitted 
along with each UMA log upload. The header contains the SHA1 
(as hex) of the payload prior to compression. 

The hash is computed at the time that the log is closed, rather than 
just before being transmitted - to minimize the chance of hashing 
an already corrupted log. 

Does not store the hash with logs when they are persisted temporarily 
to local state for later upload, since log are already hashed when stored 
(and discarded on load if the hash does not match). The SHA1 hash is 
re-computed when logs are loaded from local state. 

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

[Alexei Svitkine (slow), 2013-10-16 15:45:14]

Hey Ilya, please take a look. I'll send out CL for the server-side later today.

[Ilya Sherman, 2013-10-16 18:26:14]

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log_serializer.cc:139:
std::vector<MetricsLogManager::SerializedLog>::const_iterator it;
nit: Please keep this scoped within the for loop.  I know the code looks uglier
that way, but IMO it's more important not to accidentally leak the scope,
especially since "it" is such a commonly reused variable name.

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1468:
log_manager_.staged_log_hash().size());
nit: Why does the hash need to be hex-encoded?  If it needs to be hex-encoded,
why do that here, rather than when first setting the hash?  Is this to satisfy
constraints on what sorts of values can be passed along via a header?

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1468:
log_manager_.staged_log_hash().size());
Please consider adding a DCHECK to verify that the hash is non-empty.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.cc:10: #include "base/sha1.h"
It's probably not a big deal, but I wonder if something like CRC would be more
appropriate than SHA-1.  SHA-1 seems like a somewhat strange choice in general,
since it's a cryptographic hash function, but not really considered to be
cryptographically secure anymore.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.cc:34: }
It would be nice if the struct guaranteed that the hash were always up to date,
i.e. if the struct were a class such that the only way to modify log_text were
to call a setter that updates the hash as well.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:35: std::string log_hash;
nit: Member variables should be ordered after methods.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:38: bool IsEmpty() const;
nit: I'd either omit this method, and make this a more transparent struct by
having client code use log_text.empty(); or upgrade this struct to a proper
class, with more carefully established getters and setters.

[Alexei Svitkine (slow), 2013-10-16 19:31:46]

Comments addressed. I've also updated the tests.

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_log_serializer.cc:139:
std::vector<MetricsLogManager::SerializedLog>::const_iterator it;
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> nit: Please keep this scoped within the for loop.  I know the code looks
uglier
> that way, but IMO it's more important not to accidentally leak the scope,
> especially since "it" is such a commonly reused variable name.

Done.

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
File chrome/browser/metrics/metrics_service.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1468:
log_manager_.staged_log_hash().size());
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> Please consider adding a DCHECK to verify that the hash is non-empty.

Done.

https://codereview.chromium.org/26646003/diff/27001/chrome/browser/metrics/me...
chrome/browser/metrics/metrics_service.cc:1468:
log_manager_.staged_log_hash().size());
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> nit: Why does the hash need to be hex-encoded?  If it needs to be hex-encoded,
> why do that here, rather than when first setting the hash?  Is this to satisfy
> constraints on what sorts of values can be passed along via a header?

The SHA1 hash returned by the API we're using the the 20 byte raw hash
(non-text).

Whereas HTTP headers are expected to be in human readable text - so conversion
to hex is needed (40 bytes - this is how sha1 hashes are normally shown when
human readable).

I prefer the log to keep the raw sha1 bytes and convert to hex where needed
(e.g. here) - though willing to budge if you have a strong opinion on this.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.cc (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.cc:34: }
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> It would be nice if the struct guaranteed that the hash were always up to
date,
> i.e. if the struct were a class such that the only way to modify log_text were
> to call a setter that updates the hash as well.

Done.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:35: std::string log_hash;
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> nit: Member variables should be ordered after methods.

Done.

https://codereview.chromium.org/26646003/diff/27001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:38: bool IsEmpty() const;
On 2013/10/16 18:26:14, Ilya Sherman wrote:
> nit: I'd either omit this method, and make this a more transparent struct by
> having client code use log_text.empty(); or upgrade this struct to a proper
> class, with more carefully established getters and setters.

Upgraded to a proper class.

[Ilya Sherman, 2013-10-17 01:29:03]

Thanks.  Could you also add test coverage to ensure that the hash is being set
as expected?

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:42: void SetLogText(const
std::string& log_text);
Can we pass |log_text| as a pointer, and swap() the contents?  The code
previously did this for "efficiency"; it might be that the performance
difference actually matters, since the strings can indeed be pretty large.

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:56: };
nit: Please either add a DISALLOW_COPY_AND_ASSIGN, or add an explicit comment
indicating that it's intentionally omitted.

[Alexei Svitkine (slow), 2013-10-17 19:47:26]

Added a test for SerializedLog.

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:42: void SetLogText(const
std::string& log_text);
On 2013/10/17 01:29:03, Ilya Sherman wrote:
> Can we pass |log_text| as a pointer, and swap() the contents?  The code
> previously did this for "efficiency"; it might be that the performance
> difference actually matters, since the strings can indeed be pretty large.

I added a SwapLogText() function that does that now and changed the production
code to use it. I've kept SetLogText() since a lot of the test code sets the
text via a string literal (which doesn't work well when the function takes a
pointer).

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:56: };
On 2013/10/17 01:29:03, Ilya Sherman wrote:
> nit: Please either add a DISALLOW_COPY_AND_ASSIGN, or add an explicit comment
> indicating that it's intentionally omitted.

Done.

[Ilya Sherman, 2013-10-17 22:06:25]

Thanks!  LGTM with a final two suggestions:

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:42: void SetLogText(const
std::string& log_text);
On 2013/10/17 19:47:27, Alexei Svitkine wrote:
> On 2013/10/17 01:29:03, Ilya Sherman wrote:
> > Can we pass |log_text| as a pointer, and swap() the contents?  The code
> > previously did this for "efficiency"; it might be that the performance
> > difference actually matters, since the strings can indeed be pretty large.
> 
> I added a SwapLogText() function that does that now and changed the production
> code to use it. I've kept SetLogText() since a lot of the test code sets the
> text via a string literal (which doesn't work well when the function takes a
> pointer).

Thanks.  If SetLogText() is only used in tests, can you rename it to something
like SetLogTextForTesting()?  Even better might be to define a wrapper function
in the test file that looks something like this:

void SetLogText(const std::string& log_text, SerializedLog* log) {
  std::string log_text_copy = log_text;
  log->SwapLogText(&log_text_copy);
}

https://codereview.chromium.org/26646003/diff/61001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager_unittest.cc (right):

https://codereview.chromium.org/26646003/diff/61001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager_unittest.cc:17: typedef testing::Test
MetricsLogManagerTest;
nit: You can just remove this, and change all the TEST_F macros to TEST macros
instead.

[Alexei Svitkine (slow), 2013-10-18 18:51:06]

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager.h (right):

https://codereview.chromium.org/26646003/diff/52001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager.h:42: void SetLogText(const
std::string& log_text);
On 2013/10/17 22:06:25, Ilya Sherman wrote:
> On 2013/10/17 19:47:27, Alexei Svitkine wrote:
> > On 2013/10/17 01:29:03, Ilya Sherman wrote:
> > > Can we pass |log_text| as a pointer, and swap() the contents?  The code
> > > previously did this for "efficiency"; it might be that the performance
> > > difference actually matters, since the strings can indeed be pretty large.
> > 
> > I added a SwapLogText() function that does that now and changed the
production
> > code to use it. I've kept SetLogText() since a lot of the test code sets the
> > text via a string literal (which doesn't work well when the function takes a
> > pointer).
> 
> Thanks.  If SetLogText() is only used in tests, can you rename it to something
> like SetLogTextForTesting()?  Even better might be to define a wrapper
function
> in the test file that looks something like this:
> 
> void SetLogText(const std::string& log_text, SerializedLog* log) {
>   std::string log_text_copy = log_text;
>   log->SwapLogText(&log_text_copy);
> }

Done.

https://codereview.chromium.org/26646003/diff/61001/chrome/common/metrics/met...
File chrome/common/metrics/metrics_log_manager_unittest.cc (right):

https://codereview.chromium.org/26646003/diff/61001/chrome/common/metrics/met...
chrome/common/metrics/metrics_log_manager_unittest.cc:17: typedef testing::Test
MetricsLogManagerTest;
On 2013/10/17 22:06:25, Ilya Sherman wrote:
> nit: You can just remove this, and change all the TEST_F macros to TEST macros
> instead.

Done.

[commit-bot: I haz the power, 2013-10-18 19:00:24]

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

[commit-bot: I haz the power, 2013-10-18 21:13:30]

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

[commit-bot: I haz the power, 2013-10-19 04:37:21]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-19 15:25:34]

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

[commit-bot: I haz the power, 2013-10-19 17:53:08]

Change committed as 229594