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
--- 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;
}