1230209 - Add more telemetry for Geolocation usage

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

-- getPosition vs. watchPosition
-- secure vs. not
-- allowed vs. not

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch

Richard, is this what you had in mind?

Benjamin, can you please approve the new telemetry probes?

Josh, can you please review? :)

Comment 2

[Benjamin Smedberg]

Comment on attachment 8695390 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch

Who is going to monitor these metrics, and for how long? "expires_in_version": "default" is not a valid flag for new probes; it's an alias for "never" which is probably not appropriate for this flag.

I don't think you need/want a keyed histogram for GEOLOCATION_GETCURRENTPOSITION_COUNT: can you just use an enumerated histogram with HTTP/HTTPS/OTHER buckets? That's much more efficient to process and graph than a keyed histogram.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Who is going to monitor these metrics, and for how long?
> "expires_in_version": "default" is not a valid flag for new probes; it's an
> alias for "never" which is probably not appropriate for this flag.

Ah, I didn't know that. I assumed "default" stood for a sensible default value somehow.

> I don't think you need/want a keyed histogram for
> GEOLOCATION_GETCURRENTPOSITION_COUNT: can you just use an enumerated
> histogram with HTTP/HTTPS/OTHER buckets? That's much more efficient to
> process and graph than a keyed histogram.

Good idea, will do.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v2

Comment 5

[Benjamin Smedberg]

Comment on attachment 8695816 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v2

We just landed bug_numbers for Histograms.json, so please add that field pointing to this bug when it lands. Otherwise great.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> We just landed bug_numbers for Histograms.json, so please add that field
> pointing to this bug when it lands. Otherwise great.

Will do, thanks!

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v3

f=bsmedberg

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8697019 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v3

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

::: dom/geolocation/nsGeolocation.h
@@ +209,5 @@
>  
>    // where the content was loaded from
>    nsCOMPtr<nsIPrincipal> mPrincipal;
>  
> +  // the protocol used to load the content

Can we describe the possible values?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Josh Matthews [:jdm] from comment #8)
> > +  // the protocol used to load the content
> 
> Can we describe the possible values?

Will do, thanks!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v4

f=bsmedberg r=jdm

Comment 11

[Tanvi Vyas[:tanvi]]

Comment on attachment 8701479 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v4

Hi Tim,

Can you add in a probe (or expand the existing probes) for persistence: Always Share vs Share.  Never Share vs Not Now.

Also, you may want to increase the nvalues for GEOLOCATION_GETCURRENTPOSITION_COUNT and GEOLOCATION_WATCHPOSITION_COUNT in case we want to add another category in the future.  I propose making them 6.

Comment 12

[Richard Barnes [:rbarnes]]

Comment on attachment 8701479 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v4

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

Overall, LGTM, modulo small things below.

I disagree with Tanvi that this needs to cover persistence.  Do it if it's obvious; if not, punt to another bug.  If you do it, please separate out by http/https.

::: dom/geolocation/nsGeolocation.cpp
@@ +447,5 @@
>  NS_IMETHODIMP
>  nsGeolocationRequest::Cancel()
>  {
> +  if (mRequester) {
> +    // Record the number of denied requests for regular web content.

Maybe note that Cancel() is only called when permission is denied, not, e.g., on unload.

@@ +1429,5 @@
>    if (mPendingCallbacks.Length() > MAX_GEO_REQUESTS_PER_WINDOW) {
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  if (mPrincipal) {

Why is this conditioned on mPrincipal?  Seems like it would be better to set mProtocolType to a specific value by default (different from the current ones), then just always record telemetry.

@@ +1431,5 @@
>    }
>  
> +  if (mPrincipal) {
> +    // Count the number of requests per protocol/scheme for web content.
> +    Telemetry::Accumulate(Telemetry::GEOLOCATION_GETCURRENTPOSITION_COUNT, mProtocolType);

Might be a little more aesthetic to align this with other similar counters, e.g.:

- COOKIE_SCHEME_SECURITY
- WEBRTC_GET_USER_MEDIA_SECURE_ORIGIN

The latter is probably a closer analogue.  The background here is that I would really like to get secure / nonsecure usage counters on basically the whole DOM, so it would be nice to start establishing a pattern for what we call these things.  ${THING}_SECURE_ORIGIN seems good enough.

::: dom/geolocation/nsGeolocation.h
@@ +210,5 @@
>    // where the content was loaded from
>    nsCOMPtr<nsIPrincipal> mPrincipal;
>  
> +  // the protocol used to load the content; where HTTP=0, HTTPS=1, Other=2
> +  uint8_t mProtocolType;

If you want to be super clean about this, you could make an enum class.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tanvi Vyas [:tanvi] from comment #11)
> Can you add in a probe (or expand the existing probes) for persistence:
> Always Share vs Share.  Never Share vs Not Now.

We already have some GeoLocation telemetry:

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsISecurityUITelemetry.idl#88

It doesn't differentiate between protocols but I can't see an easy way to make this work with the current patch. Not sure where/how exactly persistence is handled.

> Also, you may want to increase the nvalues for
> GEOLOCATION_GETCURRENTPOSITION_COUNT and GEOLOCATION_WATCHPOSITION_COUNT in
> case we want to add another category in the future.  I propose making them 6.

Good idea.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v5

Addressed all of Richard's feedback and bumped n_values to 6 as suggested by Tanvi.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8704386 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v5

Just occurred to me that it's probably good to have Josh look over these changes too!

Comment 16

[Richard Barnes [:rbarnes]]

Comment on attachment 8704386 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v5

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

::: toolkit/components/telemetry/Histograms.json
@@ +444,5 @@
>      "kind": "flag",
>      "description": "Has seen location error"
>    },
> +  "GEOLOCATION_GETCURRENTPOSITION_SECURE_ORIGIN" : {
> +    "expires_in_version" : "50",

Let's bump these out to 55, just in case we need to watch it for a while.

@@ +452,5 @@
> +    "description" : "Number of navigator.geolocation.getCurrentPosition() calls (0=other, 1=http, 2=https)"
> +  },
> +  "GEOLOCATION_REQUEST_GRANTED": {
> +    "expires_in_version": "50",
> +    "kind": "boolean",

Let's go ahead and split this out by secure/nonsecure as well.  Just make this enumerated and bin the values by outcome and security state, e.g.:

0 = denied / OTHER
1 = denied / HTTP
2 = denied / HTTPS
...
10 = allowed / OTHER
11 = allowed / HTTP
12 = allowed / HTTPS

This should be straightforward to implement; just switch on mProtocolType before you Telemetry::Accumulate.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v6

Comment 18

[Richard Barnes [:rbarnes]]

Comment on attachment 8707858 [details] [diff] [review]
0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch, v6

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

LGTM

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ddfdbf048c

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e1ddfdbf048c

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1230209-Add-more-telemetry-for-Geolocation-usage.patch (uplift)

f=bsmedberg r=tanvi,rbarnes,jdm

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: No impact.
[Describe test coverage new/current, TreeHerder]: No tests.
[Risks and why]: Low risk, adds more telemetry for the Geolocation API.
[String/UUID change made/needed]: None.

We would like to uplift this to sooner have a bit more data about http vs. https usage of the Geolocation API. The patch includes the fix from bug 1240766.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/882e6011c18f