Remove broken variables from sync's AllStatus

chrome/browser/sync/protocol/client_debug_info.proto

Index: chrome/browser/sync/protocol/client_debug_info.proto
diff --git a/chrome/browser/sync/protocol/client_debug_info.proto b/chrome/browser/sync/protocol/client_debug_info.proto
index 55ca4f643fbfeb08a17327c24b83f298ff8de5d9..6201077bf2410edf83a33ab86ad9ba2df5c8dd4e 100644
--- a/chrome/browser/sync/protocol/client_debug_info.proto
+++ b/chrome/browser/sync/protocol/client_debug_info.proto
@@ -14,7 +14,7 @@ package sync_pb;
 // The additional info here is from SyncerStatus. They get sent when the event
 // SYNC_CYCLE_COMPLETED  is sent.
 message SyncCycleCompletedEventInfo {
-  optional bool syncer_stuck = 1;
+  // optional bool syncer_stuck = 1; // Was always false, now obsolete.

[rlarocque, 2012/01/10 00:07:43]

This is my preferred mechanism for deprecating protobuf fields.  If you know of
a better way to handle it, let me know.

[tim (not reviewing), 2012/01/10 17:01:47]

I really wonder if the spirit of a "stuck" signal is obsolete.  We were
mis-reporting it, but it's not like we haven't had reports of "stuck" like
behavior.  Are you absolutely certain it would never be useful to send this as
debug info to the server?  I'm not.  The cost of an extra .proto field isn't
that great.  We could add the deprecated=true option (does nothing in C++ but
generates @Deprecated annotation + warnings in java), I'm not sure if that is
more or less reversible than deleting altogether... perhaps Raz or Albert can
comment.

[rlarocque, 2012/01/10 21:39:32]

Even if we revive the concept of "syncer_stuck" (I hope we don't), it will be
based on significantly different logic than the current variable.  They should
be named differently, so we don't have to research whether or not the flag is
broken in a particular version of Chrome.

[tim (not reviewing), 2012/01/11 02:15:35]

It's just that we have made no change that ensures a client will never become
"stuck". And we only added this bit here a few months ago and I don't think
we've used it a single time yet (thankfully we haven't had any major prod issues
to debug!), so I don't think there is much legacy convention to worry about. And
lastly, hopes and wishes aside, I think it's safe to say this concept will
always be around, unless we figure out how to achieve complete perfection even
in the face of cosmic rays :)

But anyway, I'm okay with removing it.  I'd be interested to hear Lingesh's
thoughts (do we want a bug to track sending up something more useful?)

[lipalani1, 2012/01/12 23:55:49]

I guess since this is a brand new feature there is not much to worry about.
Since this field is not calculated I am OK deprecating it. and when we have the
new way to calculate the stuck behaviour we could simply use a new field without
worrying about legacy.

As far as the meta question goes: I too think stuck is a useful state to track.
How about we upload 'x', the number of  sync cycles that failed consecutively up
to this point. we track that and upload the x? Which in most cases should be 0
or 1.
If we upload this then from the server side we could do about:sync page
displaying clients with high numbers for x and do some digging there.
On 2012/01/11 02:15:35, timsteele wrote:

[albertb, 2012/01/13 15:47:03]

[deprecated=true] is fine for the server. We'll get a warning if we reference
the field.

   optional int32 num_blocking_conflicts = 2;
   optional int32 num_non_blocking_conflicts = 3;
 }
@@ -55,4 +55,4 @@ message DebugInfo {
 
   // Indicates client has dropped some events to save bandwidth.
   optional bool events_dropped = 4;
-}
+}