Starting a refactor to allow adding metrics on upload.

components/metrics/metrics_service.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 //------------------------------------------------------------------------------
 // Description of the life cycle of a instance of MetricsService.
 //
 //  OVERVIEW
 //
 // A MetricsService instance is typically created at application startup.  It is
@@ skipping 831 matching lines @@
     scheduler_->UploadFinished(true /* healthy */, false /* no unsent logs */);
     return;
   }
   // If there are unsent logs, send the next one. If not, start the asynchronous
   // process of finalizing the current log for upload.
   if (state_ == SENDING_OLD_LOGS) {
     DCHECK(log_manager_.has_unsent_logs());
     log_manager_.StageNextLogForUpload();
     SendStagedLog();
   } else {
+    for (size_t i = 0; i < metrics_providers_.size(); ++i)
+      metrics_providers_[i]->OnCollectFinalMetrics();

[Alexei Svitkine (slow), 2014/10/17 18:30:52]

Can you call this from DisableRecording() too?

Per my previous reply - this will cover cases where a log is closed without
hitting CollectFinalMetrics().

[Maria, 2014/10/17 21:44:50]

It's a little weird I think to call onCollectFinalMetrics from DisableRecording
without calling CollectFinalMetrics. What's the reason we don't call that method
before disabling?

Do you think it makes sense to add a call from OnDisableRecording to
OnCollectFinalMetrics inside the provider?

[Alexei Svitkine (slow), 2014/10/20 15:47:00]

It's not done because CollectFinalMetrics() involves some async work (i.e.
contact subprocesses to get their metrics) and the DisableRecording codepath is
executed on a shutdown when closing the final log. So I think the idea is to not
block shut down due to async work.
I'd prefer we do it in metrics service, so that each provider that wants to
record such a histogram doesn't need to worry about making sure to do this thing
itself. I agree that OnCollectFinalMetrics would be misleading in this case.

Thinking about this more, let's not introduce the new OnCollectFinalMetrics()
method at all and instead use the existing ProvideGeneralMetrics() method.
Current users of that method only provide non-histogram metrics (i.e. set the
protobuf fields themselves), but it looks like we can easily make that support
logging histograms from it.

To do that, the two calls to RecordGeneralMetrics() in metrics_service.cc have
to be moved just above  RecordCurrentHistograms(). I checked all the current
implementors of ProvideGeneralMetrics() and it should be safe to do (none of
their functionality depends on the ordering w.r.t. to histogram recording).

When you do this, please also expand the comment on ProvideGeneralMetrics() in
metrics_provider.h to mention that this method can also be used to record
histograms that will recorded with every metrics log.

+
     client_->CollectFinalMetrics(
         base::Bind(&MetricsService::OnFinalLogInfoCollectionDone,
                    self_ptr_factory_.GetWeakPtr()));
   }
 }
 
 void MetricsService::OnFinalLogInfoCollectionDone() {
   // If somehow there is a log upload in progress, we return and hope things
   // work out. The scheduler isn't informed since if this happens, the scheduler
   // will get a response from the upload.
@@ skipping 379 matching lines @@
   local_state_->SetBoolean(path, value);
   RecordCurrentState(local_state_);
 }
 
 void MetricsService::RecordCurrentState(PrefService* pref) {
   pref->SetInt64(metrics::prefs::kStabilityLastTimestampSec,
                  base::Time::Now().ToTimeT());
 }
 
 }  // namespace metrics