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

sync/engine/traffic_logger.h

Index: sync/engine/traffic_logger.h
diff --git a/sync/engine/traffic_logger.h b/sync/engine/traffic_logger.h
index 5cb8ffa81b4a3abec1d301d1522ab4bbd921e315..6e98d28411b3c04576e2c56613b92bd17984a1ed 100644
--- a/sync/engine/traffic_logger.h
+++ b/sync/engine/traffic_logger.h
@@ -10,6 +10,8 @@
 #define CHROME_BROWSER_SYNC_ENGINE_TRAFFIC_LOGGER_H_
 #pragma once
 
+#include <string>
+
 namespace sync_pb {
 class ClientToServerResponse;
 class ClientToServerMessage;
@@ -17,9 +19,35 @@ class ClientToServerMessage;
 
 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