Begin to separate the MetricsService logic for creating vs uploading logs

Begin to separate the MetricsService logic for creating vs uploading logs

The biggest change here is to remove the dedicated current->staged->upload pipeline, and instead have finishing a log add it to the queue (the same one that's used for old logs that came from disk), and always do uploading by staging from the queue. That will allow for MetricsService to handle cutting logs and uploading logs separately in a future CL (and allows for a bit of simplification now in the shutdown case, where it's no longer necessary to stage the current log for upload just so that it's possible to immediately store it).

The rest of the MetricsService changes aren't actually changing the flow, the just split some things into more methods so that future CLs to separate cutting logs and uploading them will be smaller and easier to understand.

StartFinalLogInfoCollection/OnFinalLogInfoCollectionDone were added to make it much more clear how StartScheduledUpload gets to the code that actually does the uploading, since it's confusing to have to follow the jumps through the various implementation details of the specific info being gathered to see what the end point is.

BUG=None
TEST=Metrics should continue to be reported from official builds


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

[stuartmorgan, 2012-02-14 16:34:08]

This is the first of what will be several CLs to cleanly support turning on and
off reporting without affecting the way logs are generated, per our recent
discussions.

Most of this change is covered by the log manager unit tests, but I've also
verified that in a local build with logging forced on, the normal log
creating/uploading sequence is still working correctly.

[Ilya Sherman, 2012-02-14 19:02:51]

Just as a heads up, note that https://chromiumcodereview.appspot.com/9232071/ is
also in flight.  Not sure if we'll be stepping on each other's toes with these
CLs -- my changes to the MetricsService class are relatively constrained.

[stuartmorgan, 2012-02-22 08:31:30]

jar: ping?

It's really important to us that we get this into M19, and we all agree that
earlier is better when landing metrics changes. Are you going to have time to
look at this soon? If not, should Ilya do the review instead?

[stuartmorgan, 2012-02-28 08:35:22]

Ilya, any change you could give this an initial review to hopefully speed things
up?

[Ilya Sherman, 2012-02-29 01:25:41]

Looks ok to me, modulo the question of using std::string::swap() in more places
to avoid copying large strings when possible.

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

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:42: LogType log_type =
unsent_initial_logs_.empty() ? ONGOING_LOG : INITIAL_LOG;
nit: I'm not sure how much breaking out this variable helps readability, since
you only use it on the line below, where using unsent_initial_logs_.empty()
would also be pretty clear.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:47: compressed_staged_log_text_ =
source_list->back();
Please use std::string::swap() here to avoid copying the log text, which can be
quite large.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:90: }
If we can avoid copying the log text by using std::string::swap() here, that
would probably be better, since the log text can get quite large.

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

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:38: // later upload as |log_type|,
leaving |current_log| NULL.
nit: current_log -> current_log_; log_type -> log_type_

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:46: // Populates staged_log_text
with the next stored log to send.
nit: staged_log_text -> |staged_log_text_|

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:47: // Should only be called if
has_unsent_logs is true.
nit: has_unsent_logs -> has_unsent_logs()

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:75: //
set_max_ongoing_log_store_size), then clears the staged log.
nit: set_max_ongoing_log_store_size -> |max_ongoing_log_store_size_|

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome_frame/metrics_se...
File chrome_frame/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome_frame/metrics_se...
chrome_frame/metrics_service.cc:447: NOTREACHED() << "Failed to prepare log for
transmission.";
nit: There was a thread recently about not printing easily inferable log lines
corresponding to DCHECK() and NOTREACHED(), for the sake of reducing the binary
size.  Could this text be moved into a comment instead?

[stuartmorgan, 2012-02-29 13:26:15]

Comments addressed, and I've updated to include your recently-landed protobuf
changes.

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

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:42: LogType log_type =
unsent_initial_logs_.empty() ? ONGOING_LOG : INITIAL_LOG;
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: I'm not sure how much breaking out this variable helps readability, since
> you only use it on the line below, where using unsent_initial_logs_.empty()
> would also be pretty clear.

Yep, this was an artifact of a previous iteration. Removed.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:47: compressed_staged_log_text_ =
source_list->back();
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> Please use std::string::swap() here to avoid copying the log text, which can
be
> quite large.

Good tip; I wasn't familiar with swap(). (It's a bit awkward with both xml and
proto, so I added another temporary utility method to the SerializedLog struct.)

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.cc:90: }
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> If we can avoid copying the log text by using std::string::swap() here, that
> would probably be better, since the log text can get quite large.

The semantics of the StoreLog method become unfortunate, but it's an internal
method, where non-obvious semantics are less of a concern, so the efficiency win
seems worthwhile. Done.

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

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:38: // later upload as |log_type|,
leaving |current_log| NULL.
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: current_log -> current_log_; log_type -> log_type_

Changed to current_log() (I prefer to document public methods in terms of public
API rather than private members when it's reasonable to do so).

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:46: // Populates staged_log_text
with the next stored log to send.
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: staged_log_text -> |staged_log_text_|

Same.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:47: // Should only be called if
has_unsent_logs is true.
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: has_unsent_logs -> has_unsent_logs()

Done.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome/common/metrics/m...
chrome/common/metrics/metrics_log_manager.h:75: //
set_max_ongoing_log_store_size), then clears the staged log.
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: set_max_ongoing_log_store_size -> |max_ongoing_log_store_size_|

That part of the comment was actually no longer accurate, so I've removed it.

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome_frame/metrics_se...
File chrome_frame/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/9396001/diff/1/chrome_frame/metrics_se...
chrome_frame/metrics_service.cc:447: NOTREACHED() << "Failed to prepare log for
transmission.";
On 2012/02/29 01:25:41, Ilya Sherman wrote:
> nit: There was a thread recently about not printing easily inferable log lines
> corresponding to DCHECK() and NOTREACHED(), for the sake of reducing the
binary
> size.  Could this text be moved into a comment instead?

Yep, should have noticed that while I was moving the code. Done.

[Ilya Sherman, 2012-02-29 18:58:51]

LG, thanks :)

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

https://chromiumcodereview.appspot.com/9396001/diff/10001/chrome/common/metri...
chrome/common/metrics/metrics_log_manager.h:31: void swap(SerializedLog& log);
nit: I think the typical Chromium style is to pass |log| as a pointer rather
than as a reference; but if you prefer to keep this style to match the signature
for std::string::swap, that's probably fine too.

[stuartmorgan, 2012-02-29 19:04:01]

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

https://chromiumcodereview.appspot.com/9396001/diff/10001/chrome/common/metri...
chrome/common/metrics/metrics_log_manager.h:31: void swap(SerializedLog& log);
On 2012/02/29 18:58:51, Ilya Sherman wrote:
> nit: I think the typical Chromium style is to pass |log| as a pointer rather
> than as a reference; but if you prefer to keep this style to match the
signature
> for std::string::swap, that's probably fine too.

Yep, that was the intent. I went back and forth, and decided that since it's
temporary code designed to minimize the friction of having this class instead of
the std::string we'll be (I assume) going back to, it was easier this way.

[Ilya Sherman, 2012-03-01 07:08:42]

LGTM

[jar (doing other things), 2012-03-01 07:37:40]

rubber stamp LGTM (per Isherman's review).

As noted in out-of-band email, I *suspect* the approach being driven toward (to
reportedly be able to cut logs, but then queue them with the "unsent logs") may
not be as effective as avoiding the formation of the log until 3g is available.

That said, I too have always found the architecture of this module confusing and
difficult to follow, and I've often thought of re-writing it... but instead, all
I've ever had time to do is to remove bugs, and make incremental motions in
better(?) directions.

[commit-bot: I haz the power, 2012-03-01 10:49:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/9396001/10001

[commit-bot: I haz the power, 2012-03-01 12:15:11]

Change committed as 124405