Bug 1370221 - Don't try to serialize ExtendedJSONObject r=nalexander
MozReview-Commit-ID: 3Q4rD2Ljfc
--- a/mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/pingbuilders/TelemetrySyncPingBuilder.java
@@ -6,16 +6,17 @@
package org.mozilla.gecko.telemetry.pingbuilders;
import android.os.Bundle;
import android.os.Parcelable;
import android.support.annotation.NonNull;
import org.json.simple.JSONArray;
+import org.json.simple.JSONObject;
import org.mozilla.gecko.sync.ExtendedJSONObject;
import org.mozilla.gecko.sync.telemetry.TelemetryContract;
import org.mozilla.gecko.sync.telemetry.TelemetryStageCollector;
import org.mozilla.gecko.telemetry.TelemetryLocalPing;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.HashMap;
@@ -126,17 +127,17 @@ public class TelemetrySyncPingBuilder ex
if (devicesJSON.size() > 0) {
payload.put("devices", devicesJSON);
}
return this;
}
public TelemetrySyncPingBuilder setError(@NonNull Serializable error) {
- payload.put("failureReason", castErrorObject(error));
+ payload.put("failureReason", new ExtendedJSONObject((JSONObject) error));
return this;
}
public TelemetrySyncPingBuilder setTook(long took) {
payload.put("took", took);
return this;
}
@@ -154,13 +155,9 @@ public class TelemetrySyncPingBuilder ex
/**
* We broadcast this data via LocalBroadcastManager and control both sides of this code, so it
* is acceptable to do an unchecked cast.
*/
@SuppressWarnings("unchecked")
private static HashMap<String, TelemetryStageCollector> castSyncData(final Serializable data) {
return (HashMap<String, TelemetryStageCollector>) data;
}
-
- private static ExtendedJSONObject castErrorObject(final Serializable error) {
- return (ExtendedJSONObject) error;
- }
}
--- 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
@@ -2,30 +2,28 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.gecko.sync.telemetry;
import android.os.Bundle;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
-import android.support.annotation.VisibleForTesting;
import android.util.Log;
import org.mozilla.gecko.sync.CollectionConcurrentModificationException;
import org.mozilla.gecko.sync.ExtendedJSONObject;
import org.mozilla.gecko.sync.HTTPFailureException;
import org.mozilla.gecko.sync.SyncDeadlineReachedException;
import org.mozilla.gecko.sync.Utils;
import org.mozilla.gecko.sync.net.SyncStorageResponse;
import org.mozilla.gecko.sync.repositories.FetchFailedException;
import org.mozilla.gecko.sync.repositories.StoreFailedException;
import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
-import java.io.Serializable;
import java.io.UnsupportedEncodingException;
import java.security.NoSuchAlgorithmException;
import java.util.ArrayList;
import java.util.HashMap;
/**
* Gathers telemetry about a single run of sync.
* In light of sync restarts, rarely a "single sync" will actually include more than one sync.
@@ -41,17 +39,17 @@ public class TelemetryCollector {
// 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.
// 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 ExtendedJSONObject error;
private volatile String hashedUID;
private volatile String hashedDeviceID;
private final ArrayList<Bundle> devices = new ArrayList<>();
@Nullable private volatile Long started;
@Nullable private volatile Long finished;
private volatile boolean didRestart = false;
@@ -148,17 +146,19 @@ public class TelemetryCollector {
final long took = this.finished - this.started;
final Bundle telemetry = new Bundle();
telemetry.putString(TelemetryContract.KEY_LOCAL_UID, this.hashedUID);
telemetry.putString(TelemetryContract.KEY_LOCAL_DEVICE_ID, this.hashedDeviceID);
telemetry.putParcelableArrayList(TelemetryContract.KEY_DEVICES, this.devices);
telemetry.putLong(TelemetryContract.KEY_TOOK, took);
- telemetry.putSerializable(TelemetryContract.KEY_ERROR, (Serializable) this.error);
+ if (this.error != null) {
+ telemetry.putSerializable(TelemetryContract.KEY_ERROR, this.error.object);
+ }
telemetry.putSerializable(TelemetryContract.KEY_STAGES, this.stageCollectors);
if (this.didRestart) {
telemetry.putBoolean(TelemetryContract.KEY_RESTARTED, true);
}
return telemetry;
}
/**
--- a/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
+++ b/mobile/android/tests/background/junit4/src/org/mozilla/gecko/sync/telemetry/TelemetryCollectorTest.java
@@ -1,30 +1,32 @@
/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */
package org.mozilla.gecko.sync.telemetry;
import android.os.Bundle;
+import org.json.simple.JSONObject;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mozilla.gecko.background.testhelpers.TestRunner;
import org.mozilla.gecko.sync.CollectionConcurrentModificationException;
import org.mozilla.gecko.sync.ExtendedJSONObject;
import org.mozilla.gecko.sync.HTTPFailureException;
import org.mozilla.gecko.sync.SyncDeadlineReachedException;
import org.mozilla.gecko.sync.Utils;
import org.mozilla.gecko.sync.net.SyncStorageResponse;
import org.mozilla.gecko.sync.repositories.FetchFailedException;
import org.mozilla.gecko.sync.repositories.StoreFailedException;
import org.mozilla.gecko.sync.repositories.domain.ClientRecord;
import java.util.ArrayList;
+import java.util.ConcurrentModificationException;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.doReturn;
import static org.junit.Assert.*;
@RunWith(TestRunner.class)
public class TelemetryCollectorTest {
@@ -126,19 +128,57 @@ public class TelemetryCollectorTest {
@Test(expected = IllegalStateException.class)
public void testDurationMissingFinished() throws Exception {
collector.setStarted(10L);
collector.build();
}
@Test
public void testError() throws Exception {
+ collector.setStarted(1L);
+ collector.setFinished(2L);
+
+ // Test that we can build without setting an error.
+ assertFalse(collector.hasError());
+ Bundle data = collector.build();
+ assertFalse(data.containsKey("error"));
+
+ // Test various ways to set an error.
+ // Just details.
collector.setError("testError", "unexpectedStuff");
- assertEquals("testError", collector.error.getString("name"));
- assertEquals("unexpectedStuff", collector.error.getString("error"));
+ data = collector.build();
+ assertTrue(data.containsKey("error"));
+ JSONObject errorJson = (JSONObject) data.getSerializable("error");
+ assertEquals("testError", errorJson.get("name"));
+ assertEquals("unexpectedStuff", errorJson.get("error"));
+ assertTrue(collector.hasError());
+
+ // Just exception.
+ collector.setError("exceptionTest", new IllegalArgumentException());
+ data = collector.build();
+ assertTrue(data.containsKey("error"));
+ errorJson = (JSONObject) data.getSerializable("error");
+ assertEquals("exceptionTest", errorJson.get("name"));
+ assertEquals("IllegalArgumentException", errorJson.get("error"));
+
+ // Details and exception.
+ collector.setError("anotherTest", "Error details", new ConcurrentModificationException());
+ data = collector.build();
+ assertTrue(data.containsKey("error"));
+ errorJson = (JSONObject) data.getSerializable("error");
+ assertEquals("anotherTest", errorJson.get("name"));
+ assertEquals("ConcurrentModificationException:Error details", errorJson.get("error"));
+
+ // Details and explicit null exception.
+ collector.setError("noExceptionTest", "Error details", null);
+ data = collector.build();
+ assertTrue(data.containsKey("error"));
+ errorJson = (JSONObject) data.getSerializable("error");
+ assertEquals("noExceptionTest", errorJson.get("name"));
+ assertEquals("Error details", errorJson.get("error"));
}
@Test
public void testRestarted() throws Exception {
collector.setStarted(5L);
collector.setFinished(10L);
Bundle data = collector.build();