Create and integrate a metrics service client into Blimp engine.

blimp/engine/app/blimp_metrics_service_client.cc

+// Copyright 2016 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.
+
+#include "blimp/engine/app/blimp_metrics_service_client.h"
+
+#include "base/bind.h"
+#include "base/i18n/rtl.h"
+#include "base/memory/ptr_util.h"
+#include "components/metrics/call_stack_profile_metrics_provider.h"
+#include "components/metrics/gpu/gpu_metrics_provider.h"
+#include "components/metrics/metrics_service.h"
+#include "components/metrics/metrics_state_manager.h"
+#include "components/metrics/net/net_metrics_log_uploader.h"
+#include "components/metrics/profiler/profiler_metrics_provider.h"
+#include "components/metrics/ui/screen_info_metrics_provider.h"
+#include "components/metrics/url_constants.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/browser/browser_thread.h"
+
+namespace blimp {
+namespace engine {
+
+base::LazyInstance<BlimpMetricsServiceClient>::Leaky g_lazy_instance_;

[Wez, 2016/04/26 01:38:17]

This needs to be in the anonymous namespace, surely?

[Jess, 2016/04/26 21:24:10]

Done.

+
+namespace {
+
+// Standard interval between log uploads, in minutes

[Wez, 2016/04/26 01:38:18]

Can you reword this to explain what the "standard interval" is actually for;
otherwise the comment is almost the variable name verbatim.

nit: End comment with .
nit: Constant is already named kFooMinutes, so no need to mention units.

[Jess, 2016/04/26 21:24:10]

Done.

+const int kStandardUploadIntervalMinutes = 30;  // Thirty minutes.

[Wez, 2016/04/26 01:38:18]

nit: Trailing comment adds nothing ;)

[Jess, 2016/04/26 21:24:10]

Removed.

+
+bool IsReportingEnabled() {

[Wez, 2016/04/26 01:38:18]

nit: Suggest providing a one-line comment explaining that the metrics state
manager expects an is-enabled callback to be provided.

[Jess, 2016/04/26 21:24:10]

Done.

+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Wez, 2016/04/26 01:38:17]

Why this thread-check here but nowhere else?

[Jess, 2016/04/26 21:24:10]

Usually this call makes use of features the require the Browser UI thread, but
it isn't a requirement here. I kept it as a sanity check, but in hindsight here
is not the place for such a check.

Removed.

+  return true;
+}
+
+// Callbacks for metrics::MetricsStateManager::Create. Store/LoadClientInfo
+// allows Windows Chrome to back up ClientInfo. They're no-ops for Blimp.

[Wez, 2016/04/26 01:38:18]

IsReportingEnabled also seems to be a callback for that purpose?

[Jess, 2016/04/26 21:24:10]

See comment above.

+void StoreClientInfo(const metrics::ClientInfo& client_info) {}
+
+std::unique_ptr<metrics::ClientInfo> LoadClientInfo() {
+  return nullptr;
+}
+
+}  // namespace
+
+// static
+BlimpMetricsServiceClient* BlimpMetricsServiceClient::GetInstance() {
+  return g_lazy_instance_.Pointer();
+}
+
+// static
+void BlimpMetricsServiceClient::Initialize(
+    PrefService* pref_service,
+    net::URLRequestContextGetter* request_context) {
+  DCHECK(!is_initialized_);
+  pref_service_ = pref_service;
+  request_context_ = request_context;
+
+  metrics_state_manager_ = metrics::MetricsStateManager::Create(
+      pref_service_,
+      base::Bind(&IsReportingEnabled),
+      base::Bind(&StoreClientInfo), base::Bind(&LoadClientInfo));

[Wez, 2016/04/26 01:38:18]

nit: Is this the format "git cl format" gives? Seems strange to wrap the
preceding two lines but not this one.

[Jess, 2016/04/26 21:24:10]

Ran git cl format.

+
+  metrics_service_.reset(new ::metrics::MetricsService(

[Wez, 2016/04/26 01:38:18]

nit: Do we need the leading ::?

[Jess, 2016/04/26 21:24:10]

No. Removed.

+      metrics_state_manager_.get(), this, pref_service_));
+
+  metrics_service_->RegisterMetricsProvider(
+      base::WrapUnique<metrics::MetricsProvider>(
+          new metrics::NetworkMetricsProvider(
+              content::BrowserThread::GetBlockingPool())));

[Wez, 2016/04/26 01:38:18]

We should be very careful about using content::BrowserThread stuff; means that
we presumably have some constraints about when this instance must be created,
and torn, down, so that it's dependencies are met, and those should be
documented in the comment on the class.

[Jess, 2016/04/26 21:24:10]

Honestly I am not quite sure here.  Perhaps removal is the best option.  We can
always register it in a later cr. Thoughts?

+
+  metrics_service_->RegisterMetricsProvider(
+      base::WrapUnique<metrics::MetricsProvider>(
+          new metrics::GPUMetricsProvider));

[Wez, 2016/04/26 01:38:18]

nit: Why are these two registrations grouped together, separate from the
profiler and call-stack ones? Is there something special about these two?

[Jess, 2016/04/26 21:24:10]

Separated.

+  metrics_service_->RegisterMetricsProvider(
+      base::WrapUnique<metrics::MetricsProvider>(
+          new metrics::ScreenInfoMetricsProvider));
+
+  metrics_service_->RegisterMetricsProvider(
+      base::WrapUnique<metrics::MetricsProvider>(
+          new metrics::ProfilerMetricsProvider()));
+
+  metrics_service_->RegisterMetricsProvider(
+      base::WrapUnique<metrics::MetricsProvider>(
+          new metrics::CallStackProfileMetricsProvider));
+
+  metrics_service_->InitializeMetricsRecordingState();
+
+  is_initialized_ = true;
+
+  if (IsReportingEnabled()) {
+    metrics_service_->Start();
+  }
+}
+
+void BlimpMetricsServiceClient::Finalize() {
+  DCHECK(is_initialized_);
+  metrics_service_->Stop();
+}
+
+metrics::MetricsService* BlimpMetricsServiceClient::GetMetricsService() {
+  return metrics_service_.get();
+}
+
+// In Chrome, UMA and Breakpad are enabled/disabled together by the same
+// checkbox and they share the same client ID (a.k.a. GUID). SetMetricsClientId
+// and OnRecordingDisabled are intended to provide the ID to Breakpad.
+// This is not used on required by Blimp, so these are no-ops.

[Wez, 2016/04/26 01:38:18]

typo: on -> or

I think it's reasonable to just say "We don't implement SetMetricsClientId since
we don't use Breakpad." - the MetricsServiceClient interface docs should explain
what the API would normally do.

It's not clear what OnRecordingDisabled has to do with providing an Id to
anything - it doesn't seem to even have a parameter?

[Jess, 2016/04/26 21:24:10]

Based on Marcin's work I think we actually do use breakpad, however unlike other
Chromium products we don't care about mapping between UMA and breakpad client
ids.

Separated out note on OnRecordingDisabled.

+void BlimpMetricsServiceClient::SetMetricsClientId(
+    const std::string& client_id) {}
+
+void BlimpMetricsServiceClient::OnRecordingDisabled() {}
+
+bool BlimpMetricsServiceClient::IsOffTheRecordSessionActive() {
+  // Blimp does not have incognito mode.
+  return false;
+}
+
+int32_t BlimpMetricsServiceClient::GetProduct() {
+  // Indicates product family, not reported platform.

[Wez, 2016/04/26 01:38:17]

Not sure what this comment means by "reported platform"?

Is this comment really specific to Blimp, or should the GetProduct comment in
MetricsServiceClient interface be sufficient to understand?

[Jess, 2016/04/26 21:24:10]

Added exampled to help clarify.  This Product is not the same Product you see in
the metrics service UX.

+  return metrics::ChromeUserMetricsExtension::CHROME;
+}
+
+std::string BlimpMetricsServiceClient::GetApplicationLocale() {
+  return base::i18n::GetConfiguredLocale();
+}
+
+bool BlimpMetricsServiceClient::GetBrand(std::string* brand_code) {
+  // Blimp doesn't use brand codes.

[Wez, 2016/04/26 01:38:18]

Not clear from this comment what a brand-code even is? Sadly the
MetricsServiceClient comment for it is not very helpful either :-/

[Jess, 2016/04/26 21:24:10]

Acknowledged.

+  return false;
+}
+
+metrics::SystemProfileProto::Channel BlimpMetricsServiceClient::GetChannel() {
+  // Blimp engine does not have channel info yet.
+  return metrics::SystemProfileProto::CHANNEL_UNKNOWN;

[Wez, 2016/04/26 01:38:18]

nit: Hmmm, tricky; channel is used elsewhere to decide which APIs to support, so
we should be careful that returning UNKNOWN here doesn't lead to some weird
mismatch.

[Jess, 2016/04/26 21:24:10]

Do we have the concept of a channel within the engine?  I am happy to switch to
whatever is current practice.

+}
+
+std::string BlimpMetricsServiceClient::GetVersionString() {
+  // TODO(jessicag): Add in a meaningful version string.

[Wez, 2016/04/26 01:38:17]

Excitingly I'm working on a CL that will add such a thing and was just wondering
if we even needed it.  Yay!

[Jess, 2016/04/26 21:24:10]

Acknowledged.

+  return std::string();
+}
+
+void BlimpMetricsServiceClient::OnLogUploadComplete() {}
+
+void BlimpMetricsServiceClient::InitializeSystemProfileMetrics(
+    const base::Closure& done_callback) {
+  done_callback.Run();

[Wez, 2016/04/26 01:38:18]

Add one-line comment to clarify why it's OK to call the callback immediately,
here and below?

[Jess, 2016/04/26 21:24:10]

Done.

+}
+
+void BlimpMetricsServiceClient::CollectFinalMetricsForLog(
+    const base::Closure& done_callback) {
+  done_callback.Run();
+}
+
+std::unique_ptr<metrics::MetricsLogUploader>
+BlimpMetricsServiceClient::CreateUploader(
+    const base::Callback<void(int)>& on_upload_complete) {
+  return base::WrapUnique<metrics::MetricsLogUploader>(
+      new metrics::NetMetricsLogUploader(
+          // URLRequestContextGetter* that remains valid for class lifetime
+          // In place of g_browser_process->system_request_context()
+          // In engine use subclass BlimpURLRequestContextGetter.
+          // Member of g/e/common/blimp_browser_context.

[Wez, 2016/04/26 01:38:18]

This comment is already present in the class declaration of the member.

[Jess, 2016/04/26 21:24:10]

Cleaned up.

+          request_context_,
+          metrics::kDefaultMetricsServerUrl,
+          metrics::kDefaultMetricsMimeType,
+          on_upload_complete));
+}
+
+base::TimeDelta BlimpMetricsServiceClient::GetStandardUploadInterval() {
+  return base::TimeDelta::FromMinutes(kStandardUploadIntervalMinutes);
+}
+
+BlimpMetricsServiceClient::BlimpMetricsServiceClient()
+    : is_initialized_(false),
+      pref_service_(nullptr),
+      request_context_(nullptr) {}
+
+BlimpMetricsServiceClient::~BlimpMetricsServiceClient() {}
+
+}  // namespace engine
+}  // namespace blimp