[Metrics] Re-try failed protocol buffer uploads as well as failed XML uploads.

[Metrics] Re-try failed protocol buffer uploads as well as failed XML uploads.

BUG=109818
TEST=none


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

[Ilya Sherman, 2012-06-07 04:57:13]

This CL still needs unit tests, but is otherwise fairly complete.

[Ilya Sherman, 2012-06-08 08:03:24]

Now includes unit tests, and I also manually tested the re-uploading logic. 
PTAL.

[stuartmorgan, 2012-06-08 08:26:42]

I'm assuming we think this is rare enough that we don't care, but just to be
sure we are making a conscious decision: if the XML log uploads successfully,
the proto upload is still in flight, and we persist to disk (which, remember,
happens frequently on mobile), then the new logic to keep a minimum number of
bytes will count that log as being essentially free to store (since it only
looks at the XML). Are we okay with that?

A simple way to avoid this would be to change the byte-counting logic to use the
max of the two formats' sizes rather than always using the XML. Given how small
that change is it might be worth doing even if we don't think it's a big deal.

https://chromiumcodereview.appspot.com/10546044/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (left):

https://chromiumcodereview.appspot.com/10546044/diff/2002/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1214: scheduler_->UploadCancelled();
It's vital that UploadCancelled be called if something goes catastrophically
wrong and neither current_fetch_xml_ nor current_fetch_proto_ end up with any
content, otherwise the scheduler will never call back, so it seems like this
path needs to stay, just modified to check for both being empty.

[Ilya Sherman, 2012-06-08 22:20:26]

On 2012/06/08 08:26:42, stuartmorgan wrote:
> I'm assuming we think this is rare enough that we don't care, but just to be
> sure we are making a conscious decision: if the XML log uploads successfully,
> the proto upload is still in flight, and we persist to disk (which, remember,
> happens frequently on mobile), then the new logic to keep a minimum number of
> bytes will count that log as being essentially free to store (since it only
> looks at the XML). Are we okay with that?
> 
> A simple way to avoid this would be to change the byte-counting logic to use
the
> max of the two formats' sizes rather than always using the XML. Given how
small
> that change is it might be worth doing even if we don't think it's a big deal.

That sounds like a good idea -- thanks for the suggestion!  The sum of the log
sizes might be even better, if that limit isn't just an arbitrary large number. 
This is easy to split off to a separate CL, so I'll go ahead and do that :)

https://chromiumcodereview.appspot.com/10546044/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/metrics_service.cc (left):

https://chromiumcodereview.appspot.com/10546044/diff/2002/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1214: scheduler_->UploadCancelled();
On 2012/06/08 08:26:42, stuartmorgan wrote:
> It's vital that UploadCancelled be called if something goes catastrophically
> wrong and neither current_fetch_xml_ nor current_fetch_proto_ end up with any
> content, otherwise the scheduler will never call back, so it seems like this
> path needs to stay, just modified to check for both being empty.

This check was logically redundant with the check on line 1126.  You're right,
though, that keeping the check here is probably appropriate; I've gone ahead and
removed the other one, keeping only this one.

[jar (doing other things), 2012-06-11 21:19:35]

mostly just nit comments....

LGTM

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

https://chromiumcodereview.appspot.com/10546044/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:1129: // compressed that, so that we
can signal that we're losing logs.
Simpler might be to add to some error histogram, and hope that some logs
eventually get sent (including the errors).  If no logs get sent... then none of
this will matter.
We might have a generic enumerated histogtram that records when we get
transmission errors, compression errors, etc.

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

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1320: VLOG(1) << "Metrics response
code: " << response_code
nit: I think there was a move to reduce the verbosity of the logging... since
this cost a bunch of text-segment space, and is almost(?) never used.

Is is reasonable to change this to a Debug only log?

Also note that the histograms in lines 1306 and 1310 allow a developer to see
what is going on via about:histograms.

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1345: VLOG(1) << "Metrics response
data: " << response_data;
nit: can/should this be debug-only logging?

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1406: VLOG(1) << "Metrics:
transmission attempt returned a failure code: "
nit: debug only?  Will folks ever notice this in official builds, or does it
waste space?

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/browser/metr...
chrome/browser/metrics/metrics_service.cc:1420: << "Metrics: transmission retry
being scheduled for protobuf upload.";
nit: vlogs throughout.

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/common/metri...
File chrome/common/metrics/metrics_log_manager.cc (right):

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/common/metri...
chrome/common/metrics/metrics_log_manager.cc:101: !staged_log_text().xml.empty()
&&
nit: don't bother to wrap line 100 this way, as it was rather jarring to see a
return with nothing on the line.  Better IMO is to put 101 on the end of line
100, and wrap 102 as you did.

[Ilya Sherman, 2012-06-11 22:22:25]

Will follow up on the remaining comments in separate CLs, since they are more
about existing code than the re-uploading changes.

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/common/metri...
File chrome/common/metrics/metrics_log_manager.cc (right):

https://chromiumcodereview.appspot.com/10546044/diff/6001/chrome/common/metri...
chrome/common/metrics/metrics_log_manager.cc:101: !staged_log_text().xml.empty()
&&
On 2012/06/11 21:19:35, jar wrote:
> nit: don't bother to wrap line 100 this way, as it was rather jarring to see a
> return with nothing on the line.  Better IMO is to put 101 on the end of line
> 100, and wrap 102 as you did.

Done.

[commit-bot: I haz the power, 2012-06-11 22:23:19]

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

[commit-bot: I haz the power, 2012-06-11 22:41:27]

Try job failure for 10546044-7006 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-11 22:46:25]

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

[commit-bot: I haz the power, 2012-06-12 00:13:57]

Change committed as 141570