mirror of
https://github.com/mozilla/gecko-dev.git
synced 2024-10-17 23:35:34 +00:00
Bug 1243585 - Add JSONFilePingStore with tests. r=sebastian
Note: not expected to compile. MozReview-Commit-ID: 5RTQk5m3zTx --HG-- extra : rebase_source : 1d5aa98b5c54fe6b213fe88bd91e13b5c334a7fb
This commit is contained in:
Michael Comella
parent
0959e38680
commit
daf1884121
@ -0,0 +1,279 @@
|
||||
/*
|
||||
* This Source Code Form is subject to the terms of the Mozilla Public
|
||||
* 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.telemetry.stores;
|
||||
|
||||
import android.os.Parcel;
|
||||
import android.os.Parcelable;
|
||||
import android.support.annotation.VisibleForTesting;
|
||||
import android.util.Log;
|
||||
import org.json.JSONException;
|
||||
import org.json.JSONObject;
|
||||
import org.mozilla.gecko.sync.ExtendedJSONObject;
|
||||
import org.mozilla.gecko.sync.NonObjectJSONException;
|
||||
import org.mozilla.gecko.telemetry.TelemetryPing;
|
||||
import org.mozilla.gecko.telemetry.TelemetryPingFromStore;
|
||||
import org.mozilla.gecko.util.FileUtils;
|
||||
|
||||
import java.io.File;
|
||||
import java.io.FileInputStream;
|
||||
import java.io.FileNotFoundException;
|
||||
import java.io.FileOutputStream;
|
||||
import java.io.FilenameFilter;
|
||||
import java.io.IOException;
|
||||
import java.nio.channels.FileLock;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Iterator;
|
||||
import java.util.Locale;
|
||||
import java.util.Set;
|
||||
import java.util.SortedSet;
|
||||
import java.util.TreeSet;
|
||||
import java.util.regex.Matcher;
|
||||
import java.util.regex.Pattern;
|
||||
|
||||
/**
|
||||
* An implementation of TelemetryPingStore that is backed by JSON files.
|
||||
*
|
||||
* This implementation seeks simplicity. Each ping to upload is stored in its own file with a
|
||||
* name patterned after {@link #FILENAME}. The file name includes the ping's unique id - these are used to
|
||||
* both remove pings and prune pings. During prune, the pings with the smallest ids will be removed first
|
||||
* so these ping ids should be strictly increasing as new pings are stored; consider using data already contained
|
||||
* in the ping, such as a sequence number.
|
||||
*
|
||||
* Using separate files for this store allows for less restrictive concurrency:
|
||||
* * requires locking: {@link #storePing(long, TelemetryPing)} writes a new file
|
||||
* * requires locking: {@link #getAllPings()} reads all files, including those potentially being written,
|
||||
* hence locking
|
||||
* * no locking: {@link #maybePrunePings()} deletes the least recently written pings, none of which should
|
||||
* be currently written
|
||||
* * no locking: {@link #onUploadAttemptComplete(Set)} deletes the given pings, none of which should be
|
||||
* currently written
|
||||
*/
|
||||
public class JSONFilePingStore implements TelemetryPingStore {
|
||||
private static final String LOGTAG = "Gecko" + JSONFilePingStore.class.getSimpleName();
|
||||
|
||||
@VisibleForTesting static final int MAX_PING_COUNT = 40; // TODO: value.
|
||||
|
||||
private static final String FILENAME = "ping-%s.json";
|
||||
private static final Pattern FILENAME_PATTERN = Pattern.compile("ping-([0-9]+)\\.json");
|
||||
|
||||
// We keep the key names short to reduce storage size impact.
|
||||
@VisibleForTesting static final String KEY_PAYLOAD = "p";
|
||||
@VisibleForTesting static final String KEY_URL = "u";
|
||||
|
||||
private final File storeDir;
|
||||
|
||||
public JSONFilePingStore(final File storeDir) {
|
||||
this.storeDir = storeDir;
|
||||
this.storeDir.mkdirs();
|
||||
}
|
||||
|
||||
@VisibleForTesting File getPingFile(final long id) {
|
||||
final String filename = String.format(Locale.US, FILENAME, id);
|
||||
return new File(storeDir, filename);
|
||||
}
|
||||
|
||||
/**
|
||||
* @return the ID from the filename or -1 if the id does not exist.
|
||||
*/
|
||||
@VisibleForTesting static int getIDFromFilename(final String filename) {
|
||||
final Matcher matcher = FILENAME_PATTERN.matcher(filename);
|
||||
if (!matcher.matches()) { // Matcher.matches has the side effect of starting the match - needed to call Matcher.group
|
||||
return -1;
|
||||
}
|
||||
return Integer.parseInt(matcher.group(1));
|
||||
}
|
||||
|
||||
@Override
|
||||
public void storePing(final long uniqueID, final TelemetryPing ping) throws IOException {
|
||||
final String output;
|
||||
try {
|
||||
output = new JSONObject()
|
||||
.put(KEY_PAYLOAD, ping.getPayload())
|
||||
.put(KEY_URL, ping.getURL())
|
||||
.toString();
|
||||
} catch (final JSONException e) {
|
||||
// Do not log the exception to avoid leaking personal data.
|
||||
throw new IOException("Unable to create JSON to store to disk");
|
||||
}
|
||||
|
||||
final FileOutputStream outputStream = new FileOutputStream(getPingFile(uniqueID), false);
|
||||
blockForLockAndWriteFileAndCloseStream(outputStream, output);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void maybePrunePings() {
|
||||
final File[] files = storeDir.listFiles(new PingFileFilter());
|
||||
if (files.length < MAX_PING_COUNT) {
|
||||
return;
|
||||
}
|
||||
|
||||
final SortedSet<Integer> ids = getIDsFromFileList(files);
|
||||
removeFilesWithSmallestIDs(ids, files.length - MAX_PING_COUNT);
|
||||
}
|
||||
|
||||
private static SortedSet<Integer> getIDsFromFileList(final File[] files) {
|
||||
// Since we get the ids from a file list which is probably sorted, I assume a TreeSet will get unbalanced
|
||||
// and could be inefficient. However, it sounds less complex and more efficient than the alternative: the
|
||||
// ConcurrentSkipListSet. In any case, our data is relatively small.
|
||||
final SortedSet<Integer> out = new TreeSet<>();
|
||||
for (final File file : files) {
|
||||
final int id = getIDFromFilename(file.getName());
|
||||
if (id >= 0) {
|
||||
out.add(id);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
private void removeFilesWithSmallestIDs(final SortedSet<Integer> ids, final int numFilesToRemove) {
|
||||
final Iterator<Integer> it = ids.iterator();
|
||||
int i = 0;
|
||||
while (i < numFilesToRemove) { // Sorted set so these are ascending values.
|
||||
i += 1;
|
||||
final Integer id = it.next(); // file count > files to remove so this should not throw.
|
||||
getPingFile(id).delete();
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
public ArrayList<TelemetryPingFromStore> getAllPings() {
|
||||
final File[] files = storeDir.listFiles(new PingFileFilter());
|
||||
final ArrayList<TelemetryPingFromStore> out = new ArrayList<>(files.length);
|
||||
for (final File file : files) {
|
||||
final FileInputStream inputStream;
|
||||
try {
|
||||
inputStream = new FileInputStream(file);
|
||||
} catch (final FileNotFoundException e) {
|
||||
throw new IllegalStateException("Expected file to exist");
|
||||
}
|
||||
|
||||
final JSONObject obj;
|
||||
try {
|
||||
// Potential optimization: re-use the same buffer for reading from files.
|
||||
obj = lockAndReadFileAndCloseStream(inputStream, (int) file.length());
|
||||
} catch (final IOException | JSONException e) {
|
||||
// We couldn't read this file so let's just skip it. These potentially
|
||||
// corrupted files should be removed when the data is pruned.
|
||||
Log.w(LOGTAG, "Error when reading file: " + file.getName() + " Likely corrupted. Ignoring");
|
||||
continue;
|
||||
}
|
||||
|
||||
if (obj == null) {
|
||||
Log.d(LOGTAG, "Could not read given file: " + file.getName() + " File is locked. Ignoring");
|
||||
continue;
|
||||
}
|
||||
|
||||
try {
|
||||
final String url = obj.getString(KEY_URL);
|
||||
final ExtendedJSONObject payload = new ExtendedJSONObject(obj.getString(KEY_PAYLOAD));
|
||||
final int id = getIDFromFilename(file.getName());
|
||||
if (id < 0) {
|
||||
throw new IllegalStateException("These files are already filtered - did not expect to see " +
|
||||
"an invalid ID in these files");
|
||||
}
|
||||
out.add(new TelemetryPingFromStore(url, payload, id));
|
||||
} catch (final IOException | JSONException | NonObjectJSONException e) {
|
||||
Log.w(LOGTAG, "Bad json in ping. Ignoring.");
|
||||
continue;
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void onUploadAttemptComplete(final Set<Integer> successfulRemoveIDs) {
|
||||
final File[] files = storeDir.listFiles(new PingFileFilter(successfulRemoveIDs));
|
||||
for (final File file : files) {
|
||||
file.delete();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Locks the given {@link FileOutputStream} and writes the given String. This method will close the given stream.
|
||||
*
|
||||
* Note: this method blocks until a file lock can be acquired.
|
||||
*/
|
||||
private static void blockForLockAndWriteFileAndCloseStream(final FileOutputStream outputStream, final String str)
|
||||
throws IOException {
|
||||
try {
|
||||
final FileLock lock = outputStream.getChannel().lock(0, Long.MAX_VALUE, false);
|
||||
if (lock != null) {
|
||||
// The file lock is released when the stream is closed. If we try to redundantly close it, we get
|
||||
// a ClosedChannelException. To be safe, we could catch that every time but there is a performance
|
||||
// hit to exception handling so instead we assume the file lock will be closed.
|
||||
FileUtils.writeStringToOutputStreamAndCloseStream(outputStream, str);
|
||||
}
|
||||
} finally {
|
||||
outputStream.close(); // redundant: closed when the stream is closed, but let's be safe.
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Locks the given {@link FileInputStream} and reads the data. This method will close the given stream.
|
||||
*
|
||||
* Note: this method returns null when a lock could not be acquired.
|
||||
*/
|
||||
private static JSONObject lockAndReadFileAndCloseStream(final FileInputStream inputStream, final int fileSize)
|
||||
throws IOException, JSONException {
|
||||
try {
|
||||
final FileLock lock = inputStream.getChannel().tryLock(0, Long.MAX_VALUE, true); // null when lock not acquired
|
||||
if (lock == null) {
|
||||
return null;
|
||||
}
|
||||
// The file lock is released when the stream is closed. If we try to redundantly close it, we get
|
||||
// a ClosedChannelException. To be safe, we could catch that every time but there is a performance
|
||||
// hit to exception handling so instead we assume the file lock will be closed.
|
||||
return new JSONObject(FileUtils.readStringFromInputStreamAndCloseStream(inputStream, fileSize));
|
||||
} finally {
|
||||
inputStream.close(); // redundant: closed when the steram is closed, but let's be safe.
|
||||
}
|
||||
}
|
||||
|
||||
private static class PingFileFilter implements FilenameFilter {
|
||||
private final Set<Integer> idsToFilter;
|
||||
|
||||
public PingFileFilter() {
|
||||
this(null);
|
||||
}
|
||||
|
||||
public PingFileFilter(final Set<Integer> idsToFilter) {
|
||||
this.idsToFilter = idsToFilter;
|
||||
}
|
||||
|
||||
@Override
|
||||
public boolean accept(final File dir, final String filename) {
|
||||
if (idsToFilter == null) {
|
||||
return FILENAME_PATTERN.matcher(filename).matches();
|
||||
}
|
||||
|
||||
return idsToFilter.contains(getIDFromFilename(filename));
|
||||
}
|
||||
}
|
||||
|
||||
public static final Parcelable.Creator<JSONFilePingStore> CREATOR = new Parcelable.Creator<JSONFilePingStore>() {
|
||||
@Override
|
||||
public JSONFilePingStore createFromParcel(final Parcel source) {
|
||||
final String storeDirPath = source.readString();
|
||||
return new JSONFilePingStore(new File(storeDirPath));
|
||||
}
|
||||
|
||||
@Override
|
||||
public JSONFilePingStore[] newArray(final int size) {
|
||||
return new JSONFilePingStore[size];
|
||||
}
|
||||
};
|
||||
|
||||
@Override
|
||||
public int describeContents() {
|
||||
return 0;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void writeToParcel(final Parcel dest, final int flags) {
|
||||
dest.writeString(storeDir.getAbsolutePath());
|
||||
}
|
||||
}
|
||||
@ -576,6 +576,7 @@ gbjar.sources += ['java/org/mozilla/gecko/' + x for x in [
|
||||
'telemetry/core/TelemetryCorePingBuilder.java',
|
||||
'telemetry/schedulers/TelemetryUploadAllPingsImmediatelyScheduler.java',
|
||||
'telemetry/schedulers/TelemetryUploadScheduler.java',
|
||||
'telemetry/stores/JSONFilePingStore.java',
|
||||
'telemetry/stores/TelemetryPingStore.java',
|
||||
'telemetry/TelemetryConstants.java',
|
||||
'telemetry/TelemetryDispatcher.java',
|
||||
|
||||
@ -0,0 +1,203 @@
|
||||
/*
|
||||
* This Source Code Form is subject to the terms of the Mozilla Public
|
||||
* 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.telemetry.stores;
|
||||
|
||||
import org.json.JSONObject;
|
||||
import org.junit.Before;
|
||||
import org.junit.Rule;
|
||||
import org.junit.Test;
|
||||
import org.junit.rules.TemporaryFolder;
|
||||
import org.junit.runner.RunWith;
|
||||
import org.mozilla.gecko.background.testhelpers.TestRunner;
|
||||
import org.mozilla.gecko.sync.ExtendedJSONObject;
|
||||
import org.mozilla.gecko.telemetry.TelemetryPing;
|
||||
import org.mozilla.gecko.telemetry.TelemetryPingFromStore;
|
||||
import org.mozilla.gecko.util.FileUtils;
|
||||
|
||||
import java.io.File;
|
||||
import java.io.FileOutputStream;
|
||||
import java.util.ArrayList;
|
||||
import java.util.Arrays;
|
||||
import java.util.HashSet;
|
||||
import java.util.regex.Matcher;
|
||||
import java.util.regex.Pattern;
|
||||
|
||||
import static org.junit.Assert.*;
|
||||
|
||||
/**
|
||||
* Unit test methods of the {@link JSONFilePingStore} class.
|
||||
*/
|
||||
@RunWith(TestRunner.class)
|
||||
public class TestJSONFilePingStore {
|
||||
|
||||
private final Pattern ID_PATTERN = Pattern.compile("[^0-9]*([0-9]+)[^0-9]*");
|
||||
|
||||
@Rule
|
||||
public TemporaryFolder tempDir = new TemporaryFolder();
|
||||
private File testDir;
|
||||
private JSONFilePingStore testStore;
|
||||
|
||||
@Before
|
||||
public void setUp() throws Exception {
|
||||
testDir = tempDir.newFolder();
|
||||
testStore = new JSONFilePingStore(testDir);
|
||||
}
|
||||
|
||||
private ExtendedJSONObject generateTelemetryPayload() {
|
||||
final ExtendedJSONObject out = new ExtendedJSONObject();
|
||||
out.put("str", "a String");
|
||||
out.put("int", 42);
|
||||
out.put("null", (ExtendedJSONObject) null);
|
||||
return out;
|
||||
}
|
||||
|
||||
private void assertIsGeneratedPayload(final ExtendedJSONObject actual) throws Exception {
|
||||
assertNull("Null field is null", actual.getObject("null"));
|
||||
assertEquals("int field is correct", 42, (int) actual.getIntegerSafely("int"));
|
||||
assertEquals("str field is correct", "a String", actual.getString("str"));
|
||||
}
|
||||
|
||||
private void assertStoreFileCount(final int expectedCount) {
|
||||
assertEquals("Store contains " + expectedCount + " item(s)", expectedCount, testDir.list().length);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testConstructorOnlyWritesToGivenDir() throws Exception {
|
||||
// Constructor is called in @Before method
|
||||
assertTrue("Store dir exists", testDir.exists());
|
||||
assertEquals("Temp dir contains one dir (the store dir)", 1, tempDir.getRoot().list().length);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testStorePingStoresCorrectData() throws Exception {
|
||||
assertStoreFileCount(0);
|
||||
|
||||
final int expectedID = 48679;
|
||||
final TelemetryPing expectedPing = new TelemetryPing("a/server/url", generateTelemetryPayload());
|
||||
testStore.storePing(expectedID, expectedPing);
|
||||
|
||||
assertStoreFileCount(1);
|
||||
final String filename = testDir.list()[0];
|
||||
assertTrue("Filename contains expected ID", filename.contains(Integer.toString(expectedID)));
|
||||
final JSONObject actual = FileUtils.readJSONObjectFromFile(new File(testDir, filename));
|
||||
assertEquals("Ping urls are equal", expectedPing.getURL(), actual.getString(JSONFilePingStore.KEY_URL));
|
||||
assertIsGeneratedPayload(new ExtendedJSONObject(actual.getString(JSONFilePingStore.KEY_PAYLOAD)));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testStorePingMultiplePingsStoreSeparateFiles() throws Exception {
|
||||
assertStoreFileCount(0);
|
||||
for (int i = 1; i < 10; ++i) {
|
||||
testStore.storePing(i, new TelemetryPing("server " + i, generateTelemetryPayload()));
|
||||
assertStoreFileCount(i);
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testStorePingReleasesFileLock() throws Exception {
|
||||
assertStoreFileCount(0);
|
||||
testStore.storePing(0, new TelemetryPing("server", generateTelemetryPayload()));
|
||||
assertStoreFileCount(1);
|
||||
final File file = new File(testDir, testDir.list()[0]);
|
||||
final FileOutputStream stream = new FileOutputStream(file);
|
||||
try {
|
||||
assertNotNull("File lock is released after store write", stream.getChannel().tryLock());
|
||||
} finally {
|
||||
stream.close(); // releases lock
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testGetAllPings() throws Exception {
|
||||
final String urlPrefix = "url";
|
||||
writeTestPingsToStore(3, urlPrefix);
|
||||
|
||||
final ArrayList<TelemetryPingFromStore> pings = testStore.getAllPings();
|
||||
for (final TelemetryPingFromStore ping : pings) {
|
||||
final int id = ping.getUniqueID(); // we use ID as a key for specific pings and check against the url values.
|
||||
assertEquals("Expected url value received", urlPrefix + id, ping.getURL());
|
||||
assertIsGeneratedPayload(ping.getPayload());
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testMaybePrunePingsDoesNothingIfAtMax() throws Exception {
|
||||
final int pingCount = JSONFilePingStore.MAX_PING_COUNT;
|
||||
writeTestPingsToStore(pingCount, "whatever");
|
||||
assertStoreFileCount(pingCount);
|
||||
testStore.maybePrunePings();
|
||||
assertStoreFileCount(pingCount);
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testMaybePrunePingsPrunesIfAboveMax() throws Exception {
|
||||
final int pingCount = JSONFilePingStore.MAX_PING_COUNT + 1;
|
||||
writeTestPingsToStore(pingCount, "whatever");
|
||||
assertStoreFileCount(pingCount);
|
||||
testStore.maybePrunePings();
|
||||
assertStoreFileCount(JSONFilePingStore.MAX_PING_COUNT);
|
||||
|
||||
final HashSet<Integer> existingIDs = new HashSet<>(JSONFilePingStore.MAX_PING_COUNT);
|
||||
for (final String filename : testDir.list()) {
|
||||
existingIDs.add(getIDFromFilename(filename));
|
||||
}
|
||||
assertFalse("Smallest ID was removed", existingIDs.contains(1));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testOnUploadAttemptCompleted() throws Exception {
|
||||
writeTestPingsToStore(10, "url");
|
||||
final HashSet<Integer> unuploadedPingIDs = new HashSet<>(Arrays.asList(1, 3, 5, 7, 9));
|
||||
final HashSet<Integer> removedPingIDs = new HashSet<>(Arrays.asList(2, 4, 6, 8, 10));
|
||||
testStore.onUploadAttemptComplete(removedPingIDs);
|
||||
|
||||
for (final String unuploadedFilePath : testDir.list()) {
|
||||
final int unuploadedID = getIDFromFilename(unuploadedFilePath);
|
||||
assertFalse("Unuploaded ID is not in removed ping IDs", removedPingIDs.contains(unuploadedID));
|
||||
assertTrue("Unuploaded ID is in unuploaded ping IDs", unuploadedPingIDs.contains(unuploadedID));
|
||||
unuploadedPingIDs.remove(unuploadedID);
|
||||
}
|
||||
assertTrue("All non-successful-upload ping IDs were matched", unuploadedPingIDs.isEmpty());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testGetPingFileContainsID() throws Exception {
|
||||
final int expected = 1234567890;
|
||||
final File file = testStore.getPingFile(expected);
|
||||
assertTrue("Ping filename contains ID", file.getName().contains(Integer.toString(expected)));
|
||||
}
|
||||
|
||||
@Test // assumes {@link JSONFilePingStore.getPingFile(String)} is working.
|
||||
public void testGetIDFromFilename() throws Exception {
|
||||
final int expectedID = 465739201;
|
||||
final File file = testStore.getPingFile(expectedID);
|
||||
assertEquals("Retrieved ID from filename", expectedID, JSONFilePingStore.getIDFromFilename(file.getName()));
|
||||
}
|
||||
|
||||
private int getIDFromFilename(final String filename) {
|
||||
final Matcher matcher = ID_PATTERN.matcher(filename);
|
||||
assertTrue("Filename contains ID", matcher.matches());
|
||||
return Integer.parseInt(matcher.group(1));
|
||||
}
|
||||
|
||||
/**
|
||||
* Writes pings to store without using store API with:
|
||||
* id = 1 to count (inclusive)
|
||||
* server = urlPrefix + id
|
||||
* payload = generated payload
|
||||
*
|
||||
* Note: assumes {@link JSONFilePingStore#getPingFile(long)} works.
|
||||
*/
|
||||
private void writeTestPingsToStore(final int count, final String urlPrefix) throws Exception {
|
||||
for (int i = 1; i <= count; ++i) {
|
||||
final JSONObject obj = new JSONObject()
|
||||
.put(JSONFilePingStore.KEY_URL, urlPrefix + i)
|
||||
.put(JSONFilePingStore.KEY_PAYLOAD, generateTelemetryPayload());
|
||||
FileUtils.writeJSONObjectToFile(testStore.getPingFile(i), obj);
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue
Block a user