Bug 1370111 - Post: More explicit global error handling for Sync Telemetry r=nalexander draft
authorGrigory Kruglov <gkruglov@mozilla.com>
Mon, 05 Jun 2017 19:44:15 -0400
changeset 589711 da73247ae0a27ba6ae3d6ad0d8814c1e2249e722
parent 589710 fdf22f1cdc2a5eba3ada1339aeefcc23c2debfe7
child 589740 2c80281949da72e8512bc777e381bc19abc2f452
child 589876 07c90270e1481408e1a5cdef0a1dc3731461465d
push id62477
push userbmo:gkruglov@mozilla.com
push dateTue, 06 Jun 2017 17:26:27 +0000
reviewersnalexander
bugs1370111
milestone55.0a1
Bug 1370111 - Post: More explicit global error handling for Sync Telemetry r=nalexander While this patch does make it clearer that telemetry error handling could be factored better, at least it gets us to a consistent usage pattern. MozReview-Commit-ID: 4Oamt9D03Ue
mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/sync/FxAccountSyncAdapter.java
@@ -167,29 +167,29 @@ public class FxAccountSyncAdapter extend
     public void handleSuccess(GlobalSession globalSession) {
       super.handleSuccess(globalSession);
       recordTelemetry();
     }
 
     @Override
     public void handleError(GlobalSession globalSession, Exception ex, String reason) {
       super.handleError(globalSession, ex, reason);
-      this.telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, reason);
+      // If an error hasn't been set downstream, record what we know at this point.
+      if (!telemetryCollector.hasError()) {
+        telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, reason, ex);
+      }
       recordTelemetry();
     }
 
     @Override
-    public void handleError(GlobalSession globalSession, Exception e) {
-      super.handleError(globalSession, e);
-      if (e instanceof TokenServerException) {
-        this.telemetryCollector.setError(
-                TelemetryCollector.KEY_ERROR_TOKEN, e.getClass().getSimpleName());
-      } else {
-        this.telemetryCollector.setError(
-                TelemetryCollector.KEY_ERROR_INTERNAL, e.getClass().getSimpleName());
+    public void handleError(GlobalSession globalSession, Exception ex) {
+      super.handleError(globalSession, ex);
+      // If an error hasn't been set downstream, record what we know at this point.
+      if (!telemetryCollector.hasError()) {
+        telemetryCollector.setError(TelemetryCollector.KEY_ERROR_INTERNAL, ex);
       }
       recordTelemetry();
     }
 
     @Override
     public void handleAborted(GlobalSession globalSession, String reason) {
       super.handleAborted(globalSession, reason);
       // Note to future maintainers: while there are reasons, other than 'backoff', this method
@@ -439,23 +439,31 @@ public class FxAccountSyncAdapter extend
           State state = fxAccount.getState();
           if (state.getStateLabel() == StateLabel.Married) {
             Married married = (Married) state;
             fxAccount.setState(married.makeCohabitingState());
           }
         } finally {
           fxAccount.releaseSharedAccountStateLock();
         }
+        telemetryCollector.setError(
+                TelemetryCollector.KEY_ERROR_TOKEN,
+                e.getClass().getSimpleName()
+        );
         callback.handleError(null, e);
       }
 
       @Override
       public void handleError(Exception e) {
         Logger.error(LOG_TAG, "Failed to get token.", e);
         fxAccount.releaseSharedAccountStateLock();
+        telemetryCollector.setError(
+                TelemetryCollector.KEY_ERROR_TOKEN,
+                e.getClass().getSimpleName()
+        );
         callback.handleError(null, e);
       }
 
       @Override
       public void handleBackoff(int backoffSeconds) {
         // This is the token server telling us to back off.
         Logger.info(LOG_TAG, "Token server requesting backoff of " + backoffSeconds + "s. Backoff handler: " + tokenBackoffHandler);
         didReceiveBackoff = true;
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/telemetry/TelemetryCollector.java
@@ -39,25 +39,27 @@ public class TelemetryCollector {
     public static final String KEY_ERROR_INTERNAL = "internal";
     public static final String KEY_ERROR_TOKEN = "token";
 
     // Telemetry collected by individual stages is aggregated here. Stages run sequentially,
     // and only access their own collectors.
     private final HashMap<String, TelemetryStageCollector> stageCollectors = new HashMap<>();
 
     // Data which is not specific to a single stage is aggregated in this object.
-    @VisibleForTesting protected ExtendedJSONObject error;
-    private String hashedUID;
-    private String hashedDeviceID;
+    // It's possible that these fields are read/written from different threads.
+    // Volatile is used to ensure memory visibility.
+    @VisibleForTesting protected volatile ExtendedJSONObject error;
+    private volatile String hashedUID;
+    private volatile String hashedDeviceID;
     private final ArrayList<Bundle> devices = new ArrayList<>();
 
-    @Nullable private Long started;
-    @Nullable private Long finished;
+    @Nullable private volatile Long started;
+    @Nullable private volatile Long finished;
 
-    private boolean didRestart = false;
+    private volatile boolean didRestart = false;
 
     public TelemetryStageCollector collectorFor(@NonNull String stageName) {
         if (stageCollectors.containsKey(stageName)) {
             return stageCollectors.get(stageName);
         }
 
         final TelemetryStageCollector collector = new TelemetryStageCollector(this);
         stageCollectors.put(stageName, collector);
@@ -76,23 +78,39 @@ public class TelemetryCollector {
                             deviceID.concat(uid).getBytes("UTF-8")
                     ));
         } catch (UnsupportedEncodingException | NoSuchAlgorithmException e) {
             // Should not happen.
             Log.e(LOG_TAG, "Either UTF-8 or SHA-256 are not supported", e);
         }
     }
 
+    public void setError(@NonNull String name, @NonNull Exception e) {
+        setError(name, e.getClass().getSimpleName());
+    }
+
     public void setError(@NonNull String name, @NonNull String details) {
+        setError(name, details, null);
+    }
+
+    public void setError(@NonNull String name, @NonNull String details, @Nullable Exception e) {
         final ExtendedJSONObject error = new ExtendedJSONObject();
         error.put("name", name);
-        error.put("error", details);
+        if (e != null) {
+            error.put("error", e.getClass().getSimpleName() + ":" + details);
+        } else {
+            error.put("error", details);
+        }
         this.error = error;
     }
 
+    public boolean hasError() {
+        return this.error != null;
+    }
+
     public void setStarted(long time) {
         this.started = time;
     }
 
     public void setFinished(long time) {
         this.finished = time;
     }