--- a/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
+++ b/mobile/android/base/java/org/mozilla/gecko/telemetry/TelemetryUploadService.java
@@ -1,110 +1,135 @@
/* 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;
+import android.app.IntentService;
import android.content.Context;
import android.content.Intent;
-import android.content.SharedPreferences;
-import android.support.annotation.NonNull;
-import android.support.annotation.Nullable;
-import android.support.annotation.WorkerThread;
-import android.text.TextUtils;
import android.util.Log;
import ch.boye.httpclientandroidlib.HttpResponse;
import ch.boye.httpclientandroidlib.client.ClientProtocolException;
import org.mozilla.gecko.GeckoProfile;
-import org.mozilla.gecko.GeckoSharedPrefs;
-import org.mozilla.gecko.background.BackgroundService;
-import org.mozilla.gecko.distribution.DistributionStoreCallback;
import org.mozilla.gecko.preferences.GeckoPreferences;
import org.mozilla.gecko.sync.net.BaseResource;
import org.mozilla.gecko.sync.net.BaseResourceDelegate;
import org.mozilla.gecko.sync.net.Resource;
-import org.mozilla.gecko.telemetry.core.TelemetryCorePingBuilder;
+import org.mozilla.gecko.telemetry.stores.TelemetryPingStore;
+import org.mozilla.gecko.util.NetworkUtils;
import org.mozilla.gecko.util.StringUtils;
import java.io.IOException;
import java.net.URISyntaxException;
import java.security.GeneralSecurityException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.concurrent.TimeUnit;
/**
- * The service that handles uploading telemetry payloads to the server.
- *
- * Note that we'll fail to upload if the network is off or background uploads are disabled but the caller is still
- * expected to increment the sequence number.
+ * The service that handles retrieving a list of telemetry pings to upload from the given
+ * {@link TelemetryPingStore}, uploading those payloads to the associated server, and reporting
+ * back to the Store which uploads were a success.
*/
-public class TelemetryUploadService extends BackgroundService {
+public class TelemetryUploadService extends IntentService {
private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + TelemetryUploadService.class.getSimpleName(), 0, 23);
private static final String WORKER_THREAD_NAME = LOGTAG + "Worker";
- public static final String ACTION_UPLOAD_CORE = "uploadCore";
- public static final String EXTRA_DEFAULT_SEARCH_ENGINE = "defaultSearchEngine";
- public static final String EXTRA_DOC_ID = "docId";
- public static final String EXTRA_PROFILE_NAME = "geckoProfileName";
- public static final String EXTRA_PROFILE_PATH = "geckoProfilePath";
- public static final String EXTRA_SEQ = "seq";
-
- private static final int MILLIS_IN_DAY = 1000 * 60 * 60 * 24;
+ public static final String ACTION_UPLOAD = "upload";
+ public static final String EXTRA_STORE = "store";
public TelemetryUploadService() {
super(WORKER_THREAD_NAME);
- // Intent redelivery can fail hard (e.g. we OOM as we try to upload, the Intent gets redelivered, repeat) so for
- // simplicity, we avoid it for now. In the unlikely event that Android kills our upload service, we'll thus fail
- // to upload the document with a specific sequence number. Furthermore, we never attempt to re-upload it.
- //
- // We'll fix this issue in bug 1243585.
+ // Intent redelivery can fail hard (e.g. we OOM as we try to upload, the Intent gets redelivered, repeat)
+ // so for simplicity, we avoid it. We expect the upload service to eventually get called again by the caller.
setIntentRedelivery(false);
}
/**
- * Handles a core ping with the mandatory extras:
- * EXTRA_DOC_ID: a unique document ID.
- * EXTRA_SEQ: a sequence number for this upload.
- * EXTRA_PROFILE_NAME: the gecko profile name.
- * EXTRA_PROFILE_PATH: the gecko profile path.
- *
- * Note that for a given doc ID, seq should always be identical because these are the tools the server uses to
- * de-duplicate documents. In order to maintain this consistency, we receive the doc ID and seq from the Intent and
- * rely on the caller to update the values. The Service can be killed at any time so we can't ensure seq could be
- * incremented properly if we tried to do so in the Service.
+ * Handles a ping with the mandatory extras:
+ * * EXTRA_STORE: A {@link TelemetryPingStore} where the pings to upload are located
*/
@Override
public void onHandleIntent(final Intent intent) {
Log.d(LOGTAG, "Service started");
+ if (!isReadyToUpload(this, intent)) {
+ return;
+ }
+
+ final TelemetryPingStore store = intent.getParcelableExtra(EXTRA_STORE);
+ uploadPendingPingsFromStore(store);
+ store.maybePrunePings();
+ Log.d(LOGTAG, "Service finished: upload and prune attempts completed");
+ }
+
+ private static void uploadPendingPingsFromStore(final TelemetryPingStore store) {
+ final ArrayList<TelemetryPingFromStore> pingsToUpload = store.getAllPings();
+
+ final HashSet<Integer> successfulUploadIDs = new HashSet<>(pingsToUpload.size()); // used for side effects.
+ final PingResultDelegate delegate = new PingResultDelegate(successfulUploadIDs);
+ for (final TelemetryPingFromStore ping : pingsToUpload) {
+ // There are minimal gains in trying to upload if we already failed one attempt.
+ if (delegate.hadConnectionError()) {
+ break;
+ }
+
+ // TODO: It'd be great to re-use the same HTTP connection for each upload request.
+ delegate.setPingID(ping.getUniqueID());
+ uploadPing(ping, delegate);
+ }
+
+ if (!delegate.hadConnectionError()) {
+ // We don't log individual successful uploads to avoid log spam.
+ Log.d(LOGTAG, "Telemetry upload success!");
+ }
+ store.onUploadAttemptComplete(successfulUploadIDs);
+ }
+
+ private static void uploadPing(final TelemetryPing ping, final ResultDelegate delegate) {
+ final BaseResource resource;
+ try {
+ resource = new BaseResource(ping.getURL());
+ } catch (final URISyntaxException e) {
+ Log.w(LOGTAG, "URISyntaxException for server URL when creating BaseResource: returning.");
+ return;
+ }
+
+ delegate.setResource(resource);
+ resource.delegate = delegate;
+ resource.setShouldCompressUploadedEntity(true);
+ resource.setShouldChunkUploadsHint(false); // Telemetry servers don't support chunking.
+
+ // We're in a background thread so we don't have any reason to do this asynchronously.
+ // If we tried, onStartCommand would return and IntentService might stop itself before we finish.
+ resource.postBlocking(ping.getPayload());
+ }
+
+ private static boolean isReadyToUpload(final Context context, final Intent intent) {
// Sanity check: is upload enabled? Generally, the caller should check this before starting the service.
// Since we don't have the profile here, we rely on the caller to check the enabled state for the profile.
- if (!isUploadEnabledByAppConfig(this)) {
+ if (!isUploadEnabledByAppConfig(context)) {
Log.w(LOGTAG, "Upload is not available by configuration; returning");
- return;
+ return false;
}
if (!isIntentValid(intent)) {
Log.w(LOGTAG, "Received invalid Intent; returning");
- return;
- }
-
- if (!ACTION_UPLOAD_CORE.equals(intent.getAction())) {
- Log.w(LOGTAG, "Unknown action: " + intent.getAction() + ". Returning");
- return;
+ return false;
}
- final String defaultSearchEngine = intent.getStringExtra(EXTRA_DEFAULT_SEARCH_ENGINE);
- final String docId = intent.getStringExtra(EXTRA_DOC_ID);
- final int seq = intent.getIntExtra(EXTRA_SEQ, -1);
+ if (!ACTION_UPLOAD.equals(intent.getAction())) {
+ Log.w(LOGTAG, "Unknown action: " + intent.getAction() + ". Returning");
+ return false;
+ }
- final String profileName = intent.getStringExtra(EXTRA_PROFILE_NAME);
- final String profilePath = intent.getStringExtra(EXTRA_PROFILE_PATH);
-
- uploadCorePing(docId, seq, profileName, profilePath, defaultSearchEngine);
+ return true;
}
/**
* Determines if the telemetry upload feature is enabled via the application configuration. Prefer to use
* {@link #isUploadEnabledByProfileConfig(Context, GeckoProfile)} if the profile is available as it takes into
* account more information.
*
* Note that this method logs debug statements when upload is disabled.
@@ -115,17 +140,17 @@ public class TelemetryUploadService exte
return false;
}
if (!GeckoPreferences.getBooleanPref(context, GeckoPreferences.PREFS_HEALTHREPORT_UPLOAD_ENABLED, true)) {
Log.d(LOGTAG, "Telemetry upload opt-out");
return false;
}
- if (!backgroundDataIsEnabled(context)) {
+ if (!NetworkUtils.isBackgroundDataEnabled(context)) {
Log.d(LOGTAG, "Background data is disabled");
return false;
}
return true;
}
/**
@@ -138,155 +163,124 @@ public class TelemetryUploadService exte
if (profile.inGuestMode()) {
Log.d(LOGTAG, "Profile is in guest mode");
return false;
}
return isUploadEnabledByAppConfig(context);
}
- private boolean isIntentValid(final Intent intent) {
+ private static boolean isIntentValid(final Intent intent) {
// Intent can be null. Bug 1025937.
if (intent == null) {
Log.d(LOGTAG, "Received null intent");
return false;
}
- if (intent.getStringExtra(EXTRA_DOC_ID) == null) {
- Log.d(LOGTAG, "Received invalid doc ID in Intent");
- return false;
- }
-
- if (!intent.hasExtra(EXTRA_SEQ)) {
- Log.d(LOGTAG, "Received Intent without sequence number");
- return false;
- }
-
- if (intent.getStringExtra(EXTRA_PROFILE_NAME) == null) {
- Log.d(LOGTAG, "Received invalid profile name in Intent");
- return false;
- }
-
- // GeckoProfile can use the name to get the path so this isn't strictly necessary.
- // However, getting the path requires parsing an ini file so we optimize by including it here.
- if (intent.getStringExtra(EXTRA_PROFILE_PATH) == null) {
- Log.d(LOGTAG, "Received invalid profile path in Intent");
+ if (intent.getParcelableExtra(EXTRA_STORE) == null) {
+ Log.d(LOGTAG, "Received invalid store in Intent");
return false;
}
return true;
}
- @WorkerThread
- private void uploadCorePing(@NonNull final String docId, final int seq, @NonNull final String profileName,
- @NonNull final String profilePath, @Nullable final String defaultSearchEngine) {
- final GeckoProfile profile = GeckoProfile.get(this, profileName, profilePath);
- final String clientId;
- try {
- clientId = profile.getClientId();
- } catch (final IOException e) {
- Log.w(LOGTAG, "Unable to get client ID to generate core ping: returning.", e);
- return;
- }
+ /**
+ * Logs on success & failure and appends the set ID to the given ArrayList on success.
+ *
+ * Note: you *must* set the ping ID before attempting upload or we'll throw!
+ *
+ * We use mutation on the set ID and the successful upload array to avoid object allocation.
+ */
+ private static class PingResultDelegate extends ResultDelegate {
+ // Since an IntentService handles one Intent at a time and queues subsequent Intents,
+ // an upload attempt can block the next upload attempt. This is problematic in the case
+ // that we don't have a connection – e.g. if we block for 2 minutes on each upload attempt,
+ // then each Intent will keep the radio active for two minutes and our users will have a
+ // bad day. Therefore, keep these durations short - we will try to upload these pings later
+ // and will generally not lose data.
+ //
+ // We can prevent this issue with better upload scheduling (bug 1268730).
+ private static final int SOCKET_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(20);
+ private static final int CONNECTION_TIMEOUT_MILLIS = (int) TimeUnit.SECONDS.toMillis(15);
- // Each profile can have different telemetry data so we intentionally grab the shared prefs for the profile.
- final SharedPreferences sharedPrefs = GeckoSharedPrefs.forProfileName(this, profileName);
- // TODO (bug 1241685): Sync this preference with the gecko preference.
- final String serverURLSchemeHostPort =
- sharedPrefs.getString(TelemetryConstants.PREF_SERVER_URL, TelemetryConstants.DEFAULT_SERVER_URL);
+ /** The store ID of the ping currently being uploaded. Use {@link #getPingID()} to access it. */
+ private int pingID = -1;
+ private final HashSet<Integer> successfulUploadIDs;
- final long profileCreationDate = getProfileCreationDate(profile);
- final TelemetryCorePingBuilder builder = new TelemetryCorePingBuilder(this, serverURLSchemeHostPort)
- .setClientID(clientId)
- .setDefaultSearchEngine(TextUtils.isEmpty(defaultSearchEngine) ? null : defaultSearchEngine)
- .setProfileCreationDate(profileCreationDate < 0 ? null : profileCreationDate)
- .setSequenceNumber(seq);
+ private boolean hadConnectionError = false;
- final String distributionId = sharedPrefs.getString(DistributionStoreCallback.PREF_DISTRIBUTION_ID, null);
- if (distributionId != null) {
- builder.setOptDistributionID(distributionId);
+ public PingResultDelegate(final HashSet<Integer> successfulUploadIDs) {
+ super();
+ this.successfulUploadIDs = successfulUploadIDs;
}
- final TelemetryPing corePing = builder.build();
- final CorePingResultDelegate resultDelegate = new CorePingResultDelegate();
- uploadPing(corePing, resultDelegate);
- }
+ @Override
+ public int socketTimeout() {
+ return SOCKET_TIMEOUT_MILLIS;
+ }
- private void uploadPing(final TelemetryPing ping, final ResultDelegate delegate) {
- final BaseResource resource;
- try {
- resource = new BaseResource(ping.getURL());
- } catch (final URISyntaxException e) {
- Log.w(LOGTAG, "URISyntaxException for server URL when creating BaseResource: returning.");
- return;
+ @Override
+ public int connectionTimeout() {
+ return CONNECTION_TIMEOUT_MILLIS;
}
- delegate.setResource(resource);
- resource.delegate = delegate;
- resource.setShouldCompressUploadedEntity(true);
- resource.setShouldChunkUploadsHint(false); // Telemetry servers don't support chunking.
-
- // We're in a background thread so we don't have any reason to do this asynchronously.
- // If we tried, onStartCommand would return and IntentService might stop itself before we finish.
- resource.postBlocking(ping.getPayload());
- }
+ private int getPingID() {
+ if (pingID < 0) {
+ throw new IllegalStateException("Expected ping ID to have been updated before retrieval");
+ }
+ return pingID;
+ }
- /**
- * @return the profile creation date in the format expected by TelemetryPingGenerator.
- */
- @WorkerThread
- private long getProfileCreationDate(final GeckoProfile profile) {
- final long profileMillis = profile.getAndPersistProfileCreationDate(this);
- // getAndPersistProfileCreationDate can return -1,
- // and we don't want to truncate (-1 / MILLIS) to 0.
- if (profileMillis < 0) {
- return profileMillis;
- }
- return (long) Math.floor((double) profileMillis / MILLIS_IN_DAY);
- }
-
- private static class CorePingResultDelegate extends ResultDelegate {
- public CorePingResultDelegate() {
- super();
+ public void setPingID(final int id) {
+ pingID = id;
}
@Override
public String getUserAgent() {
return TelemetryConstants.USER_AGENT;
}
@Override
public void handleHttpResponse(final HttpResponse response) {
final int status = response.getStatusLine().getStatusCode();
switch (status) {
case 200:
case 201:
- Log.d(LOGTAG, "Telemetry upload success.");
+ successfulUploadIDs.add(getPingID());
break;
default:
Log.w(LOGTAG, "Telemetry upload failure. HTTP status: " + status);
+ hadConnectionError = true;
}
}
@Override
public void handleHttpProtocolException(final ClientProtocolException e) {
// We don't log the exception to prevent leaking user data.
Log.w(LOGTAG, "HttpProtocolException when trying to upload telemetry");
+ hadConnectionError = true;
}
@Override
public void handleHttpIOException(final IOException e) {
// We don't log the exception to prevent leaking user data.
Log.w(LOGTAG, "HttpIOException when trying to upload telemetry");
+ hadConnectionError = true;
}
@Override
public void handleTransportException(final GeneralSecurityException e) {
// We don't log the exception to prevent leaking user data.
Log.w(LOGTAG, "Transport exception when trying to upload telemetry");
+ hadConnectionError = true;
+ }
+
+ private boolean hadConnectionError() {
+ return hadConnectionError;
}
}
/**
* A hack because I want to set the resource after the Delegate is constructed.
* Be sure to call {@link #setResource(Resource)}!
*/
private static abstract class ResultDelegate extends BaseResourceDelegate {