Upload UMA data using protocol buffers.

Upload UMA data using protocol buffers.

For now, we will also preserve the existing XML upload system, so that there is no risk of data loss.

BUG=109817
TEST=unit tested


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123901

[Ilya Sherman, 2012-01-28 08:17:20]

This also includes all of the changes in [
https://chromiumcodereview.appspot.com/9232070/ ].

Two things still missing from this CL, which I'll address in a follow-up patch
set: I haven't yet written unit tests for this CL.  Also, the serialization code
still has a few TODOs around error-handling.  Feel free to take a look at what's
here, or wait until I've got both of those things in as well.

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_log_serializer.cc:131: // TODO(isherman): Error
reporting...
(Will address this TODO and the others in this file in a follow-up Patch Set.)

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/browser/metrics/...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:653: // servers?
Jim, Ian, do you know how this works and/or whether it's ever used to change the
upload location?  Do we need to add a new URL to the browser distribution info
for protobuf uploads?

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/browser/metrics/...
chrome/browser/metrics/metrics_service.cc:1128: // arrives.
Reviewers: Does this approach (i.e. waiting for both responses, but only
considering the XML response for error handling) sound o.k. while we have both
the XML and protobuf versions of the pipeline running in parallel?

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
File chrome/common/metrics/metrics_log_base.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_base.cc:184: // TODO(isherman): Why is this
done only on ChromeOS?
Anyone know?

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
File chrome/common/metrics/metrics_log_manager.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:77: // be longer, or at least on
the same order of magnitude in length.
Reviewers, does this seem fine?

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
File chrome/common/metrics/metrics_log_manager.h (right):

https://chromiumcodereview.appspot.com/9232071/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:159:
std::vector<std::pair<std::string, std::string> > unsent_ongoing_logs_;
Storing these as a vector of pairs of strings is a little yucky -- anyone have
any bright alternative suggestions?  If not, it should be ok, since we're
planning to deprecate the XML pipeline relatively soon...

[Ilya Sherman, 2012-02-14 01:22:38]

The associated server-side CL has finally cleared the review hurdle and landed. 
PTAL :)

[Ilya Sherman, 2012-02-17 06:07:44]

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:1057: return true;
TODO(isherman): This is handy for testing for now... should definitely be
removed prior to committing.

[ian fette, 2012-02-23 00:23:16]

lgtm

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
https://clients4.google.com/uma/v2

[Ilya Sherman, 2012-02-23 00:51:21]

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
On 2012/02/23 00:23:16, ian fette wrote:
> https://clients4.google.com/uma/v2

Done.

[jar (doing other things), 2012-02-23 01:59:18]

A few questions and comments.

One larger element is that I found the use of the pair<string, string> made the
code less readable, and I was also pretty concerned about the cost of copying
around those strings.  The logs get pretty big... and I think you should be more
cautious about copying them when it is possible to avoid doing so.

One notable item has to do with log-upload retries.  With two copies of the
data, I think you need separate lists, in case one is rejected, an the other
isn't.  This is probably a reason why the vector of pairs is also problematic. 
Better would be a couple of vectors (see comments below).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/chrome_brows...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/chrome_brows...
chrome/browser/chrome_browser_main.cc:1057: return true;
I had proposed putting in this exact code in a:
#if !defined(NDEBUG)

This would facilitate testing and developing... and allow us to drop in
interesting DCHECKs to prevent regressions.

WDYT?


On 2012/02/17 06:07:44, Ilya Sherman wrote:
> TODO(isherman): This is handy for testing for now... should definitely be
> removed prior to committing.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:35: #include
"content/public/browser/content_browser_client.h"
nit: alphabetize

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:350: // fine.
You could add a TODO to look at profile runs for this task.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:381: // TODO(isherman): Stop writing these
attributes specially once the migration to
Isn't this a generic TODO, which involves (eventually) not writing any of the
XML variants??

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:570: // TODO(isherman): This does the
wrong thing on a 64-bit OS, doesn't it?
This question should be redirected to cpu@.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:611:
gpu_performance->set_overall_score(gpu_performance_stats.overall);
Are these a whole pile of stats that the GPU folks asked to add, or did I miss
the analogous XML?

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.h (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.h:70: // synchronously from the UI thread.
I'm a bit wary if we can't get this list, then then I'd wonder about the plugin
info ongoing accessibility.... but maybe this is a full blown copy.

Is this a copy, made to avoid thread and lifetime issues?

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:22: const size_t
kMaxInitialLogsPersisted = 20;
We should really have a style guide opinion for using anonymous namespaces, vs
static, when declaring variables.

IMO, the static keyword is VERY helpful in locating static state variables, and
is probably preferable even if we use an anonymous namespace (In contrast,
anonymous namespaces are required for keeping classes from being advertised
globally, and hence very useful in that case).

Do you know of a style guide request to the contrary?

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:74: const
std::vector<std::pair<std::string, std::string> >& logs,
This makes it clearer (reading the code) what the pairs of strings are.  Each
one is a serialized log.  The first is the xml.  The second is the protobuf
style.  

Pairs are bad for this (compared to a simple class with two named slots),
because that mapping needs to be held in your head, and is easy to forget (hard
to read).

This is almost throw-away code (soon(?) to transition to protobuf only), so I'm
guessing that is why you wrote it this way.  I need to read it and see how it
comes out.

Perhaps this would be a lot cleaner if you had two arrays, rather than array of
pairs.  You would then have the risk that the pairs of arrays would get
misaligned... but you should have that anyway since either attempted upload may
independently fail.  Hence, the number of logs that need sending from each list
should probably be different (after servers reject an upload).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
These logs get to be big.... so this copying of strings will be expensive, and
perchance undesirable.

Why don't you just keep these separate, and get to avoid this step?

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.h (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.h:42: const
std::vector<std::pair<std::string, std::string> >& logs,
Please add comment about the meaning of pairs of strings.  I *think* it was
pretty clear that the logs used to be serialized as strings.... but maybe that
wasn't clear... and this is less clear.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.h:57: // |is_xml| should be true
if this is an XML log and false if this is a
I wasn't able to understand the comment as written.  

I'm not clear what it means to change a prefs value into a string (presumably
for appending to the local_list).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer_unittest.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer_unittest.cc:17: class
MetricsLogSerializerTest : public ::testing::Test {
The class definition should still be inside the anonymous namespace, so that it
is not visible outside this object file.

(As mentioned in another comment) I think that static is helpful in adding
clarity (without having to scan farther in the file).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_unittest.cc:56: class MetricsLogTest : public
testing::Test {
class definition should remain in the anonymous namespace (unless we are trying
to friend it or such).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
I think the URLs are put into resources, and not directly embedded in the code. 
That makes it harder for a developer build to accidentally send data to our
servers.

On 2012/02/23 00:23:16, ian fette wrote:
> https://clients4.google.com/uma/v2

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:657: // servers?
no... but the URLs are kept in resources, specific to the vendor that builds and
distributes the binary.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:962: if (current_fetch_xml_.get() ||
current_fetch_proto_.get())
I think you're going to have to handle the sending of the two logs separately. 
It may get a bit messy :-/.  You have to be careful to not resend to one server,
just because the other server rejected a log upload.

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1132: // arrives.
What do you do if one provides an ack, and the other fails?  At a minimum
provide commentary here (since you're talking about this case).

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1176: UMA_HISTOGRAM_COUNTS(
nit: indent for body of if should only be 2 characters (even if the predicate in
parens had different indentation).

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
File chrome/common/metrics/metrics_log_base.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:36: // Any id less than 16 bytes is
considered to be a testing id.
Why not just use some fixed ID?  Probability of collision should be
infinitesimal.   As noted below, it wasn't clear why this was needed.

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:67: DVLOG(1) << "Metrics: Hash numeric
[" << value << "]=[" << HashToUInt64(hash)
It is good this is being preserved! ;-) Thanks!

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:141: if (IsTestingID(client_id_)) {
This seems a little strange.  It is nicer to not have special code paths (in
main code) that are for test only.  When you're running the test, you should
know what the client_id is... and hence be ready to compare with the expected
hash.

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:187: // TODO(isherman): Why is this
done only on ChromeOS?
I have no idea why :-/.

I'd have to read about how the XML module works (it is a pretty funky interface
IMO). Perhaps it is more memory efficient.... or perhaps they are trying to
reduce memory, and maybe they won't save unsent logs etc. on CrOS.

It does look quite strange, and  I can see data in the histograms, so I know
this path is getting exercise.

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:219: return
uma_proto_.SerializeAsString();
It might be nice to pass in a "std::string*" and have that fleshed out.  I'm a
little wary about the costs of passing around (large) strings in this way....
but maybe it is all super efficient with temps.... but I would expect not :-/.

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:258:
user_action->set_name_hash(numeric_hash);
Have you consistently decided to use a full 64bit hash in our reporting?

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_base.cc:259:
user_action->set_time(Time::Now().ToTimeT());
Note that Time::Now() is not monotonic, and will vary when either the clock is
push forward/back for daylight savings, or when the user adjusts the clock.  Is
it really valuable to have (untrustworthy) time-of-day, or would TimeTicks be a
more valuable basis?

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
File chrome/common/metrics/metrics_log_manager.h (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_manager.h:42: const std::string&
staged_log_text_xml() const {
This sort of accessor helps some (and keeps us away from having to remember what
element of the pair is xml vs proto), but you should try to use it consistently.

I recall seeing .first and .second in another file.

http://codereview.chromium.org/9232071/diff/18001/chrome/common/metrics/metri...
chrome/common/metrics/metrics_log_manager.h:158:
std::vector<std::pair<std::string, std::string> > unsent_initial_logs_;
IMO, you should use a tiny class or struct, and not a pair.

[ian fette, 2012-02-23 02:08:42]

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:611:
gpu_performance->set_overall_score(gpu_performance_stats.overall);
On 2012/02/23 01:59:18, jar wrote:
> Are these a whole pile of stats that the GPU folks asked to add, or did I miss
> the analogous XML?

afaik the "scores" are the new part, as well as the driver version/date...

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:22: const size_t
kMaxInitialLogsPersisted = 20;
On 2012/02/23 01:59:18, jar wrote:
> We should really have a style guide opinion for using anonymous namespaces, vs
> static, when declaring variables.
> 
> IMO, the static keyword is VERY helpful in locating static state variables,
and
> is probably preferable even if we use an anonymous namespace (In contrast,
> anonymous namespaces are required for keeping classes from being advertised
> globally, and hence very useful in that case).
> 
> Do you know of a style guide request to the contrary?


"Unnamed namespaces are allowed and even encouraged in .cc files, to avoid
runtime naming conflicts:" according to the google style guide...

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
On 2012/02/23 01:59:18, jar wrote:
> These logs get to be big.... so this copying of strings will be expensive, and
> perchance undesirable.
> 
> Why don't you just keep these separate, and get to avoid this step?

std::strings are copy on write. Creating this sort of copy is cheap IIRC...

[ian fette, 2012-02-23 03:02:53]

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
On 2012/02/23 02:08:43, ian fette wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > These logs get to be big.... so this copying of strings will be expensive,
and
> > perchance undesirable.
> > 
> > Why don't you just keep these separate, and get to avoid this step?
> 
> std::strings are copy on write. Creating this sort of copy is cheap IIRC...

actually, rsleevi reminds me that after GCC 4.6, std::string is no longer COW...
so yeah, this could be bad...

[jar (doing other things), 2012-02-23 03:03:06]

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:22: const size_t
kMaxInitialLogsPersisted = 20;
You missed my point (or perhaps I stated it poorly):

I'd like to see more explicit use of static linkage.

I don't *mind* using an anonymous namespace, and in fact, it is necessary for
classes (for which "static" linkage cannot be specified!).



On 2012/02/23 02:08:43, ian fette wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > We should really have a style guide opinion for using anonymous namespaces,
vs
> > static, when declaring variables.
> > 
> > IMO, the static keyword is VERY helpful in locating static state variables,
> and
> > is probably preferable even if we use an anonymous namespace (In contrast,
> > anonymous namespaces are required for keeping classes from being advertised
> > globally, and hence very useful in that case).
> > 
> > Do you know of a style guide request to the contrary?
> 
> 
> "Unnamed namespaces are allowed and even encouraged in .cc files, to avoid
> runtime naming conflicts:" according to the google style guide...

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
As noted by Sleevi, this copy-on-write semantic (rendering strings thread-unsafe
once copied) was peculiar to an older version of GCC, and never supported by
other vendors, such as MS and MSVC.

These strings are very large... and we need to handle them efficiently.

On 2012/02/23 02:08:43, ian fette wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > These logs get to be big.... so this copying of strings will be expensive,
and
> > perchance undesirable.
> > 
> > Why don't you just keep these separate, and get to avoid this step?
> 
> std::strings are copy on write. Creating this sort of copy is cheap IIRC...

[Ryan Sleevi, 2012-02-23 03:28:06]

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
On 2012/02/23 03:03:06, jar wrote:
> As noted by Sleevi, this copy-on-write semantic (rendering strings
thread-unsafe
> once copied) was peculiar to an older version of GCC, and never supported by
> other vendors, such as MS and MSVC.
> 
> These strings are very large... and we need to handle them efficiently.
> 
> On 2012/02/23 02:08:43, ian fette wrote:
> > On 2012/02/23 01:59:18, jar wrote:
> > > These logs get to be big.... so this copying of strings will be expensive,
> and
> > > perchance undesirable.
> > > 
> > > Why don't you just keep these separate, and get to avoid this step?
> > 
> > std::strings are copy on write. Creating this sort of copy is cheap IIRC...
> 

The relevant item from C++11 that I mentioned was
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2008/n2668.htm

libc++ implements strings as CoW, as ian notes, but has moved away from it, and
C++11 explicitly forbids it. STLport (Android) and Dinkumware (MSVC) never
supported CoW. Clang's libc++ (to avoid the LGPL of libstdc++ from GCC) has one
of it's main bullet points that it uses the short string optimization rather
than CoW.

The Sutter reference I mentioned is
http://www.gotw.ca/publications/optimizations.htm

[cpu_(ooo_6.6-7.5), 2012-02-23 18:55:19]

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/9232071/diff/18001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_log.cc:570: // TODO(isherman): This does the
wrong thing on a 64-bit OS, doesn't it?
On 2012/02/23 01:59:18, jar wrote:
> This question should be redirected to cpu@.

Yes, int is 32 bit. so you are looping of some bits. We don't have a 64 bits
build of chrome so it is not urgent.

[Ilya Sherman, 2012-02-24 02:10:06]

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:1057: return true;
On 2012/02/23 01:59:18, jar wrote:
> I had proposed putting in this exact code in a:
> #if !defined(NDEBUG)
> 
> This would facilitate testing and developing... and allow us to drop in
> interesting DCHECKs to prevent regressions.
> 
> WDYT?

Sounds fine to me, but probably best to do in a separate CL.  I think MAD is
planning to make this change sometime this week, when he gets around to it...

> 
> 
> On 2012/02/17 06:07:44, Ilya Sherman wrote:
> > TODO(isherman): This is handy for testing for now... should definitely be
> > removed prior to committing.
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:35: #include
"content/public/browser/content_browser_client.h"
On 2012/02/23 01:59:18, jar wrote:
> nit: alphabetize

Done.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:350: // fine.
On 2012/02/23 01:59:18, jar wrote:
> You could add a TODO to look at profile runs for this task. 

Done.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:381: // TODO(isherman): Stop writing these
attributes specially once the migration to
On 2012/02/23 01:59:18, jar wrote:
> Isn't this a generic TODO, which involves (eventually) not writing any of the
> XML variants??

This TODO is to remind me that there are no longer going to be any distinguished
"required" stability attributes, as all of 'em are marked "optional" in the new
protobufs.  So, once we rip out the XML logging code, this whole function can be
refactored away.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:570: // TODO(isherman): This does the
wrong thing on a 64-bit OS, doesn't it?
On 2012/02/23 01:59:18, jar wrote:
> This question should be redirected to cpu@.

Sent Carlos an email asking him to take a look.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:570: // TODO(isherman): This does the
wrong thing on a 64-bit OS, doesn't it?
On 2012/02/23 18:55:19, cpu wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > This question should be redirected to cpu@.
> 
> Yes, int is 32 bit. so you are looping of some bits. We don't have a 64 bits
> build of chrome so it is not urgent.

Ok.  Might as well fix it as long as I'm here though -- thanks for confirming :)

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:611:
gpu_performance->set_overall_score(gpu_performance_stats.overall);
On 2012/02/23 02:08:43, ian fette wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > Are these a whole pile of stats that the GPU folks asked to add, or did I
miss
> > the analogous XML?
> 
> afaik the "scores" are the new part, as well as the driver version/date...

Yeah, everything other than vendor_id and device_id here is new, per request
from Vangelis.  (Some of them we had slots for in the previous version of the
protobuf, but we were never logging them.)

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log.h (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.h:70: // synchronously from the UI thread.
On 2012/02/23 01:59:18, jar wrote:
> I'm a bit wary if we can't get this list, then then I'd wonder about the
plugin
> info ongoing accessibility.... but maybe this is a full blown copy.
> 
> Is this a copy, made to avoid thread and lifetime issues?

Yes, this is a copy (unless I'm reading the MetricsService code wrong -- this is
MetricsService::plugins_).

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:22: const size_t
kMaxInitialLogsPersisted = 20;
On 2012/02/23 03:03:06, jar wrote:
> You missed my point (or perhaps I stated it poorly):
> 
> I'd like to see more explicit use of static linkage.
> 
> I don't *mind* using an anonymous namespace, and in fact, it is necessary for
> classes (for which "static" linkage cannot be specified!).

I think the relevant style guide rule is this one: [
http://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces ].  That
said, this is mostly a rule that I've picked up implicitly from reviewers of my
code: Since we have to use anonymous namespaces to force static linkage for
classes, we might as well be consistent and use them in all cases where we'd
like to force static linkage.  (This is assuming that I understand correctly,
and that declaring a constant in an anonymous namespace effectively forces
static linkage for it.)

> On 2012/02/23 02:08:43, ian fette wrote:
> > On 2012/02/23 01:59:18, jar wrote:
> > > We should really have a style guide opinion for using anonymous
namespaces,
> vs
> > > static, when declaring variables.
> > > 
> > > IMO, the static keyword is VERY helpful in locating static state
variables,
> > and
> > > is probably preferable even if we use an anonymous namespace (In contrast,
> > > anonymous namespaces are required for keeping classes from being
advertised
> > > globally, and hence very useful in that case).
> > > 
> > > Do you know of a style guide request to the contrary?
> > 
> > 
> > "Unnamed namespaces are allowed and even encouraged in .cc files, to avoid
> > runtime naming conflicts:" according to the google style guide...
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:100: logs_xml[i] =
logs[i].first;
On 2012/02/23 01:59:18, jar wrote:
> These logs get to be big.... so this copying of strings will be expensive, and
> perchance undesirable.
> 
> Why don't you just keep these separate, and get to avoid this step?

No longer copying the strings needlessly.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.h (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.h:57: // |is_xml| should be true
if this is an XML log and false if this is a
On 2012/02/23 01:59:18, jar wrote:
> I wasn't able to understand the comment as written.  
> 
> I'm not clear what it means to change a prefs value into a string (presumably
> for appending to the local_list).

Is it clearer now, or still confusing?

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer_unittest.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer_unittest.cc:17: class
MetricsLogSerializerTest : public ::testing::Test {
On 2012/02/23 01:59:18, jar wrote:
> The class definition should still be inside the anonymous namespace, so that
it
> is not visible outside this object file.
> 
> (As mentioned in another comment) I think that static is helpful in adding
> clarity (without having to scan farther in the file).

Somebody... and unfortunately, I don't remember who, at this point... once told
me that the classes that are used as test fixtures should be defined outside of
anonymous namespaces, due to something about how the bots are configured.  I'm
happy to be convinced that that's false, though -- I'm all for hiding symbols in
anonymous namespaces, generally :)

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
On 2012/02/23 01:59:18, jar wrote:
> I think the URLs are put into resources, and not directly embedded in the
code. 
> That makes it harder for a developer build to accidentally send data to our
> servers.

So, the "kServerUrlXML" constant I pulled up from an inline literal further down
in this file.  I'm not sure I understand your concern -- do we want to try to
hide the URL, or just require some extra special build configuration to be able
to send data to the real server, or something else?

> 
> On 2012/02/23 00:23:16, ian fette wrote:
> > https://clients4.google.com/uma/v2
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:657: // servers?
On 2012/02/23 01:59:18, jar wrote:
> no... but the URLs are kept in resources, specific to the vendor that builds
and
> distributes the binary.

Unless I'm misreading the pre-existing code, we were only using resources on
Windows.  Maybe I'm just being daft, but I don't understand the value of using
resources on one OS if we use the direct URL on other OSes.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:962: if (current_fetch_xml_.get() ||
current_fetch_proto_.get())
On 2012/02/23 01:59:18, jar wrote:
> I think you're going to have to handle the sending of the two logs separately.

> It may get a bit messy :-/.  You have to be careful to not resend to one
server,
> just because the other server rejected a log upload.

I'd really prefer to avoid decoupling the sending, as I think that would require
adding additional states to the MetricsService, and make the XML pipeline that
much harder to strip out once we're happy with the protobuf pipeline.

Right now, I'm basically preserving the flow of the XML pipeline, and not being
very careful with edge cases for the protobuf pipeline, since it's currently
experimental anyway.  Once we think the protobuf pipeline is pretty stable, I'll
swap the two roles; and then eventually remove the XML pipeline code entirely. 
Since we don't expect there to be too many problems along the way, I'd rather
reduce the size of the code changes needed at each step.  WDYT?

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1132: // arrives.
On 2012/02/23 01:59:18, jar wrote:
> What do you do if one provides an ack, and the other fails?  At a minimum
> provide commentary here (since you're talking about this case).

I tried to describe that: "For now, only the XML upload's response affects
decisions like whether to retry the upload, whether to abandon the upload
because it is too large, etc."  Tried to emphasize it a bit more -- please let
me know if the new wording is better.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1176: UMA_HISTOGRAM_COUNTS(
On 2012/02/23 01:59:18, jar wrote:
> nit: indent for body of if should only be 2 characters (even if the predicate
in
> parens had different indentation).

Done.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
File chrome/common/metrics/metrics_log_base.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:36: // Any id less than 16 bytes is
considered to be a testing id.
On 2012/02/23 01:59:18, jar wrote:
> Why not just use some fixed ID?  Probability of collision should be
> infinitesimal.   As noted below, it wasn't clear why this was needed.

No idea -- I was just trying to match the existing code (this change used to be
performed server-side...).  If nobody's aware of any need for this, I'm happy to
get rid of it.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:187: // TODO(isherman): Why is this
done only on ChromeOS?
On 2012/02/23 01:59:18, jar wrote:
> I have no idea why :-/.
> 
> I'd have to read about how the XML module works (it is a pretty funky
interface
> IMO). Perhaps it is more memory efficient.... or perhaps they are trying to
> reduce memory, and maybe they won't save unsent logs etc. on CrOS.
> 
> It does look quite strange, and  I can see data in the histograms, so I know
> this path is getting exercise.

Ok.  Well, the good news is that we're (hopefully) getting rid of all the XML
code soon, at which point this will be moot...

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:219: return
uma_proto_.SerializeAsString();
On 2012/02/23 01:59:18, jar wrote:
> It might be nice to pass in a "std::string*" and have that fleshed out.  I'm a
> little wary about the costs of passing around (large) strings in this way....
> but maybe it is all super efficient with temps.... but I would expect not :-/.

I wrote it this way based on the recommendation in [
http://www.chromium.org/developers/coding-style#Method_Signatures ]; but I'll go
ahead and change it to your suggestion, since these are indeed larger strings
than usual.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:258:
user_action->set_name_hash(numeric_hash);
On 2012/02/23 01:59:18, jar wrote:
> Have you consistently decided to use a full 64bit hash in our reporting?

Yes, that was my understanding.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:259:
user_action->set_time(Time::Now().ToTimeT());
On 2012/02/23 01:59:18, jar wrote:
> Note that Time::Now() is not monotonic, and will vary when either the clock is
> push forward/back for daylight savings, or when the user adjusts the clock. 
Is
> it really valuable to have (untrustworthy) time-of-day, or would TimeTicks be
a
> more valuable basis?

Hmm, that's a really good point.  I think TimeTicks() will be more useful; but I
want to also double-check with Mark Pearson, who is actually thinking about
sessionization of this and omnibox data.  (Updated the omnibox logging to also
use TimeTicks()).

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
File chrome/common/metrics/metrics_log_manager.h (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/common/metri...
chrome/common/metrics/metrics_log_manager.h:158:
std::vector<std::pair<std::string, std::string> > unsent_initial_logs_;
On 2012/02/23 01:59:18, jar wrote:
> IMO, you should use a tiny class or struct, and not a pair.

Done.

[Ilya Sherman, 2012-02-25 01:54:11]

Changed to also never re-upload protobuf logs, so that we avoid duplicated
successful uploads corresponding to failed XML uploads.

[jar (doing other things), 2012-02-27 20:35:33]

A few more questions and comments.

Thanks in advance!

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:1057: return true;
Since it would be a bad thing to check this in by accident, I'd suggest that you
at least (tentatively) enclose it in an if !defined(NDEBUG).  If you checked it
in by accident, it would be no big deal that way.

I think it makes it much easier to test this way (as you noted).


On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > I had proposed putting in this exact code in a:
> > #if !defined(NDEBUG)
> > 
> > This would facilitate testing and developing... and allow us to drop in
> > interesting DCHECKs to prevent regressions.
> > 
> > WDYT?
> 
> Sounds fine to me, but probably best to do in a separate CL.  I think MAD is
> planning to make this change sometime this week, when he gets around to it...
> 
> > 
> > 
> > On 2012/02/17 06:07:44, Ilya Sherman wrote:
> > > TODO(isherman): This is handy for testing for now... should definitely be
> > > removed prior to committing.
> > 
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.h (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.h:57: // |is_xml| should be true
if this is an XML log and false if this is a
Much better. Thanks!

On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > I wasn't able to understand the comment as written.  
> > 
> > I'm not clear what it means to change a prefs value into a string
(presumably
> > for appending to the local_list).
> 
> Is it clearer now, or still confusing?

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
I think the most common build (no proof here :-/ ) is a windows build, and we
were (historically) trying to make it harder to err and upload UMA data from a
home-brew chromium build.  It is interesting that the linux build has the
host/path explicitly... but I could have sworn that Windows puts this in a
resource file (somewhere).

On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > I think the URLs are put into resources, and not directly embedded in the
> code. 
> > That makes it harder for a developer build to accidentally send data to our
> > servers.
> 
> So, the "kServerUrlXML" constant I pulled up from an inline literal further
down
> in this file.  I'm not sure I understand your concern -- do we want to try to
> hide the URL, or just require some extra special build configuration to be
able
> to send data to the real server, or something else?
> 
> > 
> > On 2012/02/23 00:23:16, ian fette wrote:
> > > https://clients4.google.com/uma/v2
> > 
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:657: // servers?
I *think* the issue was that a) on windows we could cleanly do this separation;
b) Windows is the largest platform, and most common source of chromium induced
stat spam.

Unless it is painful, we probably should persist in this separation (and if it
were easy... we should do this for Linux and mac ;-) ).

On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > no... but the URLs are kept in resources, specific to the vendor that builds
> and
> > distributes the binary.
> 
> Unless I'm misreading the pre-existing code, we were only using resources on
> Windows.  Maybe I'm just being daft, but I don't understand the value of using
> resources on one OS if we use the direct URL on other OSes.

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:962: if (current_fetch_xml_.get() ||
current_fetch_proto_.get())
I think you've settled on avoiding dup-uploads of the proto based logs... at the
cost of sometimes dropping them (ignoring protobuf upload failures). I'll be
reading code assuming that is what I should find.

On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > I think you're going to have to handle the sending of the two logs
separately.
> 
> > It may get a bit messy :-/.  You have to be careful to not resend to one
> server,
> > just because the other server rejected a log upload.
> 
> I'd really prefer to avoid decoupling the sending, as I think that would
require
> adding additional states to the MetricsService, and make the XML pipeline that
> much harder to strip out once we're happy with the protobuf pipeline.
> 
> Right now, I'm basically preserving the flow of the XML pipeline, and not
being
> very careful with edge cases for the protobuf pipeline, since it's currently
> experimental anyway.  Once we think the protobuf pipeline is pretty stable,
I'll
> swap the two roles; and then eventually remove the XML pipeline code entirely.

> Since we don't expect there to be too many problems along the way, I'd rather
> reduce the size of the code changes needed at each step.  WDYT?

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1132: // arrives.
Wording changes are great.  Thanks!

On 2012/02/24 02:10:06, Ilya Sherman wrote:
> On 2012/02/23 01:59:18, jar wrote:
> > What do you do if one provides an ack, and the other fails?  At a minimum
> > provide commentary here (since you're talking about this case).
> 
> I tried to describe that: "For now, only the XML upload's response affects
> decisions like whether to retry the upload, whether to abandon the upload
> because it is too large, etc."  Tried to emphasize it a bit more -- please let
> me know if the new wording is better.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:783:
omnibox_event->set_time(base::TimeTicks::Now().ToInternalValue());
ToTimeT returns a count of seconds.  ToInternalValue returns a time in
microseconds.  I think you should stick with something closer to resolutions in
seconds.  

The easy way is to subtrac TimeTicks() and then convert the resulting TimeDelta
ToSeconds().

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:179: return
MakeRecallStatusHistogram(LIST_EMPTY, is_xml);
Looking at the data, this LIST_EMPTY case is the vast bulk of all errors. In
fact, it is roughly equally likely to having a success (50% failure on this
case).

Looking at user counts, it appears that roughly every user encounter this error
case.  This suggests (perchance) that the logic does a read of the prefs twice,
and one time it is empty??

Do you have any idea why we had an empty list so often?  The system "should"
have written a partial ongoing log at shutdown, and it should be (I think) very
rare that we had no list at all.
Comments?

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:196: // can be fairly large.
Although the logs may be large, this only grows the vector, which is typically
small (rarely over 20?  Usually 1 or 2?).

Am I confused, or is this a waste?

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:223: if
(!base::Base64Decode(encoded_log, &decoded_log)) {
I'm curious: Does the JSON encoding of prefs automatically take care of this
Base64 encode/decode?  Are we doing this redundantly (unnecessarily) and
inducing data expansion by a factor of 1.37?  (Yes... this was in original
code... and perchance should not be addressed in this CL... but might be worth a
TODO and a bug if what I'm saying makes sense).

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1000:
DCHECK(current_fetch_proto_.get() || !log_manager_.has_staged_log_proto());
Please add a comment that (currently) the staged log for proto is discarded as
we create the URL request, so that there is no chance of a re-transmission.  ...
and that this will change when we switch over to sending only proto bufs.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1085: GURL(server_url_xml_),
content::URLFetcher::POST, this));
Is this where we need to add the TODO() to avoid sending cookies?

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1103:
log_manager_.DiscardStagedLogProto();
This might be another place to leave a TODO() trail about not doing this when we
switch to sending only protobufs.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
File chrome/common/metrics/metrics_log_base.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:218: *encoded_log =
uma_proto_.SerializeAsString();
Use std::swap to avoid a string copy.  There is a *chance* that the compiler
does this for you, but I'd expect not.

Alternatively, you could pass in the std::string* to SerializeAsString() to do
the placement.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:258:
user_action->set_time(base::TimeTicks::Now().ToInternalValue());
ToInternalValue returns microseconds... better would be seconds (more like what
we had with ToTimeT().)

[Ilya Sherman, 2012-02-28 00:23:10]

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/chro...
chrome/browser/chrome_browser_main.cc:1057: return true;
On 2012/02/27 20:35:34, jar wrote:
> Since it would be a bad thing to check this in by accident, I'd suggest that
you
> at least (tentatively) enclose it in an if !defined(NDEBUG).  If you checked
it
> in by accident, it would be no big deal that way.
> 
> I think it makes it much easier to test this way (as you noted).

D'oh.  Re-removed from this CL, and added the "#if !defined(NDEBUG)" wrapper to
my local diff file that I patch in for testing.

> On 2012/02/24 02:10:06, Ilya Sherman wrote:
> > On 2012/02/23 01:59:18, jar wrote:
> > > I had proposed putting in this exact code in a:
> > > #if !defined(NDEBUG)
> > > 
> > > This would facilitate testing and developing... and allow us to drop in
> > > interesting DCHECKs to prevent regressions.
> > > 
> > > WDYT?
> > 
> > Sounds fine to me, but probably best to do in a separate CL.  I think MAD is
> > planning to make this change sometime this week, when he gets around to
it...
> > 
> > > 
> > > 
> > > On 2012/02/17 06:07:44, Ilya Sherman wrote:
> > > > TODO(isherman): This is handy for testing for now... should definitely
be
> > > > removed prior to committing.
> > > 
> > 
>

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/18001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:220: const char kServerUrlProto[] =
"http://rhennthyl.mtv.corp.google.com:8888/uma";
On 2012/02/27 20:35:34, jar wrote:
> I think the most common build (no proof here :-/ ) is a windows build, and we
> were (historically) trying to make it harder to err and upload UMA data from a
> home-brew chromium build.  It is interesting that the linux build has the
> host/path explicitly... but I could have sworn that Windows puts this in a
> resource file (somewhere).

I'm still not following how embedding the URL in a resource file helps though...
if I make a home-brew chromium build, I'm going to build with that same resource
file, so I'm no less likely to upload UMA data.  Is there something I'm missing?

> On 2012/02/24 02:10:06, Ilya Sherman wrote:
> > On 2012/02/23 01:59:18, jar wrote:
> > > I think the URLs are put into resources, and not directly embedded in the
> > code. 
> > > That makes it harder for a developer build to accidentally send data to
our
> > > servers.
> > 
> > So, the "kServerUrlXML" constant I pulled up from an inline literal further
> down
> > in this file.  I'm not sure I understand your concern -- do we want to try
to
> > hide the URL, or just require some extra special build configuration to be
> able
> > to send data to the real server, or something else?
> > 
> > > 
> > > On 2012/02/23 00:23:16, ian fette wrote:
> > > > https://clients4.google.com/uma/v2
> > > 
> > 
>

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log.cc:783:
omnibox_event->set_time(base::TimeTicks::Now().ToInternalValue());
On 2012/02/27 20:35:34, jar wrote:
> ToTimeT returns a count of seconds.  ToInternalValue returns a time in
> microseconds.  I think you should stick with something closer to resolutions
in
> seconds.  
> 
> The easy way is to subtrac TimeTicks() and then convert the resulting
TimeDelta
> ToSeconds().

Done.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:179: return
MakeRecallStatusHistogram(LIST_EMPTY, is_xml);
On 2012/02/27 20:35:34, jar wrote:
> Looking at the data, this LIST_EMPTY case is the vast bulk of all errors. In
> fact, it is roughly equally likely to having a success (50% failure on this
> case).
> 
> Looking at user counts, it appears that roughly every user encounter this
error
> case.  This suggests (perchance) that the logic does a read of the prefs
twice,
> and one time it is empty??
> 
> Do you have any idea why we had an empty list so often?  The system "should"
> have written a partial ongoing log at shutdown, and it should be (I think)
very
> rare that we had no list at all.
> Comments?

It looks like the MetricsLogManager code reads in two sets of persisted logs:
unsent initial logs and unsent ongoing logs [1].  I'm guessing it's very rare
that there are any unsent initial logs.  Does that sound about right?
[1]
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/metrics/metri...

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:196: // can be fairly large.
On 2012/02/27 20:35:34, jar wrote:
> Although the logs may be large, this only grows the vector, which is typically
> small (rarely over 20?  Usually 1 or 2?).
> 
> Am I confused, or is this a waste?

This is really making sure that we can safely pass in a point to e.g.
(*local_list)[local_index].xml to base::Base64Decode() -- the previous code made
a copy of the string.  Updated the comment to hopefully make this clearer...

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:223: if
(!base::Base64Decode(encoded_log, &decoded_log)) {
On 2012/02/27 20:35:34, jar wrote:
> I'm curious: Does the JSON encoding of prefs automatically take care of this
> Base64 encode/decode?  Are we doing this redundantly (unnecessarily) and
> inducing data expansion by a factor of 1.37?  (Yes... this was in original
> code... and perchance should not be addressed in this CL... but might be worth
a
> TODO and a bug if what I'm saying makes sense).

Looks like the base64 encoding/decoding is necessary, per the comment on line
154.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1000:
DCHECK(current_fetch_proto_.get() || !log_manager_.has_staged_log_proto());
On 2012/02/27 20:35:34, jar wrote:
> Please add a comment that (currently) the staged log for proto is discarded as
> we create the URL request, so that there is no chance of a re-transmission. 
...
> and that this will change when we switch over to sending only proto bufs.

Done.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1085: GURL(server_url_xml_),
content::URLFetcher::POST, this));
On 2012/02/27 20:35:34, jar wrote:
> Is this where we need to add the TODO() to avoid sending cookies?

Yep.  In fact... I'll just go ahead and strip off the cookies in this CL :)

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1103:
log_manager_.DiscardStagedLogProto();
On 2012/02/27 20:35:34, jar wrote:
> This might be another place to leave a TODO() trail about not doing this when
we
> switch to sending only protobufs.

Done.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
File chrome/common/metrics/metrics_log_base.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:218: *encoded_log =
uma_proto_.SerializeAsString();
On 2012/02/27 20:35:34, jar wrote:
> Use std::swap to avoid a string copy.  There is a *chance* that the compiler
> does this for you, but I'd expect not.
> 
> Alternatively, you could pass in the std::string* to SerializeAsString() to do
> the placement.

Done.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/common/metri...
chrome/common/metrics/metrics_log_base.cc:258:
user_action->set_time(base::TimeTicks::Now().ToInternalValue());
On 2012/02/27 20:35:34, jar wrote:
> ToInternalValue returns microseconds... better would be seconds (more like
what
> we had with ToTimeT().)

Done.

[jar (doing other things), 2012-02-28 01:33:21]

Thanks for the responses!

The only pending item was the use of a resource file (in Windows) to spec the
URL to upload UMA data.  I went grepping around, and was able to (to my
surprise) find it in
chrome/src/chrome/installer/util/google_chrome_distribution.cc, which is part of
the chromium source distribution.  As a result, this seems like a non-issue (we
do seem to currently include it).  I can still recall this as being chrome
private historically.. but it is not so now... so...

LGTM

Thanks.

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
File chrome/browser/metrics/metrics_log_serializer.cc (right):

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:179: return
MakeRecallStatusHistogram(LIST_EMPTY, is_xml);
Yup... that explains it perfectly.  Thanks.

On 2012/02/28 00:23:10, Ilya Sherman wrote:
> On 2012/02/27 20:35:34, jar wrote:
> > Looking at the data, this LIST_EMPTY case is the vast bulk of all errors. In
> > fact, it is roughly equally likely to having a success (50% failure on this
> > case).
> > 
> > Looking at user counts, it appears that roughly every user encounter this
> error
> > case.  This suggests (perchance) that the logic does a read of the prefs
> twice,
> > and one time it is empty??
> > 
> > Do you have any idea why we had an empty list so often?  The system "should"
> > have written a partial ongoing log at shutdown, and it should be (I think)
> very
> > rare that we had no list at all.
> > Comments?
> 
> It looks like the MetricsLogManager code reads in two sets of persisted logs:
> unsent initial logs and unsent ongoing logs [1].  I'm guessing it's very rare
> that there are any unsent initial logs.  Does that sound about right?
> [1]
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/common/metrics/metri...

https://chromiumcodereview.appspot.com/9232071/diff/37001/chrome/browser/metr...
chrome/browser/metrics/metrics_log_serializer.cc:223: if
(!base::Base64Decode(encoded_log, &decoded_log)) {
OK. Thanks.

On 2012/02/28 00:23:10, Ilya Sherman wrote:
> On 2012/02/27 20:35:34, jar wrote:
> > I'm curious: Does the JSON encoding of prefs automatically take care of this
> > Base64 encode/decode?  Are we doing this redundantly (unnecessarily) and
> > inducing data expansion by a factor of 1.37?  (Yes... this was in original
> > code... and perchance should not be addressed in this CL... but might be
worth
> a
> > TODO and a bug if what I'm saying makes sense).
> 
> Looks like the base64 encoding/decoding is necessary, per the comment on line
> 154.

[commit-bot: I haz the power, 2012-02-28 01:44:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/9232071/44027

[commit-bot: I haz the power, 2012-02-28 05:31:34]

Change committed as 123901

[cpu_(ooo_6.6-7.5), 2012-02-28 05:45:33]

this seems to have broken the tree:

chrome/common/metrics/metrics_log_base.cc: In member function 'void
MetricsLogBase::CloseLog()':
chrome/common/metrics/metrics_log_base.cc:216:error: 'class
metrics::SystemProfileProto' has no member named 'set_hardware_class'

so it was reverted: r123901

[ian fette, 2012-02-28 05:59:50]

Interesting, not sure why this compiled locally. In the proto that's checked
into the Chrome repository, hardware_class is under the hardware message,
whereas the line below (from your patch) would imply that it's at the system
profile level, not system profile -> hardware -> hardware class.

uma_proto_.mutable_system_profile()->set_hardware_class(hardware_class_);

Really no idea why that compiled / didn't barf all over trybots.

[Ilya Sherman, 2012-02-28 06:44:29]

On 2012/02/28 05:59:50, ian fette wrote:
> Interesting, not sure why this compiled locally. In the proto that's checked
> into the Chrome repository, hardware_class is under the hardware message,
> whereas the line below (from your patch) would imply that it's at the system
> profile level, not system profile -> hardware -> hardware class.
> 
> uma_proto_.mutable_system_profile()->set_hardware_class(hardware_class_);
> 
> Really no idea why that compiled / didn't barf all over trybots.

It's #ifdef'd for ChromeOS only :(
Fixing, recommitting...