1116810 - Telemetry feature for Fennec Stumbler

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[:garvan]

Attachment: about:telemetry showing stumbler histograms

Adding histograms for the following:

"STUMBLER_TIME_BETWEEN_UPLOADS_SEC"
"STUMBLER_BYTES_UPLOADED_PER_SEC"
"STUMBLER_TIME_BETWEEN_START_SEC;
"STUMBLER_BYTES_PER_UPLOAD"
"STUMBLER_OBSERVATIONS_PER_UPLOAD"
"STUMBLER_CELLS_PER_UPLOAD"
"STUMBLER_WIFIS_PER_UPLOAD"
"STUMBLER_OBSERVATIONS_PER_DAY"

The mozstumbler github issue for this is https://github.com/mozilla/MozStumbler/pull/1378

Comment 1

[:garvan]

Attachment: Part 1: histograms.json update

Hopefully this isn't too many new histograms to add.

Comment 2

[:garvan]

Attachment: Part 2: mozbuild update for added files

Comment 3

[:garvan]

Attachment: Part 3: added telemetry to stumbler java code

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8543028 [details] [diff] [review]
Part 1: histograms.json update

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

::: toolkit/components/telemetry/Histograms.json
@@ +7061,5 @@
>      "keyed": true,
>      "description": "Counts of plugin and content process crashes which are reported with a crash dump."
> +  },
> +  "STUMBLER_TIME_BETWEEN_UPLOADS_SEC": {
> +    "expires_in_version": "never",

Probably all of these measurements should be marked as expiring in Fx45 or sooner. You can always extend them later.

@@ +7067,5 @@
> +    "n_buckets": 50,
> +    "high": 259200,
> +    "description": "Stumbler: The time in seconds between uploads."
> +  },
> +  "STUMBLER_BYTES_UPLOADED_PER_SEC": {

I'm not sure about this measurement. Is this transfer rate, a measure of amount of data uploaded, or something else?

@@ +7081,5 @@
> +    "n_buckets": 50,
> +    "high": 259200,
> +    "description": "Stumbler: The time between the service starts."
> +  },
> +  "STUMBLER_BYTES_PER_UPLOAD": {

For the uploads, I would prefer the pattern:

  STUMBLER_UPLOAD_BYTES
  STUMBLER_UPLOAD_OBSERVATIONS

etc.

@@ +7085,5 @@
> +  "STUMBLER_BYTES_PER_UPLOAD": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "n_buckets": 50,
> +    "high": 100000,

Is 100K the max? Should you make the max higher to find bugs?

@@ +7102,5 @@
> +    "n_buckets": 50,
> +    "high": 5000,
> +    "description": "Stumbler: The cells per upload."
> +  },
> +  "STUMBLER_WIFIS_PER_UPLOAD": {

STUMBLER_UPLOAD_APS?

@@ +7113,5 @@
> +  "STUMBLER_OBSERVATIONS_PER_DAY": {
> +    "expires_in_version": "never",
> +    "kind": "exponential",
> +    "n_buckets": 50,
> +    "high": 5000,

What are the realistic high values for observations per day, APs and cells per upload? Do we expect any of those to actually hit 5000? Or dramatically exceed it?

Comment 5

[:garvan]

Attachment: Part 1: histograms.json update

Thanks Richard, I addressed your comments in this updated patch.

> Probably all of these measurements should be marked as expiring in Fx45 or
> sooner. You can always extend them later.

Agreed, changed to expire in 45.
 
> > +  "STUMBLER_BYTES_UPLOADED_PER_SEC": {
> 
> I'm not sure about this measurement. Is this transfer rate, a measure of
> amount of data uploaded, or something else?

It is a volume measurement, normalized to per/sec, I updated the name and description.
 
> For the uploads, I would prefer the pattern:
> 
>   STUMBLER_UPLOAD_BYTES
>   STUMBLER_UPLOAD_OBSERVATIONS

Done.

> Is 100K the max? Should you make the max higher to find bugs?

Good point, upped this.
 
> STUMBLER_UPLOAD_APS?

Name changed (..WIFI_AP_COUNT)
 
> @@ +7113,5 @@
> > +  "STUMBLER_OBSERVATIONS_PER_DAY": 
> What are the realistic high values for observations per day, APs and cells
> per upload? Do we expect any of those to actually hit 5000? Or dramatically
> exceed it?

I raised these values in the updated patch, but I don't have enough data to really state what expected max is.

Comment 6

[:garvan]

Attachment: part 3: added telemetry to stumbler java code

- Updated to match changes in Histograms.json

- Added one more probe: TELEMETRY_TIME_BETWEEN_RECEIVED_LOCATIONS_SEC

Comment 7

[:garvan]

Attachment: Part 3: added telemetry to stumbler java code

(prev patch accidentally included debugging code in it, cleaned up)

- Updated to match changes in Histograms.json

- Added one more probe: TELEMETRY_TIME_BETWEEN_RECEIVED_LOCATIONS_SEC

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8544187 [details] [diff] [review]
Part 1: histograms.json update

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

::: toolkit/components/telemetry/Histograms.json
@@ +7114,5 @@
> +    "expires_in_version": "45",
> +    "kind": "exponential",
> +    "n_buckets": 50,
> +    "high": 10000,
> +    "description": "Stumbler: The observations per day."

Make sure this is well-defined. Each time an entry is added to this histogram, does it represent observations in the last 24 hours? If not, what does it represent, and is it useful in the aggregate?

@@ +7121,5 @@
> +    "expires_in_version": "45",
> +    "kind": "exponential",
> +    "n_buckets": 50,
> +    "high": 86400,
> +    "description": "Stumbler: The time between receiving passive locations."

Time since last passive location during stumbling activity?

Comment 9

[Victor Ng [:vng] [:crankycoder]]

Comment on attachment 8544227 [details] [diff] [review]
Part 3: added telemetry to stumbler java code

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

Just have to make sure that we backport the TELEMETRY_TIME_BETWEEN_RECEIVED_LOCATIONS_SEC metric back into the Android standalone client.

Comment 10

[:garvan]

> > +    "description": "Stumbler: The observations per day."
> Make sure this is well-defined. Each time an entry is added to this
> histogram, does it represent observations in the last 24 hours? If not, what
> does it represent, and is it useful in the aggregate?

Good point. I'll update the comment for this to clarify its use. It is normalized to per-day for reporting purposes. What we know in the code is observation count per time period (often a few days), which is updated only when uploading (which is a sporadic event). We would like to make statements about roughly how many observations per-day, per-week, per-month, are normal/low/high.
 
> @@ +7121,5 @@
> > +    "expires_in_version": "45",
> > +    "kind": "exponential",
> > +    "n_buckets": 50,
> > +    "high": 86400,
> > +    "description": "Stumbler: The time between receiving passive locations."
> 
> Time since last passive location during stumbling activity?

After a passive location arrives, when the next one arrives, probe that time diff. Should I reword the description? I am not sure what you are clarifying.

Comment 11

[:garvan]

> Just have to make sure that we backport the
> TELEMETRY_TIME_BETWEEN_RECEIVED_LOCATIONS_SEC metric back into the Android
> standalone client.

Will do, I am still testing this locally to ensure I am happy with the metrics before changing to checkin-needed. Looks ok when I create an artificial test environment, gathering enough data in the wild is time-consuming, and tricky because I have to make sure Fennec stays live, since the local telemetry data appears to be in-memory.

Comment 12

[Richard Newman [:rnewman]]

> After a passive location arrives, when the next one arrives, probe that time
> diff. Should I reword the description? I am not sure what you are clarifying.

Yeah, just trying to make these descriptions as clear as possible. Clarifying: does this only record while we're stumbling, or is it recorded every time we get a passive location? (Depends on when you unattach listeners, but it's worth documenting here.)

E.g., "Time in seconds between passive location updates received while stumbling."

Comment 13

[:garvan]

> Yeah, just trying to make these descriptions as clear as possible.
> Clarifying: does this only record while we're stumbling, or is it recorded
> every time we get a passive location? (Depends on when you unattach
> listeners, but it's worth documenting here.)

I think I can clarify a bit more, and in the process maybe come up with a better wording.
The phrase 'while stumbling' I avoid because it can mean 2 things:
1) User opted in to stumbling, passive listeners are registered at all times, thus user is "stumbling"
2) Because in (1) stumbling system is idle, only when a passive location arrives does the system wake up, run some scanning and then go back to sleep after a short time period. This is actively "stumbling" during this scan.

We are interested in this time distribution between passive location events arriving.

Hmm, still stuck for a better wording than the original one :).

Comment 14

[:garvan]

Attachment: Part 3: added telemetry to stumbler java code

Updating patch to remove an unused block of code. Carrying over r+ from [:vng].

Comment 15

[:garvan]

Attachment: Part 1: updates to Histograms.json

Updated description for observations per day histogram: "Stumbler: The number of observations between upload events, normalized to per day."
Carry over r+ from rnewman

Comment 16

[:garvan]

Attachment: Part 3: added telemetry to stumbler java code

Carry over r+ from vng.
Removed unnecessary variable.

Comment 17

[:garvan]

vng, here is a debugging patch, this will create toasts on the scanning events:
https://pastebin.mozilla.org/8167129

Comment 18

[:garvan]

try green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dc35de5b11b

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3d6070292804
https://hg.mozilla.org/integration/fx-team/rev/db1367a5cc7c
https://hg.mozilla.org/integration/fx-team/rev/6ba69cd7598b

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/3d6070292804
https://hg.mozilla.org/mozilla-central/rev/db1367a5cc7c
https://hg.mozilla.org/mozilla-central/rev/6ba69cd7598b