Bug 1370111 - Post: More explicit global error handling for Sync Telemetry r=nalexander
authorGrigory Kruglov <gkruglov@mozilla.com>
Mon, 05 Jun 2017 19:44:15 -0400
changeset 410881 12af2c27c2b2bb6dd55dea06f0364b564b27e26a
parent 410880 84818c89ef9ce8734678e9f099025da3e394de18
child 410882 d0254e59eb1078f06d7ebb19d7751525e3bd56a9
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs1370111
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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;
     }