[Sync] Store the past 10 traffic records in memory.

sync/engine/traffic_logger.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.
 
 // This file has the functions to log all the sync related HTTP communication.
 // To get the log run a debug build of chrome with the flag
 // --vmodule=traffic_logger=1.
 
 #ifndef CHROME_BROWSER_SYNC_ENGINE_TRAFFIC_LOGGER_H_
 #define CHROME_BROWSER_SYNC_ENGINE_TRAFFIC_LOGGER_H_
 #pragma once
 
+#include <string>
+
 namespace sync_pb {
 class ClientToServerResponse;
 class ClientToServerMessage;
 }  // namespace sync_pb
 
 namespace browser_sync {
 
-void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg);
+namespace sessions {
+class SyncSession;
+}  // namespace sessions
+
+enum TrafficMessageType {
+  CLIENT_TO_SERVER_MESSAGE,
+  CLIENT_TO_SERVER_RESPONSE
+};
+
+struct TrafficRecord {
+  // The serialized message.
+  std::string message;
+  TrafficMessageType message_type;
+  // If the message is too big to be kept in memory then it would be trucated.
+  // For now the entire message would be truncated if it is big.
+  // TODO(lipalani): Truncate the specifics to fit with in size.
+  bool truncated;
+ public:

[akalin, 2012/03/22 20:27:28]

no 'public'; everything in a struct is public by default

[lipalani1, 2012/03/23 00:03:11]

Done.

+  TrafficRecord(const std::string& message,
+                TrafficMessageType message_type,
+                bool truncated);

[akalin, 2012/03/22 20:27:28]

you should also define the default constructor (and initialize
message_type/truncated)

[lipalani1, 2012/03/23 00:03:11]

Done.

+  ~TrafficRecord();
+};
+
+void LogClientToServerMessage(const sync_pb::ClientToServerMessage& msg,

[akalin, 2012/03/22 20:27:28]

hmm I don't think the storing logic belongs in this class.  Since the queue is
in the context object, why not add methods to it like
'RecordClientToServer{Message,Response}()'?  Then either the caller can do:

RecordClientToServerMessage(msg);
LogClientToServerMessage(msg);

or just have RecordClientToServerMessage call LogClientToServerMessage (my
preference).

[lipalani1, 2012/03/23 00:03:11]

The context object for the most part has only getters. Take the examples of
resolver(), directory(), registrar(), extensions_monitor() etc. It might make it
cumbersome to add member methods here for each operation.

One option that I can do is make the datastructure in context to be a
TrafficLogger class which can have a public method called LogData which in turn
calls private methods to log data locally and to memory.

Let me know if you prefer that. What we have currently are global functions
which (in my personal opinion) is sufficient enough for this light use.
On 2012/03/22 20:27:28, akalin wrote:

[akalin, 2012/03/23 00:58:50]

I think the problem is that you take 'log' to mean 'log and record', and I take
it to mean just 'log'.  I think it would be better to separate out the two
concepts.

A separate class would be best, actually, as it would encapsulate all the logic,
instead of having it spread out across other files.  Perhaps TrafficRecorder?

[lipalani1, 2012/03/26 21:25:21]

Done.

[akalin, 2012/03/27 18:18:45]

I meant a separate class for *just* storing/recording the traffic.  You haven't
addressed my main concern, which is to separate out the two concepts.

Basically, you should leave traffic_logger.{h,cc} as is (since otherwise you'd
have to change the switch), and have traffic_recorder.{h,cc} store/record the
traffic and do nothing else.  The caller can just call into the recorder and
logger at the same time.

+                              sessions::SyncSession* session);
 void LogClientToServerResponse(
-    const sync_pb::ClientToServerResponse& response);
+    const sync_pb::ClientToServerResponse& response,
+    sessions::SyncSession* session);
 
 }  // namespace browser_sync
 
 #endif  // CHROME_BROWSER_SYNC_ENGINE_TRAFFIC_LOGGER_H_