[Sync] Display the client server traffic log in about:sync.

sync/engine/traffic_recorder.cc

Index: sync/engine/traffic_recorder.cc
diff --git a/sync/engine/traffic_recorder.cc b/sync/engine/traffic_recorder.cc
index a92412e06ed0931b3f140c81f7e621c19f6dbaf3..2bdcac7f75ed7c66f6880f3e79e20bceca9560fe 100644
--- a/sync/engine/traffic_recorder.cc
+++ b/sync/engine/traffic_recorder.cc
@@ -7,6 +7,7 @@
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/stringprintf.h"
 #include "base/values.h"
 #include "sync/protocol/proto_value_conversions.h"
 #include "sync/protocol/sync.pb.h"
@@ -39,6 +40,63 @@ TrafficRecorder::TrafficRecorder(unsigned int max_messages,
 TrafficRecorder::~TrafficRecorder() {
 }
 
+namespace {
+#define ENUM_CASE(x) case TrafficRecorder::##x: return #x; break;
+
+const char* GetMessageTypeString(TrafficRecorder::TrafficMessageType type) {

[rlarocque, 2012/03/29 22:54:27]

I'd like to take this opportunity to hide some unusual terminology.  Could we
translate these as either {"To client","To server"} or {"Request","Response"}? 
I think the current naming, where both types are called "CLIENT_TO_SERVER" is
confusing.  

While there is a cost to not having the enums match the strings exactly, in this
case the enums exist only inside the TrafficRecorder so I don't feel too bad
about it.  

[lipalani1, 2012/03/29 23:58:37]

Done.

+  switch(type) {
+    ENUM_CASE(CLIENT_TO_SERVER_MESSAGE)
+    ENUM_CASE(CLIENT_TO_SERVER_RESPONSE)
+  }
+  NOTREACHED();
+  return "";
+}
+
+#undef ENUM_CASE
+}
+
+DictionaryValue* TrafficRecorder::TrafficRecord::ToValue() const {
+  scoped_ptr<DictionaryValue> value;
+  if (truncated) {
+    value.reset(new DictionaryValue());
+    value->SetString("message_type",
+                               GetMessageTypeString(message_type));
+    value->SetBoolean("truncated", true);
+  } else if (message_type == TrafficRecorder::CLIENT_TO_SERVER_MESSAGE) {
+    sync_pb::ClientToServerMessage message_proto;
+    if (message_proto.ParseFromString(message))

[rlarocque, 2012/03/30 00:20:51]

By the way, do we really need to round-trip the message protos through a
serialization to string and back?  Why do we serialize the message only to
ParseFromString() later on?

[akalin, 2012/03/30 01:55:04]

Yeah, I wanted it to be stored serialized to save memory (since this is always
logged, regardless of whether about:sync is open).  We already bound memory
usage, but storing it serialized will enable storing bigger messages.

[lipalani1, 2012/03/30 20:35:16]

We had a discussion about the best format for storing the message(as the
protobuf struct, string or dictionary). string gave the most bang for the buck
because it was very compressed(probably because it is the same text that is sent
across the wire) and serializing is not too heavy too. I am not too sure about
deserializing but in that case performance delay is OK.
On 2012/03/30 00:20:51, rlarocque wrote:

[rlarocque, 2012/03/30 21:01:11]

I'm not entirely convinced that this will be a significant performance win (when
compared against a more straightforward solution), but I do agree that it will
do little or no harm.  That's good enough for me.  

+      value.reset(
+          ClientToServerMessageToValue(message_proto,

[rlarocque, 2012/03/29 22:58:49]

Because the JavaScript isn't processing this in any way, would it be easier to
expose this as one big string?  Our serialization format doesn't necessarily
have to be JSON.

[lipalani1, 2012/03/29 23:58:37]

JS converts this into a formatted string using JSON. this is the convention so
far. Also this makes it easier to wiring up the function to sync manager
etc(which expects dictionary)
On 2012/03/29 22:58:49, rlarocque wrote:

[rlarocque, 2012/03/30 00:20:51]

Right, but that dictionary could consist solely of { "value" : <custom
serialization traffic logs> }.  We can serialize and format this however we want
from C++, rather than trying to stuff this information into a hierarchy of Value
objects.

Besides, I thought this function was only called from
TrafficRecorder::ToValue()?  I realize you need to return a dictionary
eventually, but I don't think that constraint applies here.  

The current approach gets the job done, so I'm not against it.  I'm just
pointing out that there might be other options.  An automatically generated
protobuf-to-string method, for example, would probably be easier to maintain. 
Though I think we lost that option by using the "lite" version of protobufs.

[akalin, 2012/03/30 01:55:04]

I'm not sure I follow; why would serializing to a string be better than
(essentially) the current JSON format?  Ideally, this would be displayed in JS
as something other than a raw string dump (a tree, etc.), and keeping the JSON
format would avoid the need for the JS code to try and parse the string to split
it up, etc.

[rlarocque, 2012/03/30 17:49:02]

I don't mean to argue that it's necessarily better, only to point out that it
should be considered.

Serializing to JSON is only useful if we intend to slice and dice this data in
JavaScript.

The drawback to converting everything to JSON/Value* is that we have to maintain
the conversion functions.  It's going to be hard to keep
ClientToServerMessageToValue up to date.  I was hoping protobufs could help us
with this, but I'm not sure that's possible, due to other technical limitations.

Using JSON also makes it harder to use C++ code to process this data before
presenting it.  Though it seems unlikely that we'll be doing much formatting in
any language, so I'm not too concerned about that at this point.

+                                       false /* include_specifics */));
+  } else if (message_type == TrafficRecorder::CLIENT_TO_SERVER_RESPONSE) {
+    sync_pb::ClientToServerResponse message_proto;
+    if (message_proto.ParseFromString(message))
+      value.reset(
+          ClientToServerResponseToValue(message_proto,
+                                        false /* include_specifics */));
+  } else {
+    NOTREACHED();
+  }
+
+  return value.release();
+}
+
+
+DictionaryValue* TrafficRecorder::ToValue() const {
+  int message_count = 0;
+  scoped_ptr<DictionaryValue> dictionary(new DictionaryValue());
+  std::deque<TrafficRecord>::const_iterator it;
+  for (it = records_.begin(); it != records_.end(); ++it) {
+    const TrafficRecord& record = *it;
+    std::string message_id = StringPrintf("message%d", message_count);

[rlarocque, 2012/03/29 22:54:27]

Couldn't this be an array instead?  I don't see the point of enumerating each
message with a string key.

[lipalani1, 2012/03/29 23:58:37]

We have to put this in a dictionary. Having it as message1, message2 etc in
about:sync page looked to me. let me know if you have some other idea.
On 2012/03/29 22:54:27, rlarocque wrote:

[rlarocque, 2012/03/30 00:20:51]

Same idea as above.  A single-attribute dictionary will let you return anything
you want.  I'd prefer that form of dictionary abuse rather than generating keys
to pretend it's an array.  At least that way you can still grab the array and
iterate over it without having to do strange things like construct the set of
keys.  

Cleaner solutions, like modifying the SyncManager to not necessarily require a
dictionary, would be acceptable, too.

[akalin, 2012/03/30 01:55:04]

See my comment re. using ListValue for this.  The returned dictionary value
should look something like:

{
  trafficRecords: [
    { (traffic record 0) },
    { (traffic record 1) },
    ....
  ]
}

+    dictionary->Set(message_id, record.ToValue());
+    ++message_count;
+  }
+
+  return dictionary.release();
+}
+
+
 
 void TrafficRecorder::AddTrafficToQueue(TrafficRecord* record) {
   records_.resize(records_.size() + 1);
@@ -76,6 +134,5 @@ void TrafficRecorder::RecordClientToServerResponse(
     const sync_pb::ClientToServerResponse& response) {
   StoreProtoInQueue(response, CLIENT_TO_SERVER_RESPONSE);
 }
-
 }  // namespace browser_sync