1332770 - The "google4" provider is showing up as "other" in some telemetry pings

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

We should be seeing "google", "google4" and "mozilla" in pings from Nightly, but instead, we see "google", "other" and "mozilla":

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-01-19&keys=google!other!mozilla!__none__&max_channel_version=nightly%252F53&measure=URLCLASSIFIER_UPDATE_REMOTE_STATUS2&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-12-21&table=0&trim=1&use_submission_date=0

although in this probe, we see both "google4" and "other":

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2017-01-19&keys=google!mozilla!other!google4&max_channel_version=nightly%252F53&measure=URLCLASSIFIER_UPDATE_ERROR&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-12-21&table=0&trim=1&use_submission_date=0

Comment 1

[Dimi Lee [ooo 16.04 - 21.04］]

The root cause is different for URLCLASSIFIER_UPDATE_ERROR & URLCLASSIFIER_UPDATE_REMOTE_STATUS2.

For URLCLASSIFIER_UPDATE_ERROR, we show google, mozilla, google4 and other. The "other" comes from update of test table[1], i think the solution here maybe add a "test" provider for test tables and ignore the results from test provider to be added to telemetry.

For URLCLASSIFIER_UPDATE_REMOTE_STATUS2, we show google, mozilla and other. The key used to record this telemetry is by converting mStreamTable to provider[2], and mStreamTable will only be set when UpdateUrlRequested is called[3]. So for v2 update, the status of first OnStartRequest of update will always be recorded to "other"(Since mStreamTable is empty), and then ProtocolParser got "u:redirect-url" and trigger another OnStartRequest for the real update url with correct provider recorded into telemetry. As for v4, there is no redirect, so every v4 update request is recorded to "other" category.

[1] https://dxr.mozilla.org/mozilla-beta/source/toolkit/components/url-classifier/SafeBrowsing.jsm#382
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#642
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#567

Comment 2

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: WIP Patch

Comment 3

[Dimi Lee [ooo 16.04 - 21.04］]

Attachment: Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

Review commit: https://reviewboard.mozilla.org/r/107058/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107058/

Comment 4

[François Marier [:francois]]

Comment on attachment 8830162 [details]
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

https://reviewboard.mozilla.org/r/107058/#review108458

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h:77
(Diff revision 1)
>    bool mIsUpdating;
>    bool mInitialized;
>    bool mDownloadError;
>    bool mBeganStream;
> +
> +  // Notice that mStreamTable is only used by v2, it is empty for v4 update.

nit: "Note that" instead of "Notice that"

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h:106
(Diff revision 1)
>    nsCOMPtr<nsIUrlClassifierCallback> mSuccessCallback;
>    nsCOMPtr<nsIUrlClassifierCallback> mUpdateErrorCallback;
>    nsCOMPtr<nsIUrlClassifierCallback> mDownloadErrorCallback;
> +
> +  // The provider for current update request.
> +  nsCString mProvider;

Maybe that should be called `mTelemetryProvider` to make sure we don't try to reuse it for something else (it would show up as "other" for any other providers that may be added by add-ons).

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:293
(Diff revision 1)
> +  nsCOMPtr<nsIUrlClassifierUtils> urlUtil =
> +    do_GetService(NS_URLCLASSIFIERUTILS_CONTRACTID);
> +
> +  nsTArray<nsCString> tables;
> +  mozilla::safebrowsing::Classifier::SplitTables(aRequestTables, tables);
> +  urlUtil->GetTelemetryProvider(tables.SafeElementAt(0, EmptyCString()), mProvider);  

nit: whitespace at the end of the line

::: toolkit/components/url-classifier/nsUrlClassifierUtils.cpp:282
(Diff revision 1)
>  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-"))) {

Some of the test tables are starting to have something else between `test` and `-` so we should test for a string starting with just `"test"`.

Comment 5

[Dimi Lee [ooo 16.04 - 21.04］]

Comment on attachment 8830162 [details]
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107058/diff/1-2/

Comment 6

[François Marier [:francois]]

Comment on attachment 8830162 [details]
Bug 1332770 - Fix the google4 provider is showing up as other in some telemetry pings.

https://reviewboard.mozilla.org/r/107058/#review109494

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e23905ef624
Fix the google4 provider is showing up as other in some telemetry pings. r=francois

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e23905ef624