1703318 - Send skeleton Glean custom ping from `--backgroundtask backgroundupdate`

Status: RESOLVED FIXED

(Toolkit :: Application Update, task)

Description

[Nick Alexander :nalexander [he/him]]

This ticket tracks using Firefox-on-Glean to send a skeleton Glean custom ping from --backgroundtask backgoundupdate that will allow us to assess the health of the background update system in the wild.

Most of the discussion has been in the meta ticket, Bug 1654891; this just tracks a small piece of the total work that we might want to do.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Pre: Fix typos. r?bytesized

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Pre: Allow to `app.update.background.force` updating. r?bytesized

Right now, force only forces scheduling. This makes it force
updating as well, useful for testing against local builds.

Depends on D111017

Comment 3

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Pre: Start out chatty. r?bytesized

This is just a convenience. I noticed while testing that we show no
output at all when no default profile exists, since any logging about
that fact cannot be shown because the relevant prefs to control it
can't be read.

Depends on D111018

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Pre: Separate NO_DEFAULT_PROFILE_EXISTS from NOT_DEFAULT_PROFILE. r?bytesized

This will help us discover if the assumption that "everybody has a
default profile" is not as universal as we had thought.

Depends on D111019

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Pre: Log with 'sync'. r?bytesized

This is just a precaution. I'm not sure if it will matter in the
wild, but better safe than sorry!

Depends on D111020

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Part 1: Add "background-update" Glean ping. r?chutten!,bytesized!

This does a few things:

1. It registers a few basic health metrics and a new Glean custom ping.

2. It arranges to mirror the default profile's
   datareporting.healthreport.uploadEnabled preference to the
   temporary background task profile. This requires not setting that
   pref for every background task. This is not risky because the
   Telemetry mechanism is completely disabled in background tasks at
   this time.

3. It initializes Firefox-on-Glean.

4. It uses the Glean APIs to submit the Glean custom ping.

Depends on D111021

Comment 7

[Phabricator Automation]

Comment on attachment 9213952 [details]
Bug 1703318 - Pre: Log with 'sync'. r?bytesized

Revision D111021 was moved to bug 1703877. Setting attachment 9213952 [details] to obsolete.

Comment 8

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Part 1: Extract default profile managing functions to BackgroundTasksUtils. r?bytesized

This is simply for testing convenience: it's awkward to use the
profile service to set the default profile. Centralizing these
functions in one place both makes the utility functions consistent and
helps make future tasks that want to do similar things adopt the
testing conventions automatically.

Comment 9

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Part 2: Add update environment to "background-update" Glean ping. r?chutten!

Depends on D111022

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Part 3: Add Marionette "background-update" ping test. r?chutten!,bytesized!

This is subtle but not so subtle that it can't be understood. The
major issue we might see is that the background update tasks all use
the same UpdRootD and app.update.background.enabled state,
essentially preventing concurrent Marionette tests. In the future we
may be able to do better.

Depends on D111348

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: bug1703318-data-review.md

Hello data reviewers! I want to get feedback as early as possible on this ping, so hopefully y'all can steer me right.

:chutten, I flagged you since you're familiar with most everything already, and presumably you know the more suitable victims to redirect to :)

Comment 12

[Chris H-C :chutten]

Comment on attachment 9214569 [details]
bug1703318-data-review.md

Please list the data collections, descriptions, and data collection categories in the request. It may seem redundant, but having complete documentation in the review request is a good thing. (At the very least it's a good habit to build as you can't expect Data Stewards to be able to read code or find their way around Phabricator to find YAML files they can read).

A fun one I'd like to talk about is background_update.exception -- when you say it has the text of the exception, is it possible to have PII in that text? Or user-supplied text of some kind?

Aside from that, everything looks in order for a speedy review.

Comment 13

[Phabricator Automation]

Comment on attachment 9214552 [details]
Bug 1703318 - Part 1: Extract default profile managing functions to BackgroundTasksUtils. r?bytesized

Revision D111347 was moved to bug 1704146. Setting attachment 9214552 [details] to obsolete.

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Chris H-C :chutten from comment #12)

Comment on attachment 9214569 [details]
bug1703318-data-review.md

Please list the data collections, descriptions, and data collection categories in the request. It may seem redundant, but having complete documentation in the review request is a good thing. (At the very least it's a good habit to build as you can't expect Data Stewards to be able to read code or find their way around Phabricator to find YAML files they can read).

I have done this. Just FYI, it would be nice for the Glean tooling to do this, since it can be automated. For future me, I ran the following script:

import yaml
tracking_bug = "[Bug 1703318](https://bugzilla.mozilla.org/show_bug.cgi?id=1703318)"
chunks = yaml.load(open("update_metrics.yaml"))
for section, metrics in chunks.items():
    if section.startswith("$"):  # Skip $schema.
        continue
    for metric, config in metrics.items():
        print(" | ".join(["{}.{}".format(section, metric), config["description"].strip(), " + ".join(config["data_sensitivity"]), tracking_bug]))

A fun one I'd like to talk about is background_update.exception -- when you say it has the text of the exception, is it possible to have PII in that text? Or user-supplied text of some kind?

I wasn't concerned about either, because these stacks are all of code we control, not untrusted JS. But you and :bytesized raised concerns, so I've removed it entirely. I'll file a ticket about capturing stacks in Glean at some point in the future.

Aside from that, everything looks in order for a speedy review.

Yay! v2 incoming.

Comment 15

[Nick Alexander :nalexander [he/him]]

Attachment: bug1703318-data-review-v2.md

Comment 16

[Chris H-C :chutten]

(In reply to Nick Alexander :nalexander [he/him] from comment #14)

I have done this. Just FYI, it would be nice for the Glean tooling to do this, since it can be automated. For future me, I ran the following script:

Yeah, we were chatting about this just last week, but were stuck on how to make glean_parser VCS-aware. The common case is someone adding or renewing a metric, after all, so giving folks a review filled with every metric in a file isn't the greatest.

But then I remembered that the one thing all the added/renewed metrics share is a bug. So if we wrote it as glean_parser --data-review-request <bug#> <files>, no VCS awareness needed.

I've filed this as bug 1704541.

Comment 17

[Chris H-C :chutten]

Comment on attachment 9214991 [details]
bug1703318-data-review-v2.md

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, :nalexander, :rtublitz, :rtestard and the rest of the Install/Update Team are responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical for the most part.
It includes the Telemetry client_id, which can be considered Cat4, but then again so does the "update" ping.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does there need to be a check-in in the future to determine whether to renew the data?

No. This collection is permanent.

---

Result: datareview+

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1703318 - Part 4: Avoid overlapping background update tasks accessing Glean storage concurrently. r?chutten!,bytesized!

Depends on D111349

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/441aa78b46f7
Pre: Fix typos. r=bytesized,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/4996717d09cd
Pre: Allow to `app.update.background.force` updating. r=bytesized,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/4a9b2fd76c6e
Part 1: Add "background-update" Glean ping. r=chutten,bytesized
https://hg.mozilla.org/integration/autoland/rev/c39df4a8d9ad
Part 2: Add update environment to "background-update" Glean ping. r=chutten,application-update-reviewers,bytesized
https://hg.mozilla.org/integration/autoland/rev/2d8b8f11b599
Part 3: Add Marionette "background-update" ping test. r=chutten,bytesized
https://hg.mozilla.org/integration/autoland/rev/215ec7244a6f
Part 4: Avoid overlapping background update tasks accessing Glean storage concurrently. r=chutten,bytesized,application-update-reviewers

Comment 20

[Dorel Luca [:dluca]]

Backed out 6 changesets (bug 1703318) for Doc failure in builds/worker/checkouts/gecko/toolkit/mozapps/update/update_metrics.yaml'. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer?job_id=336578596&repo=autoland&lineNumber=301

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&searchStr=documentation%2Copt%2Cdocumentation%2Csource-test-doc-generate%2Cgenerate&revision=215ec7244a6f5ef2797d639b1e770825f7a8c9cb

Backout:
https://hg.mozilla.org/integration/autoland/rev/3b9876116bf11c00b17bf89f0821a94d96a08171

Comment 21

[Nick Alexander :nalexander [he/him]]

Renamed the relevant files to not need to update the sparse checkout definition; here's a green doc(generate) task. Relanding.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f6f9bf20cd2
Pre: Fix typos. r=bytesized,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/1ad01b002e70
Pre: Allow to `app.update.background.force` updating. r=bytesized,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/92e0b182ed80
Part 1: Add "background-update" Glean ping. r=chutten,bytesized
https://hg.mozilla.org/integration/autoland/rev/0ada4975447a
Part 2: Add update environment to "background-update" Glean ping. r=chutten,application-update-reviewers,bytesized
https://hg.mozilla.org/integration/autoland/rev/eef49d5b41e5
Part 3: Add Marionette "background-update" ping test. r=chutten,bytesized
https://hg.mozilla.org/integration/autoland/rev/57a1854594d7
Part 4: Avoid overlapping background update tasks accessing Glean storage concurrently. r=chutten,bytesized,application-update-reviewers

Comment 23

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/5f6f9bf20cd2
https://hg.mozilla.org/mozilla-central/rev/1ad01b002e70
https://hg.mozilla.org/mozilla-central/rev/92e0b182ed80
https://hg.mozilla.org/mozilla-central/rev/0ada4975447a
https://hg.mozilla.org/mozilla-central/rev/eef49d5b41e5
https://hg.mozilla.org/mozilla-central/rev/57a1854594d7