[remoting][FTL] Integrate with new telemetry log server

* Implement RemotingLogToServer to talk to the new telemetry server
* Add a unittest for it
* Modify existing code to use it. For IT2ME we also have to make
  XmppLogToServer able to be initialized on a sequence different than
  the constructor's sequence, so that It2MeNativeMessagingHost can
  create it on the main thread and It2MeHost can use it on the
  network thread
* Make HeartbeatSender send a log when it heartbeats, similar to the
  behavior of XmppHeartbeatSender

Bug: 966993
Change-Id: I3c313acff91bad3ff3b28dd6d803669780dbac50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1629148
Commit-Queue: Joe Downing <joedow@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#664138}

remoting/host/heartbeat_sender.cc

@@ -22,8 +22,11 @@
 #include "remoting/base/logging.h"
 #include "remoting/base/service_urls.h"
 #include "remoting/host/host_details.h"
+#include "remoting/host/server_log_entry_host.h"
 #include "remoting/proto/remoting/v1/directory_service.grpc.pb.h"
 #include "remoting/signaling/ftl_signal_strategy.h"
+#include "remoting/signaling/log_to_server.h"
+#include "remoting/signaling/server_log_entry.h"
 #include "remoting/signaling/signal_strategy.h"
 #include "remoting/signaling/signaling_address.h"
 #include "remoting/signaling/xmpp_signal_strategy.h"
@@ -116,15 +119,18 @@ HeartbeatSender::HeartbeatSender(
     const std::string& host_id,
     MuxingSignalStrategy* signal_strategy,
     const scoped_refptr<const RsaKeyPair>& host_key_pair,
-    OAuthTokenGetter* oauth_token_getter)
+    OAuthTokenGetter* oauth_token_getter,
+    LogToServer* log_to_server)
     : on_heartbeat_successful_callback_(
           std::move(on_heartbeat_successful_callback)),
       on_unknown_host_id_error_(std::move(on_unknown_host_id_error)),
       host_id_(host_id),
       signal_strategy_(signal_strategy),
       host_key_pair_(host_key_pair),
-      client_(std::make_unique<HeartbeatClient>(oauth_token_getter)) {
+      client_(std::make_unique<HeartbeatClient>(oauth_token_getter)),
+      log_to_server_(log_to_server) {
   DCHECK(host_key_pair_.get());
+  DCHECK(log_to_server_);
 
   signal_strategy_->AddListener(this);
   OnSignalStrategyStateChange(signal_strategy_->GetState());
@@ -214,6 +220,11 @@ void HeartbeatSender::SendHeartbeat() {
       CreateHeartbeatRequest(),
       base::BindOnce(&HeartbeatSender::OnResponse, base::Unretained(this)));
   ++sequence_id_;
+
+  // Send a heartbeat log
+  std::unique_ptr<ServerLogEntry> log_entry = MakeLogEntryForHeartbeat();
+  AddHostFieldsToLogEntry(log_entry.get());
+  log_to_server_->Log(*log_entry);
 }
 
 void HeartbeatSender::OnResponse(const grpc::Status& status,

remoting/host/heartbeat_sender.h

@@ -29,6 +29,7 @@ class Status;
 
 namespace remoting {
 
+class LogToServer;
 class OAuthTokenGetter;
 class RsaKeyPair;
 
@@ -73,7 +74,8 @@ class HeartbeatSender final : public SignalStrategy::Listener {
                   const std::string& host_id,
                   MuxingSignalStrategy* signal_strategy,
                   const scoped_refptr<const RsaKeyPair>& host_key_pair,
-                  OAuthTokenGetter* oauth_token_getter);
+                  OAuthTokenGetter* oauth_token_getter,
+                  LogToServer* log_to_server);
   ~HeartbeatSender() override;
 
   // Sets host offline reason for future heartbeat, and initiates sending a
@@ -121,6 +123,7 @@ class HeartbeatSender final : public SignalStrategy::Listener {
   MuxingSignalStrategy* const signal_strategy_;
   scoped_refptr<const RsaKeyPair> host_key_pair_;
   std::unique_ptr<HeartbeatClient> client_;
+  LogToServer* const log_to_server_;
 
   base::OneShotTimer heartbeat_timer_;
 

remoting/host/heartbeat_sender_unittest.cc

@@ -8,6 +8,7 @@
 
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
@@ -22,6 +23,7 @@
 #include "remoting/base/test_rsa_key_pair.h"
 #include "remoting/proto/remoting/v1/directory_service.grpc.pb.h"
 #include "remoting/signaling/fake_signal_strategy.h"
+#include "remoting/signaling/log_to_server.h"
 #include "remoting/signaling/muxing_signal_strategy.h"
 #include "remoting/signaling/signaling_address.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -114,7 +116,7 @@ decltype(auto) DoValidateHeartbeatAndRespondOk(
 
 }  // namespace
 
-class HeartbeatSenderTest : public testing::Test {
+class HeartbeatSenderTest : public testing::Test, public LogToServer {
  public:
   HeartbeatSenderTest() {
     auto ftl_signal_strategy =
@@ -137,7 +139,7 @@ class HeartbeatSenderTest : public testing::Test {
     heartbeat_sender_ = std::make_unique<HeartbeatSender>(
         mock_heartbeat_successful_callback_.Get(),
         mock_unknown_host_id_error_callback_.Get(), kHostId,
-        muxing_signal_strategy_.get(), key_pair, &oauth_token_getter_);
+        muxing_signal_strategy_.get(), key_pair, &oauth_token_getter_, this);
     heartbeat_sender_->SetGrpcChannelForTest(
         test_server_.CreateInProcessChannel());
   }
@@ -155,7 +157,16 @@ class HeartbeatSenderTest : public testing::Test {
   base::MockCallback<base::OnceClosure> mock_heartbeat_successful_callback_;
   base::MockCallback<base::OnceClosure> mock_unknown_host_id_error_callback_;
 
+  std::vector<ServerLogEntry> received_log_entries_;
+
  private:
+  // LogToServer interface.
+  void Log(const ServerLogEntry& entry) override {
+    received_log_entries_.push_back(entry);
+  }
+
+  ServerLogEntry::Mode mode() const override { return ServerLogEntry::ME2ME; }
+
   std::unique_ptr<MuxingSignalStrategy> muxing_signal_strategy_;
 
   // |heartbeat_sender_| must be deleted before |muxing_signal_strategy_|.
@@ -387,4 +398,25 @@ TEST_F(HeartbeatSenderTest, UnknownHostId) {
   run_loop.Run();
 }
 
+TEST_F(HeartbeatSenderTest, SendHeartbeatLogEntryOnHeartbeat) {
+  base::RunLoop run_loop;
+
+  EXPECT_CALL(*test_server_, Heartbeat(_, _, _))
+      .WillOnce(DoValidateHeartbeatAndRespondOk(/* ftl */ true,
+                                                /* xmpp */ true, 0));
+
+  EXPECT_CALL(mock_heartbeat_successful_callback_, Run()).WillOnce([&]() {
+    run_loop.Quit();
+  });
+
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindLambdaForTesting([&]() {
+        ftl_signal_strategy_->Connect();
+        xmpp_signal_strategy_->Connect();
+      }));
+  run_loop.Run();
+
+  ASSERT_EQ(1u, received_log_entries_.size());
+}
+
 }  // namespace remoting

remoting/host/host_status_logger.cc

@@ -14,11 +14,10 @@
 namespace remoting {
 
 HostStatusLogger::HostStatusLogger(scoped_refptr<HostStatusMonitor> monitor,
-                                   ServerLogEntry::Mode mode,
-                                   SignalStrategy* signal_strategy,
-                                   const std::string& directory_bot_jid)
-    : xmpp_log_to_server_(mode, signal_strategy, directory_bot_jid),
-      monitor_(monitor) {
+                                   LogToServer* log_to_server)
+    : log_to_server_(log_to_server), monitor_(monitor) {
+  DCHECK(log_to_server_);
+  DCHECK(monitor_);
   monitor_->AddStatusObserver(this);
 }
 
@@ -34,12 +33,12 @@ void HostStatusLogger::LogSessionStateChange(const std::string& jid,
   std::unique_ptr<ServerLogEntry> entry(
       MakeLogEntryForSessionStateChange(connected));
   AddHostFieldsToLogEntry(entry.get());
-  entry->AddModeField(xmpp_log_to_server_.mode());
+  entry->AddModeField(log_to_server_->mode());
 
   if (connected && connection_route_type_.count(jid) > 0)
     AddConnectionTypeToLogEntry(entry.get(), connection_route_type_[jid]);
 
-  xmpp_log_to_server_.Log(*entry.get());
+  log_to_server_->Log(*entry.get());
 }
 
 void HostStatusLogger::OnClientConnected(const std::string& jid) {
@@ -64,8 +63,4 @@ void HostStatusLogger::OnClientRouteChange(
   }
 }
 
-void HostStatusLogger::SetSignalingStateForTest(SignalStrategy::State state) {
-  xmpp_log_to_server_.OnSignalStrategyStateChange(state);
-}
-
 }  // namespace remoting

remoting/host/host_status_logger.h

@@ -12,7 +12,7 @@
 #include "base/sequence_checker.h"
 #include "remoting/host/host_status_observer.h"
 #include "remoting/protocol/transport.h"
-#include "remoting/signaling/xmpp_log_to_server.h"
+#include "remoting/signaling/log_to_server.h"
 
 namespace remoting {
 
@@ -24,9 +24,7 @@ class HostStatusMonitor;
 class HostStatusLogger : public HostStatusObserver {
  public:
   HostStatusLogger(scoped_refptr<HostStatusMonitor> monitor,
-                   ServerLogEntry::Mode mode,
-                   SignalStrategy* signal_strategy,
-                   const std::string& directory_bot_jid);
+                   LogToServer* log_to_server);
   ~HostStatusLogger() override;
 
   // Logs a session state change. Currently, this is either
@@ -40,11 +38,8 @@ class HostStatusLogger : public HostStatusObserver {
                            const std::string& channel_name,
                            const protocol::TransportRoute& route) override;
 
-  // Allows test code to fake SignalStrategy state change events.
-  void SetSignalingStateForTest(SignalStrategy::State state);
-
  private:
-  XmppLogToServer xmpp_log_to_server_;
+  LogToServer* log_to_server_;
 
   scoped_refptr<HostStatusMonitor> monitor_;
 

remoting/host/host_status_logger_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "remoting/host/host_status_monitor.h"
 #include "remoting/signaling/mock_signal_strategy.h"
+#include "remoting/signaling/xmpp_log_to_server.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gmock_mutant.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -129,15 +130,17 @@ class HostStatusLoggerTest : public testing::Test {
         host_status_monitor_(new HostStatusMonitor()) {}
   void SetUp() override {
     EXPECT_CALL(signal_strategy_, AddListener(_));
-    host_status_logger_.reset(
-        new HostStatusLogger(host_status_monitor_, ServerLogEntry::ME2ME,
-                             &signal_strategy_, kTestBotJid));
+    log_to_server_ = std::make_unique<XmppLogToServer>(
+        ServerLogEntry::ME2ME, &signal_strategy_, kTestBotJid);
+    host_status_logger_ = std::make_unique<HostStatusLogger>(
+        host_status_monitor_, log_to_server_.get());
     EXPECT_CALL(signal_strategy_, RemoveListener(_));
   }
 
  protected:
   base::MessageLoop message_loop_;
   MockSignalStrategy signal_strategy_;
+  std::unique_ptr<XmppLogToServer> log_to_server_;
   std::unique_ptr<HostStatusLogger> host_status_logger_;
   scoped_refptr<HostStatusMonitor> host_status_monitor_;
 };
@@ -154,14 +157,13 @@ TEST_F(HostStatusLoggerTest, SendNow) {
         .WillOnce(QuitRunLoop(&run_loop))
         .RetiresOnSaturation();
   }
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::CONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
   protocol::TransportRoute route;
   route.type = protocol::TransportRoute::DIRECT;
   host_status_logger_->OnClientRouteChange(kClientJid1, "video", route);
   host_status_logger_->OnClientAuthenticated(kClientJid1);
   host_status_logger_->OnClientConnected(kClientJid1);
-  host_status_logger_->SetSignalingStateForTest(
-      SignalStrategy::DISCONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   run_loop.Run();
 }
 
@@ -183,8 +185,8 @@ TEST_F(HostStatusLoggerTest, SendLater) {
         .WillOnce(QuitRunLoop(&run_loop))
         .RetiresOnSaturation();
   }
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::CONNECTED);
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::DISCONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   run_loop.Run();
 }
 
@@ -212,8 +214,8 @@ TEST_F(HostStatusLoggerTest, SendTwoEntriesLater) {
         .WillOnce(QuitRunLoop(&run_loop))
         .RetiresOnSaturation();
   }
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::CONNECTED);
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::DISCONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   run_loop.Run();
 }
 
@@ -236,7 +238,7 @@ TEST_F(HostStatusLoggerTest, HandleRouteChangeInUnusualOrder) {
         .WillOnce(QuitRunLoop(&run_loop))
         .RetiresOnSaturation();
   }
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::CONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
   protocol::TransportRoute route1;
   route1.type = protocol::TransportRoute::DIRECT;
   host_status_logger_->OnClientRouteChange(kClientJid1, "video", route1);
@@ -248,7 +250,7 @@ TEST_F(HostStatusLoggerTest, HandleRouteChangeInUnusualOrder) {
   host_status_logger_->OnClientDisconnected(kClientJid1);
   host_status_logger_->OnClientAuthenticated(kClientJid2);
   host_status_logger_->OnClientConnected(kClientJid2);
-  host_status_logger_->SetSignalingStateForTest(SignalStrategy::DISCONNECTED);
+  log_to_server_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   run_loop.Run();
 }
 

remoting/host/it2me/it2me_host.cc

@@ -38,7 +38,7 @@
 #include "remoting/protocol/transport_context.h"
 #include "remoting/protocol/validating_authenticator.h"
 #include "remoting/signaling/jid_util.h"
-#include "remoting/signaling/server_log_entry.h"
+#include "remoting/signaling/log_to_server.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace remoting {
@@ -87,6 +87,7 @@ void It2MeHost::Connect(
     std::unique_ptr<base::DictionaryValue> policies,
     std::unique_ptr<It2MeConfirmationDialogFactory> dialog_factory,
     std::unique_ptr<RegisterSupportHostRequest> register_request,
+    std::unique_ptr<LogToServer> log_to_server,
     base::WeakPtr<It2MeHost::Observer> observer,
     std::unique_ptr<SignalStrategy> signal_strategy,
     const std::string& username,
@@ -98,6 +99,7 @@ void It2MeHost::Connect(
   observer_ = std::move(observer);
   confirmation_dialog_factory_ = std::move(dialog_factory);
   signal_strategy_ = std::move(signal_strategy);
+  log_to_server_ = std::move(log_to_server);
 
   OnPolicyUpdate(std::move(policies));
 
@@ -206,8 +208,7 @@ void It2MeHost::ConnectOnNetworkThread(
       host_context_->video_encode_task_runner(), options));
   host_->status_monitor()->AddStatusObserver(this);
   host_status_logger_.reset(
-      new HostStatusLogger(host_->status_monitor(), ServerLogEntry::IT2ME,
-                           signal_strategy_.get(), directory_bot_jid));
+      new HostStatusLogger(host_->status_monitor(), log_to_server_.get()));
 
   // Create event logger.
   host_event_logger_ =
@@ -486,6 +487,7 @@ void It2MeHost::DisconnectOnNetworkThread() {
 
   register_request_ = nullptr;
   host_status_logger_ = nullptr;
+  log_to_server_ = nullptr;
   signal_strategy_ = nullptr;
   host_event_logger_ = nullptr;
 

remoting/host/it2me/it2me_host.h

@@ -33,6 +33,7 @@ class ChromotingHostContext;
 class DesktopEnvironmentFactory;
 class HostEventLogger;
 class HostStatusLogger;
+class LogToServer;
 class RegisterSupportHostRequest;
 class RsaKeyPair;
 
@@ -81,11 +82,15 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   // Methods called by the script object, from the plugin thread.
 
   // Creates It2Me host structures and starts the host.
+  //
+  // XmppLogToServer cannot be created and used in different sequence, so pass
+  // in a factory callback instead.
   virtual void Connect(
       std::unique_ptr<ChromotingHostContext> context,
       std::unique_ptr<base::DictionaryValue> policies,
       std::unique_ptr<It2MeConfirmationDialogFactory> dialog_factory,
       std::unique_ptr<RegisterSupportHostRequest> register_request,
+      std::unique_ptr<LogToServer> log_to_server,
       base::WeakPtr<It2MeHost::Observer> observer,
       std::unique_ptr<SignalStrategy> signal_strategy,
       const std::string& username,
@@ -167,6 +172,7 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   std::unique_ptr<ChromotingHostContext> host_context_;
   base::WeakPtr<It2MeHost::Observer> observer_;
   std::unique_ptr<SignalStrategy> signal_strategy_;
+  std::unique_ptr<LogToServer> log_to_server_;
 
   It2MeHostState state_ = kDisconnected;