1121559 - Use telemetry to measure WAP sites served to mobile Firefox browsers

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mike Taylor [:miketaylr]]

The Web Compat team would like to measure how frequently mobile Firefox users are served WAP/WML content.

Ideally this would run for few months and we could make a decision on next steps at that point.

See Bug 997668 and Bug 941241 (more specifically the bugs they block) for examples of mobile Firefox users being served WAP/WML content.

I think a "count" histogram makes sense as a proxy for page views.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 1121559.patch

Here's a proposed patch--but I have no idea who to ping for a review. Any suggestions?

Comment 2

[Mike Taylor [:miketaylr]]

AFAICT, there's no way to set a cpp_guard for "mobile" or b2g+Android (or is there?). Presumably Desktop browsers are never sent WAP content, so I think we could live with the few (if any) false negatives here. Unless someone has a better suggestion.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Mike Taylor [:miketaylr] from comment #2)
> AFAICT, there's no way to set a cpp_guard for "mobile" or b2g+Android (or is
> there?). Presumably Desktop browsers are never sent WAP content, so I think
> we could live with the few (if any) false negatives here. Unless someone has
> a better suggestion.

You'd probably want to #ifdef the measurement part anyway, which would be easily doable for "mobile"; having an extra histogram in Histograms.json isn't going to hurt anything.

The Histograms.json portion of the patch looks OK, can't speaking to the networking part.

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: 1121559-2.patch

Thanks for the feedback Nathan. I've #ifdef'ed it on ANDROID and MOZ_B2G.

Comment 5

[Mike Taylor [:miketaylr]]

Comment on attachment 8549030 [details] [diff] [review]
1121559-2.patch

r? :mcmanus because I see he did a review for Bug 976912 (hope that's oK!)

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8549030 [details] [diff] [review]
1121559-2.patch

Review of attachment 8549030 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1557,5 @@
> +#if defined(ANDROID) || defined(MOZ_B2G)
> +    // Gather telemetry on being sent a WAP content-type
> +    // This check will catch (at least) the following content types:
> +    // application/wml+xml, application/vnd.wap.xhtml+xml,
> +    // text/vnd.wap.wml, application/vnd.wap.wmlc

if (gHttpHandler->IsTelemetryEnabled()) {} will avoid those string operations if we aren't reporting telemetry.

@@ +1560,5 @@
> +    // application/wml+xml, application/vnd.wap.xhtml+xml,
> +    // text/vnd.wap.wml, application/vnd.wap.wmlc
> +    if (mResponseHead->ContentType().Find(".wap") != -1 ||
> +        mResponseHead->ContentType().Find("/wml") != -1) {
> +        Telemetry::Accumulate(Telemetry::HTTP_WAP_CONTENT_TYPE_COUNT, 1);

I think you want a boolean here - isWap and report that.

That way you can figure out the fraction of responses that are wap instead of a raw number without any context.

::: toolkit/components/telemetry/Histograms.json
@@ +1211,5 @@
>      "description": "Whether a HTTP transaction routed via Alt-Svc was scheme=http"
>    },
> +  "HTTP_WAP_CONTENT_TYPE_COUNT": {
> +    "expires_in_version": "40",
> +    "kind": "count",

boolean

Comment 7

[Mike Taylor [:miketaylr]]

Attachment: 1121559-3.patch

Thanks for the review! I think I've addressed all comments and also added a ContentType().IsEmpty() condition before setting isWap to true.

Comment 8

[Patrick McManus [:mcmanus]]

Comment on attachment 8550570 [details] [diff] [review]
1121559-3.patch

Review of attachment 8550570 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm thanks

Comment 9

[Mike Taylor [:miketaylr]]

Thanks!

Comment 10

[Carsten Book [:Tomcat]]

Hi,

can we get a try run for this change, thanks!

Comment 11

[Mike Taylor [:miketaylr]]

Here's the try run (ran this on Friday--forgot to link /o\): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3104f386a2d7

This was my first time using try, I wasn't really sure what to select, but this is green.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/8b09ab087404

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b09ab087404

Comment 14

[Mike Taylor [:miketaylr]]

My apologies, I didn't see [1] until today. ni? :bsmedberg for approval, even though this has landed.

[1] <https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval>