[Chromoting] Use ProcessStatsSender in DaemonProcess (daemon process)

remoting/host/daemon_process.h

 // 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.
 
 #ifndef REMOTING_HOST_DAEMON_PROCESS_H_
 #define REMOTING_HOST_DAEMON_PROCESS_H_
 
 #include <stdint.h>
 
 #include <list>
 #include <memory>
 #include <string>
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #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;
 
 namespace tracked_objects {
 class Location;
 }  // namespace tracked_objects
 
 namespace remoting {
 
 class AutoThreadTaskRunner;
 class DesktopSession;
 class HostEventLogger;
 class HostStatusObserver;
+class ProcessStatsSender;
 class ScreenResolution;
 
 // This class implements core of the daemon process. It manages the networking
 // process running at lower privileges and maintains the list of desktop
 // sessions.
 class DaemonProcess
     : public ConfigWatcher::Delegate,
       public HostStatusMonitor,
-      public WorkerProcessIpcDelegate {
+      public WorkerProcessIpcDelegate,
+      public protocol::ProcessStatsStub {
  public:
   typedef std::list<DesktopSession*> DesktopSessionList;
 
   ~DaemonProcess() override;
 
   // Creates a platform-specific implementation of the daemon process object
   // passing relevant task runners. Public methods of this class must be called
   // on the |caller_task_runner| thread. |io_task_runner| is used to handle IPC
   // and background I/O tasks.
   static std::unique_ptr<DaemonProcess> Create(
@@ skipping 94 matching lines @@
   // Let the test code analyze the list of desktop sessions.
   friend class DaemonProcessTest;
   const DesktopSessionList& desktop_sessions() const {
     return desktop_sessions_;
   }
 
  private:
   // 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_;
 
   // Handles IPC and background I/O tasks.
   scoped_refptr<AutoThreadTaskRunner> io_task_runner_;
 
   std::unique_ptr<ConfigWatcher> config_watcher_;
 
   // The configuration file contents.
   std::string serialized_config_;
 
   // The list of active desktop sessions.
   DesktopSessionList desktop_sessions_;
 
   // The highest desktop session ID that has been seen so far.
   int next_terminal_id_;
 
   // Keeps track of observers receiving host status notifications.
   base::ObserverList<HostStatusObserver> status_observers_;
 
   // Invoked to ask the owner to delete |this|.
   base::Closure stopped_callback_;
 
   // 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);
 };
 
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_DAEMON_PROCESS_H_