Use telemetry to measure WAP sites served to mobile Firefox browsers

RESOLVED FIXED in mozilla38

Status

()

Toolkit
Telemetry
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: miketaylr, Assigned: miketaylr)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8548999 [details] [diff] [review]
1121559.patch

Here's a proposed patch--but I have no idea who to ping for a review. Any suggestions?
Assignee: nobody → miket
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.
(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.
Attachment #8548999 - Flags: feedback+
Created attachment 8549030 [details] [diff] [review]
1121559-2.patch

Thanks for the feedback Nathan. I've #ifdef'ed it on ANDROID and MOZ_B2G.
Attachment #8548999 - Attachment is obsolete: true
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 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
Attachment #8549030 - Flags: review?(mcmanus)
Created attachment 8550570 [details] [diff] [review]
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.
Attachment #8549030 - Attachment is obsolete: true
Comment on attachment 8550570 [details] [diff] [review]
1121559-3.patch

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

lgtm thanks
Attachment #8550570 - Flags: review?(mcmanus) → review+
Thanks!
Keywords: checkin-needed
Hi,

can we get a try run for this change, thanks!
Flags: needinfo?(miket)
Keywords: checkin-needed
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/8b09ab087404
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8b09ab087404
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
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>

Updated

4 years ago
Flags: needinfo?(benjamin)
Attachment #8550570 - Flags: feedback+
You need to log in before you can comment on or make changes to this bug.