[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

remoting/host/daemon_process.cc

Index: remoting/host/daemon_process.cc
diff --git a/remoting/host/daemon_process.cc b/remoting/host/daemon_process.cc
index 7e3142da453b9851d676f8eea3f9f72b192f4b40..2976a72d2dd15679a592d123bbc3fa3a036aeb70 100644
--- a/remoting/host/daemon_process.cc
+++ b/remoting/host/daemon_process.cc
@@ -16,6 +16,7 @@
 #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"
@@ -23,6 +24,7 @@
 #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"
 
@@ -120,6 +122,10 @@ bool DaemonProcess::OnMessageReceived(const IPC::Message& message) {
                         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()
 
@@ -139,6 +145,11 @@ void DaemonProcess::OnPermanentError(int exit_code) {
   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());
 
@@ -180,6 +191,7 @@ DaemonProcess::DaemonProcess(
       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
@@ -287,6 +299,8 @@ void DaemonProcess::Initialize() {
 void DaemonProcess::Stop() {
   DCHECK(caller_task_runner()->BelongsToCurrentThread());
 
+  OnWorkerProcessStopped(0);
+
   if (!stopped_callback_.is_null()) {
     base::ResetAndReturn(&stopped_callback_).Run();
   }
@@ -365,4 +379,37 @@ void DaemonProcess::DeleteAllDesktopSessions() {
   }
 }
 
+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