[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

remoting/host/daemon_process.cc

 // Copyright (c) 2012 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 "remoting/host/daemon_process.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "remoting/base/auto_thread_task_runner.h"
+#include "remoting/base/constants.h"
 #include "remoting/host/branding.h"
 #include "remoting/host/chromoting_messages.h"
 #include "remoting/host/config_file_watcher.h"
 #include "remoting/host/desktop_session.h"
 #include "remoting/host/host_event_logger.h"
 #include "remoting/host/host_exit_codes.h"
 #include "remoting/host/host_status_observer.h"
+#include "remoting/host/process_stats_sender.h"
 #include "remoting/host/screen_resolution.h"
 #include "remoting/protocol/transport.h"
 
 namespace remoting {
 
 namespace {
 
 // This is used for tagging system event logs.
 const char kApplicationName[] = "chromoting";
 
@@ skipping 77 matching lines @@
     IPC_MESSAGE_HANDLER(ChromotingNetworkDaemonMsg_ClientConnected,
                         OnClientConnected)
     IPC_MESSAGE_HANDLER(ChromotingNetworkDaemonMsg_ClientDisconnected,
                         OnClientDisconnected)
     IPC_MESSAGE_HANDLER(ChromotingNetworkDaemonMsg_ClientRouteChange,
                         OnClientRouteChange)
     IPC_MESSAGE_HANDLER(ChromotingNetworkDaemonMsg_HostStarted,
                         OnHostStarted)
     IPC_MESSAGE_HANDLER(ChromotingNetworkDaemonMsg_HostShutdown,
                         OnHostShutdown)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkToAnyMsg_StartProcessStatsReport,
+                        StartProcessStatsReport)
+    IPC_MESSAGE_HANDLER(ChromotingNetworkToAnyMsg_StopProcessStatsReport,
+                        StopProcessStatsReport)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
   if (!handled) {
     LOG(ERROR) << "Received unexpected IPC type: " << message.type();
     CrashNetworkProcess(FROM_HERE);
   }
 
   return handled;
 }
 
 void DaemonProcess::OnPermanentError(int exit_code) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
   DCHECK(kMinPermanentErrorExitCode <= exit_code &&
          exit_code <= kMaxPermanentErrorExitCode);
 
   Stop();
 }
 
+void DaemonProcess::OnWorkerProcessStopped(int exit_code) {

[joedow, 2017/06/23 18:03:07]

You can remove the exit_code param since it isn't being used.

[Hzj_jie, 2017/06/24 00:51:40]

From the perspective of WorkerProcessLauncher, it's pretty reasonable to export
the exit_code in OnWorkerProcessStopped() callback. The same parameter exists in
OnPermanentError() callback.

[joedow, 2017/06/29 17:24:03]

I think we should prefer a cleaner function signature unless you can think of a
usage of the exit_code.  It is only used in a DCHECK in the permanent error
method (which is OK) but is completely unused here.  If we decide we need it at
some point in the future, we can add it back.

[Hzj_jie, 2017/06/29 20:24:57]

Removed.

+  process_stats_request_count_ = 0;
+  stats_sender_.reset();
+}
+
 void DaemonProcess::CloseDesktopSession(int terminal_id) {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
   // Validate the supplied terminal ID. An attempt to use a desktop session ID
   // that couldn't possibly have been allocated is considered a protocol error
   // and the network process will be restarted.
   if (!WasTerminalIdAllocated(terminal_id)) {
     LOG(ERROR) << "Invalid terminal ID: " << terminal_id;
     CrashNetworkProcess(FROM_HERE);
     return;
@@ skipping 21 matching lines @@
 }
 
 DaemonProcess::DaemonProcess(
     scoped_refptr<AutoThreadTaskRunner> caller_task_runner,
     scoped_refptr<AutoThreadTaskRunner> io_task_runner,
     const base::Closure& stopped_callback)
     : caller_task_runner_(caller_task_runner),
       io_task_runner_(io_task_runner),
       next_terminal_id_(0),
       stopped_callback_(stopped_callback),
+      current_process_stats_("DaemonProcess"),
       weak_factory_(this) {
   DCHECK(caller_task_runner->BelongsToCurrentThread());
   // TODO(sammc): On OSX, mojo::edk::SetMachPortProvider() should be called with
   // a base::PortProvider implementation. Add it here when this code is used on
   // OSX.
 }
 
 void DaemonProcess::CreateDesktopSession(int terminal_id,
                                          const ScreenResolution& resolution,
                                          bool virtual_terminal) {
@@ skipping 87 matching lines @@
   host_event_logger_ =
       HostEventLogger::Create(weak_factory_.GetWeakPtr(), kApplicationName);
 
   // Launch the process.
   LaunchNetworkProcess();
 }
 
 void DaemonProcess::Stop() {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
+  OnWorkerProcessStopped(0);
+
   if (!stopped_callback_.is_null()) {
     base::ResetAndReturn(&stopped_callback_).Run();
   }
 }
 
 bool DaemonProcess::WasTerminalIdAllocated(int terminal_id) {
   return terminal_id < next_terminal_id_;
 }
 
 void DaemonProcess::OnAccessDenied(const std::string& jid) {
@@ skipping 58 matching lines @@
     observer.OnShutdown();
 }
 
 void DaemonProcess::DeleteAllDesktopSessions() {
   while (!desktop_sessions_.empty()) {
     delete desktop_sessions_.front();
     desktop_sessions_.pop_front();
   }
 }
 
+void DaemonProcess::StartProcessStatsReport(base::TimeDelta interval) {
+  DCHECK(caller_task_runner()->BelongsToCurrentThread());
+  if (interval <= base::TimeDelta::FromSeconds(0)) {
+    interval = kDefaultProcessStatsInterval;
+  }
+
+  process_stats_request_count_++;
+  DCHECK(process_stats_request_count_ > 0);

[joedow, 2017/06/23 18:03:07]

If you want to keep the request_count, I'd prefer that we
DCHECK_EQ(process_stats_request_count_, 0); here instead (with a comment that
the functionality needs to be tested before enabling if we support multiple,
concurrent sessions in the future.

The reason is that since we can't test the code, I'd prefer to indicate that
explicitly instead of assuming it will work later.

The other option is to remove the process_stats_request_count_ completely and
DCHECK on whether the stats_sender_ is valid.

[Hzj_jie, 2017/06/24 00:51:40]

Test cases are added to cover the scenarios.
I think it's pretty fine to assume DaemonProcess can work as expected now :)

+  if (process_stats_request_count_ == 1 ||
+      stats_sender_->interval() > interval) {
+    DCHECK_EQ(process_stats_request_count_ == 1, !stats_sender_);

[joedow, 2017/06/23 18:03:07]

I don't think you should allow calling start multiple times with different
intervals, that just complicates the code.  I don't think the ability to change
the interval dynamically is useful, either way the originating process can send
a stop IPC and then restart the timers with the new interval.

Removing this extra condition means you can simplify the code too as
process_stats_request_count_ is guaranteed to be 1:

DCHECK(!stats_sender_);

[Hzj_jie, 2017/06/24 00:51:40]

Here the trouble is different sessions may request different interval. I have
added test case to cover it anyway.

+    stats_sender_.reset(new ProcessStatsSender(
+        this,
+        interval,
+        { &current_process_stats_ }));
+  }
+}
+
+void DaemonProcess::StopProcessStatsReport() {
+  DCHECK(caller_task_runner()->BelongsToCurrentThread());
+  process_stats_request_count_--;
+  DCHECK(process_stats_request_count_ >= 0);

[joedow, 2017/06/23 18:03:07]

DCHECK_EQ(process_stats_request_count_, 0)

[Hzj_jie, 2017/06/24 00:51:40]

Done.

[joedow, 2017/06/29 17:24:03]

I don't see this change in the latest patchset.

If you want to keep the condition, then you can use DCHECK_GE.

[Hzj_jie, 2017/06/29 20:24:57]

Sorry.

+  if (process_stats_request_count_ == 0) {
+    DCHECK(stats_sender_);
+    stats_sender_.reset();
+  }
+}
+
+void DaemonProcess::OnProcessStats(
+    const protocol::AggregatedProcessResourceUsage& usage) {
+  SendToNetwork(new ChromotingAnyToNetworkMsg_ReportProcessStats(usage));
+}
+
 }  // namespace remoting