[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

remoting/host/daemon_process.h

Index: remoting/host/daemon_process.h
diff --git a/remoting/host/daemon_process.h b/remoting/host/daemon_process.h
index 391682a3534e1ff9a8332aad4acaa03f18185969..af9aab298b56dd88a9cbc614b061919682e87d35 100644
--- a/remoting/host/daemon_process.h
+++ b/remoting/host/daemon_process.h
@@ -17,11 +17,14 @@
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/process/process.h"
+#include "base/time/time.h"
 #include "ipc/ipc_channel.h"
 #include "ipc/ipc_channel_handle.h"
 #include "remoting/host/config_watcher.h"
+#include "remoting/host/current_process_stats_agent.h"
 #include "remoting/host/host_status_monitor.h"
 #include "remoting/host/worker_process_ipc_delegate.h"
+#include "remoting/protocol/process_stats_stub.h"
 
 struct SerializedTransportRoute;
 
@@ -35,6 +38,7 @@ class AutoThreadTaskRunner;
 class DesktopSession;
 class HostEventLogger;
 class HostStatusObserver;
+class ProcessStatsSender;
 class ScreenResolution;
 
 // This class implements core of the daemon process. It manages the networking
@@ -43,7 +47,8 @@ class ScreenResolution;
 class DaemonProcess
     : public ConfigWatcher::Delegate,
       public HostStatusMonitor,
-      public WorkerProcessIpcDelegate {
+      public WorkerProcessIpcDelegate,
+      public protocol::ProcessStatsStub {
  public:
   typedef std::list<DesktopSession*> DesktopSessionList;
 
@@ -158,6 +163,17 @@ class DaemonProcess
   // Deletes all desktop sessions.
   void DeleteAllDesktopSessions();
 
+  // Starts to report process statistic data to network process. If |interval|
+  // is less then or equal to 0, a default non-zero value will be used.
+  void StartProcessStatsReport(base::TimeDelta interval);
+
+  // Stops sending process statistic data to network process.
+  void StopProcessStatsReport();
+
+  // ProcessStatsStub implementation.
+  void OnProcessStats(
+      const protocol::AggregatedProcessResourceUsage& usage) override;
+
   // Task runner on which public methods of this class must be called.
   scoped_refptr<AutoThreadTaskRunner> caller_task_runner_;
 
@@ -184,6 +200,20 @@ class DaemonProcess
   // Writes host status updates to the system event log.
   std::unique_ptr<HostEventLogger> host_event_logger_;
 
+  // Reports process statistic data to network process.
+  std::unique_ptr<ProcessStatsSender> stats_sender_;
+
+  // The number of StartProcessStatsReport requests received.
+  // Daemon and Network processes manages multiple desktop sessions. Some of

[joedow, 2017/06/21 15:30:50]

Process stats data is tied to a specific ClientSession (which lives in the
network process), it be initiated from the desktop session right?

My mental model was that the Network process (specifically the ClientSession
instance) would send the initial start/stop IPC messages to the other processes.

Individual desktop sessions would start/stop on their own as they receive IPC
messages and are created / destroyed.  We shouldn't need ref-counting in the
Daemon process to track them.

[joedow, 2017/06/21 15:32:04]

Oops., the top line should say:
Process stats data is tied to a specific ClientSession (which lives in the
network process), it shouldn't be initiated from the desktop session right?

[Hzj_jie, 2017/06/21 21:42:56]

After some more detailed investigations, I believe current solution is still
reasonable.
The ref-counter is just a two-line change, but it matches existing design of the
host instance in network process. If it was a 20-line change, it would be
questionable whether we should support this non-supported feature. But a
two-line change seems like a no-brainer to me: why should not we avoid the
confusion for free?
From the analysis perspective, daemon process is far more important than the RDP
process. Daemon process handles all functions RDP process handles, plus
configuration update, host status monitoring, connect / disconnect client
sessions, etc. More important is that daemon process is running on every
machine. But RDP process is running when curtain mode is on, which, I doubt, is
for Googlers only.
I have no concern to also let RDP process report its resource usage though.

[joedow, 2017/06/21 22:00:19]

My concerns are:
1.) The ref count code isn't going to be exercised since we don't support
multiple connections
2.) LOC doesn't really matter, if code is never run then it doesn't provide
value (whether it is 2 lines or 20)
3.) I don't think the Daemon process or RDP session is interesting enough to
monitor.  The question I posed is whether anyone can think of a scenario that we
require the data, so far I have not been able to.

My proposal is to only report Network and Desktop process info.
- This is future proof with respect to concurrent connections
- It is resilient in that the cleanup is guaranteed
  - desktop session is closed when session ends
  - Client session owns the Network process sender which is destroyed when the
session is closed
- It provide data for the processes we are most likely to want data on
- You can rename the IPC messages to be more specific (remove the 'any'
sender/recipient term and replace with the actual process name.  This makes the
IPC message code more readable

If we are able to conceive a scenario where having the Daemon or RDP process
data is useful then adding more code is fine.  I can't think of anything that
isn't a stretch.

+  // them may request for process statistic reports. So the resource usage of
+  // daemon process and network process will be merged to each desktop session.
+  //
+  // As long as at least process statistic reports is enabled for one desktop
+  // session, daemon process should continually send the reports.
+  int process_stats_request_count_ = 0;
+
+  CurrentProcessStatsAgent current_process_stats_;
+
   base::WeakPtrFactory<DaemonProcess> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DaemonProcess);