Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings. r?francois draft
authorDimi Lee <dlee@mozilla.com>
Fri, 27 Jan 2017 18:02:36 +0800
changeset 467661 33e5a0c68d1b2f0a32f27990e2efe47c42c25d68
parent 467601 045d8fe30f546ab08466c9586ce298e6459c2069
child 543745 63cd22ad5fba60c0c5b0ba17476441211960f776
push id43240
push userdlee@mozilla.com
push dateSat, 28 Jan 2017 13:58:58 +0000
reviewersfrancois
bugs1332770
milestone54.0a1
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings. r?francois MozReview-Commit-ID: KbpYAnf6qxd
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
toolkit/components/url-classifier/nsUrlClassifierUtils.h
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -626,18 +626,21 @@ nsUrlClassifierDBServiceWorker::FinishUp
   urlUtil->GetTelemetryProvider(mUpdateTables.SafeElementAt(0, EmptyCString()), provider);
 
   nsresult updateStatus = mUpdateStatus;
   if (NS_FAILED(mUpdateStatus)) {
    updateStatus = NS_ERROR_GET_MODULE(mUpdateStatus) == NS_ERROR_MODULE_URL_CLASSIFIER ?
      mUpdateStatus : NS_ERROR_UC_UPDATE_UNKNOWN;
   }
 
-  Telemetry::Accumulate(Telemetry::URLCLASSIFIER_UPDATE_ERROR, provider,
-                        NS_ERROR_GET_CODE(updateStatus));
+  // Do not record telemetry for testing tables.
+  if (!provider.Equals(TESTING_TABLE_PROVIDER_NAME)) {
+    Telemetry::Accumulate(Telemetry::URLCLASSIFIER_UPDATE_ERROR, provider,
+                          NS_ERROR_GET_CODE(updateStatus));
+  }
 
   mMissCache.Clear();
 
   if (NS_SUCCEEDED(mUpdateStatus)) {
     LOG(("Notifying success: %d", mUpdateWaitSec));
     mUpdateObserver->UpdateSuccess(mUpdateWaitSec);
   } else if (NS_ERROR_NOT_IMPLEMENTED == mUpdateStatus) {
     LOG(("Treating NS_ERROR_NOT_IMPLEMENTED a successful update "
@@ -984,17 +987,17 @@ nsUrlClassifierLookupCallback::LookupCom
       NS_ENSURE_SUCCESS(rv, rv);
       LOG(("The match from %s needs to be completed at %s",
            result.mTableName.get(), gethashUrl.get()));
       // gethashUrls may be empty in 2 cases: test tables, and on startup where
       // we may have found a prefix in an existing table before the listmanager
       // has registered the table. In the second case we should not call
       // complete.
       if ((!gethashUrl.IsEmpty() ||
-           StringBeginsWith(result.mTableName, NS_LITERAL_CSTRING("test-"))) &&
+           StringBeginsWith(result.mTableName, NS_LITERAL_CSTRING("test"))) &&
           mDBService->GetCompleter(result.mTableName,
                                    getter_AddRefs(completer))) {
 
         // TODO: Figure out how long the partial hash should be sent
         //       for completion. See Bug 1323953.
         nsAutoCString partialHash;
         if (StringEndsWith(result.mTableName, NS_LITERAL_CSTRING("-proto"))) {
           // We send the complete partial hash for v4 at the moment.
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ -19,16 +19,17 @@
 #include "mozilla/BasePrincipal.h"
 #include "mozilla/ErrorNames.h"
 #include "mozilla/Logging.h"
 #include "nsIInterfaceRequestor.h"
 #include "mozilla/LoadContext.h"
 #include "mozilla/Telemetry.h"
 #include "nsContentUtils.h"
 #include "nsIURLFormatter.h"
+#include "Classifier.h"
 
 static const char* gQuitApplicationMessage = "quit-application";
 
 // Limit the list file size to 32mb
 const uint32_t MAX_FILE_SIZE = (32 * 1024 * 1024);
 
 #undef LOG
 
@@ -92,16 +93,17 @@ nsUrlClassifierStreamUpdater::DownloadDo
   LOG(("nsUrlClassifierStreamUpdater::DownloadDone [this=%p]", this));
   mIsUpdating = false;
 
   mPendingUpdates.Clear();
   mDownloadError = false;
   mSuccessCallback = nullptr;
   mUpdateErrorCallback = nullptr;
   mDownloadErrorCallback = nullptr;
+  mTelemetryProvider.Truncate();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // nsIUrlClassifierStreamUpdater implementation
 
 nsresult
 nsUrlClassifierStreamUpdater::FetchUpdate(nsIURI *aUpdateUrl,
                                           const nsACString & aRequestPayload,
@@ -278,16 +280,24 @@ nsUrlClassifierStreamUpdater::DownloadUp
     request->mDownloadErrorCallback = aDownloadErrorCallback;
     return NS_OK;
   }
 
   if (NS_FAILED(rv)) {
     return rv;
   }
 
+  nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
+    do_GetService(NS_URLCLASSIFIERUTILS_CONTRACTID);
+
+  nsTArray<nsCString> tables;
+  mozilla::safebrowsing::Classifier::SplitTables(aRequestTables, tables);
+  urlUtil->GetTelemetryProvider(tables.SafeElementAt(0, EmptyCString()),
+                                mTelemetryProvider);
+
   mSuccessCallback = aSuccessCallback;
   mUpdateErrorCallback = aUpdateErrorCallback;
   mDownloadErrorCallback = aDownloadErrorCallback;
 
   mIsUpdating = true;
   *_retval = true;
 
   LOG(("FetchUpdate: %s", aUpdateUrl.Data()));
@@ -630,38 +640,32 @@ nsUrlClassifierStreamUpdater::OnStartReq
       if (NS_SUCCEEDED(rv) && uri) {
         uri->GetAsciiSpec(spec);
       }
       LOG(("nsUrlClassifierStreamUpdater::OnStartRequest "
            "(status=%s, uri=%s, this=%p)", errorName.get(),
            spec.get(), this));
     }
 
-    nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
-      do_GetService(NS_URLCLASSIFIERUTILS_CONTRACTID);
-
-    nsCString provider;
-    urlUtil->GetTelemetryProvider(mStreamTable, provider);
-
     if (NS_FAILED(status)) {
       // Assume we're overloading the server and trigger backoff.
       downloadError = true;
       mozilla::Telemetry::Accumulate(mozilla::Telemetry::URLCLASSIFIER_UPDATE_REMOTE_STATUS2,
-                                     provider, 15 /* unknown response code */);
+                                     mTelemetryProvider, 15 /* unknown response code */);
 
     } else {
       bool succeeded = false;
       rv = httpChannel->GetRequestSucceeded(&succeeded);
       NS_ENSURE_SUCCESS(rv, rv);
 
       uint32_t requestStatus;
       rv = httpChannel->GetResponseStatus(&requestStatus);
       NS_ENSURE_SUCCESS(rv, rv);
       mozilla::Telemetry::Accumulate(mozilla::Telemetry::URLCLASSIFIER_UPDATE_REMOTE_STATUS2,
-                                     provider, HTTPStatusToBucket(requestStatus));
+                                     mTelemetryProvider, HTTPStatusToBucket(requestStatus));
       LOG(("nsUrlClassifierStreamUpdater::OnStartRequest %s (%d)", succeeded ?
            "succeeded" : "failed", requestStatus));
       if (!succeeded) {
         // 404 or other error, pass error status back
         strStatus.AppendInt(requestStatus);
         downloadError = true;
       }
     }
--- a/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h
@@ -68,17 +68,20 @@ private:
   // Fetches the next request, from mPendingRequests
   nsresult FetchNextRequest();
 
 
   bool mIsUpdating;
   bool mInitialized;
   bool mDownloadError;
   bool mBeganStream;
+
+  // Note that mStreamTable is only used by v2, it is empty for v4 update.
   nsCString mStreamTable;
+
   nsCOMPtr<nsIChannel> mChannel;
   nsCOMPtr<nsIUrlClassifierDBService> mDBService;
   nsCOMPtr<nsITimer> mTimer;
 
   struct PendingRequest {
     nsCString mTables;
     nsCString mRequestPayload;
     bool mIsPostRequest;
@@ -93,11 +96,15 @@ private:
     nsCString mUrl;
     nsCString mTable;
   };
   nsTArray<PendingUpdate> mPendingUpdates;
 
   nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
   nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
+
+  // The provider for current update request and should be only used by telemetry
+  // since it would show up as "other" for any other providers.
+  nsCString mTelemetryProvider;
 };
 
 #endif // nsUrlClassifierStreamUpdater_h_
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.cpp
@@ -274,34 +274,37 @@ nsUrlClassifierUtils::ConvertListNameToT
 }
 
 NS_IMETHODIMP
 nsUrlClassifierUtils::GetProvider(const nsACString& aTableName,
                                   nsACString& aProvider)
 {
   MutexAutoLock lock(mProviderDictLock);
   nsCString* provider = nullptr;
-  if (mProviderDict.Get(aTableName, &provider)) {
+  if (StringBeginsWith(aTableName, NS_LITERAL_CSTRING("test"))) {
+    aProvider = NS_LITERAL_CSTRING(TESTING_TABLE_PROVIDER_NAME);
+  } else if (mProviderDict.Get(aTableName, &provider)) {
     aProvider = provider ? *provider : EmptyCString();
   } else {
     aProvider = EmptyCString();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierUtils::GetTelemetryProvider(const nsACString& aTableName,
                                   nsACString& aProvider)
 {
   GetProvider(aTableName, aProvider);
   // Filter out build-in providers: mozilla, google, google4
   // Empty provider is filtered as "other"
   if (!NS_LITERAL_CSTRING("mozilla").Equals(aProvider) &&
       !NS_LITERAL_CSTRING("google").Equals(aProvider) &&
-      !NS_LITERAL_CSTRING("google4").Equals(aProvider)) {
+      !NS_LITERAL_CSTRING("google4").Equals(aProvider) &&
+      !NS_LITERAL_CSTRING(TESTING_TABLE_PROVIDER_NAME).Equals(aProvider)) {
     aProvider.Assign(NS_LITERAL_CSTRING("other"));
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierUtils::GetProtocolVersion(const nsACString& aProvider,
--- a/toolkit/components/url-classifier/nsUrlClassifierUtils.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierUtils.h
@@ -5,16 +5,18 @@
 #ifndef nsUrlClassifierUtils_h_
 #define nsUrlClassifierUtils_h_
 
 #include "nsAutoPtr.h"
 #include "nsIUrlClassifierUtils.h"
 #include "nsClassHashtable.h"
 #include "nsIObserver.h"
 
+#define TESTING_TABLE_PROVIDER_NAME "test"
+
 class nsUrlClassifierUtils final : public nsIUrlClassifierUtils,
                                    public nsIObserver
 {
 public:
   typedef nsClassHashtable<nsCStringHashKey, nsCString> ProviderDictType;
 
 private:
   /**