Include additional host info in every heartbeat for logging purposes

Prior to this change, the host will always make at least two service
calls, one to heartbeat and one to log that it heartbeated.  This
dance is left over from the design which used Buzz as the signaling
channel.  Since both heartbeat and log calls go to the same service
API now, there isn't much value in having two separate calls.

In this CL, we will add a couple of fields to the heartbeat message
and populate them for every heartbeat.  The backend service will
determine whether or not they need to be persisted in the Directory.

Change-Id: I6f1f9ca18b903c63a36ad78ec1bc94869db64dd7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2170174
Commit-Queue: Gary Kacmarcik <garykac@chromium.org>
Auto-Submit: Joe Downing <joedow@chromium.org>
Reviewed-by: Gary Kacmarcik <garykac@chromium.org>
Cr-Commit-Position: refs/heads/master@{#763583}

remoting/host/heartbeat_sender.cc

@@ -13,6 +13,7 @@
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringize_macros.h"
+#include "base/system/sys_info.h"
 #include "base/time/time.h"
 #include "remoting/base/constants.h"
 #include "remoting/base/grpc_support/grpc_async_unary_request.h"
@@ -25,8 +26,6 @@
 #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/signaling_address.h"
 
 namespace remoting {
@@ -130,18 +129,15 @@ void HeartbeatSender::HeartbeatClientImpl::CancelPendingRequests() {
 HeartbeatSender::HeartbeatSender(Delegate* delegate,
                                  const std::string& host_id,
                                  SignalStrategy* signal_strategy,
-                                 OAuthTokenGetter* oauth_token_getter,
-                                 LogToServer* log_to_server)
+                                 OAuthTokenGetter* oauth_token_getter)
     : delegate_(delegate),
       host_id_(host_id),
       signal_strategy_(signal_strategy),
       client_(std::make_unique<HeartbeatClientImpl>(oauth_token_getter)),
-      log_to_server_(log_to_server),
       oauth_token_getter_(oauth_token_getter),
       backoff_(&kBackoffPolicy) {
   DCHECK(delegate_);
   DCHECK(signal_strategy_);
-  DCHECK(log_to_server_);
 
   signal_strategy_->AddListener(this);
   OnSignalStrategyStateChange(signal_strategy_->GetState());
@@ -241,11 +237,6 @@ void HeartbeatSender::SendHeartbeat() {
   client_->Heartbeat(
       CreateHeartbeatRequest(),
       base::BindOnce(&HeartbeatSender::OnResponse, base::Unretained(this)));
-
-  // 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,
@@ -349,21 +340,17 @@ apis::v1::HeartbeatRequest HeartbeatSender::CreateHeartbeatRequest() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   apis::v1::HeartbeatRequest heartbeat;
-  std::string signaling_id;
   heartbeat.set_tachyon_id(signal_strategy_->GetLocalAddress().id());
   heartbeat.set_host_id(host_id_);
   if (!host_offline_reason_.empty()) {
     heartbeat.set_host_offline_reason(host_offline_reason_);
   }
-  // Append host version.
   heartbeat.set_host_version(STRINGIZE(VERSION));
-  // If we have not recorded a heartbeat success, continue sending host OS info.
-  if (!initial_heartbeat_sent_) {
-    // Append host OS name.
-    heartbeat.set_host_os_name(GetHostOperatingSystemName());
-    // Append host OS version.
-    heartbeat.set_host_os_version(GetHostOperatingSystemVersion());
-  }
+  heartbeat.set_host_os_name(GetHostOperatingSystemName());
+  heartbeat.set_host_os_version(GetHostOperatingSystemVersion());
+  heartbeat.set_host_cpu_type(base::SysInfo::OperatingSystemArchitecture());
+  heartbeat.set_is_initial_heartbeat(!initial_heartbeat_sent_);
+
   return heartbeat;
 }
 

remoting/host/heartbeat_sender.h

@@ -27,7 +27,6 @@ class Status;
 
 namespace remoting {
 
-class LogToServer;
 class OAuthTokenGetter;
 
 // HeartbeatSender periodically sends heartbeat to the directory service. See
@@ -75,8 +74,7 @@ class HeartbeatSender final : public SignalStrategy::Listener {
   HeartbeatSender(Delegate* delegate,
                   const std::string& host_id,
                   SignalStrategy* signal_strategy,
-                  OAuthTokenGetter* oauth_token_getter,
-                  LogToServer* log_to_server);
+                  OAuthTokenGetter* oauth_token_getter);
   ~HeartbeatSender() override;
 
   // Sets host offline reason for future heartbeat, and initiates sending a
@@ -133,7 +131,6 @@ class HeartbeatSender final : public SignalStrategy::Listener {
   std::string host_id_;
   SignalStrategy* const signal_strategy_;
   std::unique_ptr<HeartbeatClient> client_;
-  LogToServer* const log_to_server_;
   OAuthTokenGetter* const oauth_token_getter_;
 
   base::OneShotTimer heartbeat_timer_;

remoting/host/heartbeat_sender_unittest.cc

@@ -20,7 +20,6 @@
 #include "remoting/base/fake_oauth_token_getter.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/mock_signaling_tracker.h"
 #include "remoting/signaling/signal_strategy.h"
 #include "remoting/signaling/signaling_address.h"
@@ -56,6 +55,7 @@ constexpr base::TimeDelta kTestHeartbeatDelay =
     base::TimeDelta::FromSeconds(350);
 
 void ValidateHeartbeat(const apis::v1::HeartbeatRequest& request,
+                       bool expected_is_initial_heartbeat = false,
                        const std::string& expected_host_offline_reason = {}) {
   ASSERT_TRUE(request.has_host_version());
   if (expected_host_offline_reason.empty()) {
@@ -65,13 +65,20 @@ void ValidateHeartbeat(const apis::v1::HeartbeatRequest& request,
   }
   ASSERT_EQ(kHostId, request.host_id());
   ASSERT_EQ(kFtlId, request.tachyon_id());
+  ASSERT_TRUE(request.has_host_version());
+  ASSERT_TRUE(request.has_host_os_version());
+  ASSERT_TRUE(request.has_host_os_name());
+  ASSERT_TRUE(request.has_host_cpu_type());
+  ASSERT_EQ(expected_is_initial_heartbeat, request.is_initial_heartbeat());
 }
 
 decltype(auto) DoValidateHeartbeatAndRespondOk(
+    bool expected_is_initial_heartbeat = false,
     const std::string& expected_host_offline_reason = {}) {
   return [=](const apis::v1::HeartbeatRequest& request,
              HeartbeatResponseCallback callback) {
-    ValidateHeartbeat(request, expected_host_offline_reason);
+    ValidateHeartbeat(request, expected_is_initial_heartbeat,
+                      expected_host_offline_reason);
     apis::v1::HeartbeatResponse response;
     response.set_set_interval_seconds(kGoodIntervalSeconds);
     std::move(callback).Run(grpc::Status::OK, response);
@@ -88,7 +95,7 @@ class MockDelegate : public HeartbeatSender::Delegate {
 
 }  // namespace
 
-class HeartbeatSenderTest : public testing::Test, public LogToServer {
+class HeartbeatSenderTest : public testing::Test {
  public:
   HeartbeatSenderTest() {
     signal_strategy_ =
@@ -105,8 +112,7 @@ class HeartbeatSenderTest : public testing::Test, public LogToServer {
     signal_strategy_->Disconnect();
 
     heartbeat_sender_ = std::make_unique<HeartbeatSender>(
-        &mock_delegate_, kHostId, signal_strategy_.get(), &oauth_token_getter_,
-        this);
+        &mock_delegate_, kHostId, signal_strategy_.get(), &oauth_token_getter_);
     auto heartbeat_client = std::make_unique<MockHeartbeatClient>();
     mock_client_ = heartbeat_client.get();
     heartbeat_sender_->client_ = std::move(heartbeat_client);
@@ -144,18 +150,9 @@ class HeartbeatSenderTest : public testing::Test, public LogToServer {
 
   MockDelegate mock_delegate_;
 
-  std::vector<ServerLogEntry> received_log_entries_;
-
-  MockSignalingTracker mock_signaling_tracker_;
+  testing::NiceMock<MockSignalingTracker> mock_signaling_tracker_;
 
  private:
-  // LogToServer interface.
-  void Log(const ServerLogEntry& entry) override {
-    received_log_entries_.push_back(entry);
-  }
-
-  ServerLogEntry::Mode mode() const override { return ServerLogEntry::ME2ME; }
-
   // |heartbeat_sender_| must be deleted before |signal_strategy_|.
   std::unique_ptr<HeartbeatSender> heartbeat_sender_;
 
@@ -165,7 +162,7 @@ class HeartbeatSenderTest : public testing::Test, public LogToServer {
 
 TEST_F(HeartbeatSenderTest, SendHeartbeat) {
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce(DoValidateHeartbeatAndRespondOk());
+      .WillOnce(DoValidateHeartbeatAndRespondOk(true));
 
   EXPECT_CALL(mock_delegate_, OnFirstHeartbeatSuccessful()).Times(1);
 
@@ -187,7 +184,7 @@ TEST_F(HeartbeatSenderTest, SignalingReconnect_NewHeartbeats) {
   base::RunLoop run_loop;
 
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce(DoValidateHeartbeatAndRespondOk())
+      .WillOnce(DoValidateHeartbeatAndRespondOk(true))
       .WillOnce(DoValidateHeartbeatAndRespondOk())
       .WillOnce(DoValidateHeartbeatAndRespondOk());
 
@@ -200,6 +197,20 @@ TEST_F(HeartbeatSenderTest, SignalingReconnect_NewHeartbeats) {
   signal_strategy_->Connect();
 }
 
+TEST_F(HeartbeatSenderTest, Signaling_MultipleHeartbeats) {
+  base::RunLoop run_loop;
+
+  EXPECT_CALL(*mock_client_, Heartbeat(_, _))
+      .WillOnce(DoValidateHeartbeatAndRespondOk(true))
+      .WillOnce(DoValidateHeartbeatAndRespondOk())
+      .WillOnce(DoValidateHeartbeatAndRespondOk());
+
+  EXPECT_CALL(mock_delegate_, OnFirstHeartbeatSuccessful()).Times(1);
+
+  signal_strategy_->Connect();
+  task_environment_.FastForwardBy(kTestHeartbeatDelay * 2);
+}
+
 TEST_F(HeartbeatSenderTest, SetHostOfflineReason) {
   base::MockCallback<base::OnceCallback<void(bool success)>> mock_ack_callback;
   EXPECT_CALL(mock_ack_callback, Run(_)).Times(0);
@@ -210,7 +221,7 @@ TEST_F(HeartbeatSenderTest, SetHostOfflineReason) {
   testing::Mock::VerifyAndClearExpectations(&mock_ack_callback);
 
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce(DoValidateHeartbeatAndRespondOk("test_error"));
+      .WillOnce(DoValidateHeartbeatAndRespondOk(true, "test_error"));
 
   // Callback should run once, when we get response to offline-reason.
   EXPECT_CALL(mock_ack_callback, Run(_)).Times(1);
@@ -219,40 +230,11 @@ TEST_F(HeartbeatSenderTest, SetHostOfflineReason) {
   signal_strategy_->Connect();
 }
 
-TEST_F(HeartbeatSenderTest, HostOsInfoOnFirstHeartbeat) {
-  EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce([](const apis::v1::HeartbeatRequest& request,
-                   HeartbeatResponseCallback callback) {
-        ValidateHeartbeat(request);
-        // First heartbeat has host OS info.
-        EXPECT_TRUE(request.has_host_os_name());
-        EXPECT_TRUE(request.has_host_os_version());
-        apis::v1::HeartbeatResponse response;
-        response.set_set_interval_seconds(kGoodIntervalSeconds);
-        std::move(callback).Run(grpc::Status::OK, response);
-      });
-  EXPECT_CALL(mock_delegate_, OnFirstHeartbeatSuccessful()).Times(1);
-  signal_strategy_->Connect();
-
-  EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce([&](const apis::v1::HeartbeatRequest& request,
-                    HeartbeatResponseCallback callback) {
-        ValidateHeartbeat(request);
-        // Subsequent heartbeat has no host OS info.
-        EXPECT_FALSE(request.has_host_os_name());
-        EXPECT_FALSE(request.has_host_os_version());
-        apis::v1::HeartbeatResponse response;
-        response.set_set_interval_seconds(kGoodIntervalSeconds);
-        std::move(callback).Run(grpc::Status::OK, response);
-      });
-  task_environment_.FastForwardBy(kTestHeartbeatDelay);
-}
-
 TEST_F(HeartbeatSenderTest, UnknownHostId) {
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
       .WillRepeatedly([](const apis::v1::HeartbeatRequest& request,
                          HeartbeatResponseCallback callback) {
-        ValidateHeartbeat(request);
+        ValidateHeartbeat(request, true);
         std::move(callback).Run(
             grpc::Status(grpc::StatusCode::NOT_FOUND, "not found"), {});
       });
@@ -264,17 +246,6 @@ TEST_F(HeartbeatSenderTest, UnknownHostId) {
   task_environment_.FastForwardUntilNoTasksRemain();
 }
 
-TEST_F(HeartbeatSenderTest, SendHeartbeatLogEntryOnHeartbeat) {
-  EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-      .WillOnce(DoValidateHeartbeatAndRespondOk());
-
-  EXPECT_CALL(mock_delegate_, OnFirstHeartbeatSuccessful()).Times(1);
-
-  signal_strategy_->Connect();
-
-  ASSERT_EQ(1u, received_log_entries_.size());
-}
-
 TEST_F(HeartbeatSenderTest, FailedToHeartbeat_Backoff) {
   {
     InSequence sequence;
@@ -283,13 +254,13 @@ TEST_F(HeartbeatSenderTest, FailedToHeartbeat_Backoff) {
         .Times(2)
         .WillRepeatedly([&](const apis::v1::HeartbeatRequest& request,
                             HeartbeatResponseCallback callback) {
-          ValidateHeartbeat(request);
+          ValidateHeartbeat(request, true);
           std::move(callback).Run(
               grpc::Status(grpc::StatusCode::UNAVAILABLE, "unavailable"), {});
         });
 
     EXPECT_CALL(*mock_client_, Heartbeat(_, _))
-        .WillOnce(DoValidateHeartbeatAndRespondOk());
+        .WillOnce(DoValidateHeartbeatAndRespondOk(true));
   }
 
   ASSERT_EQ(0, GetBackoff().failure_count());
@@ -306,7 +277,7 @@ TEST_F(HeartbeatSenderTest, Unauthenticated) {
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
       .WillRepeatedly([&](const apis::v1::HeartbeatRequest& request,
                           HeartbeatResponseCallback callback) {
-        ValidateHeartbeat(request);
+        ValidateHeartbeat(request, true);
         heartbeat_count++;
         std::move(callback).Run(
             grpc::Status(grpc::StatusCode::UNAUTHENTICATED, "unauthenticated"),
@@ -326,7 +297,7 @@ TEST_F(HeartbeatSenderTest, RemoteCommand) {
   EXPECT_CALL(*mock_client_, Heartbeat(_, _))
       .WillOnce([](const apis::v1::HeartbeatRequest& request,
                    HeartbeatResponseCallback callback) {
-        ValidateHeartbeat(request);
+        ValidateHeartbeat(request, true);
         apis::v1::HeartbeatResponse response;
         response.set_set_interval_seconds(kGoodIntervalSeconds);
         response.set_remote_command(apis::v1::HeartbeatResponse::RESTART_HOST);

remoting/host/remoting_me2me_host.cc

@@ -407,7 +407,7 @@ class HostProcess : public ConfigWatcher::Delegate,
   // Must outlive |signal_strategy_| and |ftl_signaling_connector_|.
   std::unique_ptr<OAuthTokenGetterImpl> oauth_token_getter_;
 
-  // Must outlive |heartbeat_sender_| and |host_status_logger_|.
+  // Must outlive |host_status_logger_|.
   std::unique_ptr<LogToServer> log_to_server_;
 
   // Signal strategies must outlive |ftl_signaling_connector_|.
@@ -1439,8 +1439,7 @@ void HostProcess::InitializeSignaling() {
       base::BindOnce(&HostProcess::OnAuthFailed, base::Unretained(this)));
   ftl_signaling_connector_->Start();
   heartbeat_sender_ = std::make_unique<HeartbeatSender>(
-      this, host_id_, ftl_signal_strategy.get(), oauth_token_getter_.get(),
-      log_to_server_.get());
+      this, host_id_, ftl_signal_strategy.get(), oauth_token_getter_.get());
   signal_strategy_ = std::move(ftl_signal_strategy);
 }
 

remoting/host/server_log_entry_host.cc

@@ -12,7 +12,6 @@ namespace remoting {
 
 namespace {
 const char kValueEventNameSessionState[] = "session-state";
-const char kValueEventNameHeartbeat[] = "heartbeat";
 
 const char kValueRoleHost[] = "host";
 
@@ -42,13 +41,6 @@ std::unique_ptr<ServerLogEntry> MakeLogEntryForSessionStateChange(
   return entry;
 }
 
-std::unique_ptr<ServerLogEntry> MakeLogEntryForHeartbeat() {
-  std::unique_ptr<ServerLogEntry> entry(new ServerLogEntry());
-  entry->AddRoleField(kValueRoleHost);
-  entry->AddEventNameField(kValueEventNameHeartbeat);
-  return entry;
-}
-
 void AddHostFieldsToLogEntry(ServerLogEntry* entry) {
   // TODO os name, os version, and version will be in the main message body,
   // remove these fields at a later date to remove redundancy.

remoting/host/server_log_entry_host.h

@@ -16,9 +16,6 @@ class ServerLogEntry;
 std::unique_ptr<ServerLogEntry> MakeLogEntryForSessionStateChange(
     bool connected);
 
-// Constructs a log entry for a heartbeat.
-std::unique_ptr<ServerLogEntry> MakeLogEntryForHeartbeat();
-
 // Adds fields describing the host to this log entry.
 void AddHostFieldsToLogEntry(ServerLogEntry* entry);
 

remoting/host/server_log_entry_host_unittest.cc

@@ -32,18 +32,6 @@ TEST(ServerLogEntryHostTest, MakeForSessionStateChange) {
       << error;
 }
 
-TEST(ServerLogEntryHostTest, MakeForHeartbeat) {
-  std::unique_ptr<ServerLogEntry> entry(MakeLogEntryForHeartbeat());
-  std::unique_ptr<XmlElement> stanza = entry->ToStanza();
-  std::string error;
-  std::map<std::string, std::string> key_value_pairs;
-  key_value_pairs["role"] = "host";
-  key_value_pairs["event-name"] = "heartbeat";
-  std::set<std::string> keys;
-  ASSERT_TRUE(VerifyStanza(key_value_pairs, keys, stanza.get(), &error))
-      << error;
-}
-
 TEST(ServerLogEntryHostTest, AddHostFields) {
   std::unique_ptr<ServerLogEntry> entry(
       MakeLogEntryForSessionStateChange(true));

remoting/proto/remoting/v1/directory_messages.proto

@@ -68,6 +68,12 @@ message HeartbeatRequest {
 
   // Operating system version the host is running on.
   optional string host_os_version = 9;
+
+  // CPU type of the host machine (e.g. x86 or ARM).
+  optional string host_cpu_type = 10;
+
+  // Indicates whether this request is the first heartbeat from a host instance.
+  optional bool is_initial_heartbeat = 11;
 }
 
 // Sent in response to a HeartbeatRequest.

remoting/signaling/mock_signaling_tracker.h

@@ -10,7 +10,7 @@
 
 namespace remoting {
 
-class MockSignalingTracker final : public SignalingTracker {
+class MockSignalingTracker : public SignalingTracker {
  public:
   MockSignalingTracker();
   ~MockSignalingTracker() override;
@@ -22,4 +22,4 @@ class MockSignalingTracker final : public SignalingTracker {
 
 }  // namespace remoting
 
-#endif  // REMOTING_SIGNALING_MOCK_SIGNALING_TRACKER_H_
+#endif  // REMOTING_SIGNALING_MOCK_SIGNALING_TRACKER_H_
\ No newline at end of file