1811374 - Include `installation.first_seen` event details in `new_profile` ping on Windows

Status: RESOLVED FIXED

(Firefox :: Installer, enhancement, P3)

Description

[Nick Alexander :nalexander [he/him]]

Right now, I can find installation.first_seen events for only about half of the new_profile pings that I see on Windows: query. This ticket tracks including this valuable information in our new_profile pings, helping us trace installation through to new profile creation.

Comment 1

[Nick Alexander :nalexander [he/him]]

Max and I looked into this a few days ago and it's more complicated than I realized.

• there is support for scalars in the new-profile ping (added in https://bugzilla.mozilla.org/show_bug.cgi?id=1570652), but it doesn't seem to have been used again
• the scalar used there is set very early during startup; the scalars in this ticket would need to block the ping
• perhaps the best approach is to kick off a promise for them when scheduling the ping (https://searchfox.org/mozilla-central/rev/edccfac5746704da49c1551aed8b79f57003bd68/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs#1215) and await that promise before sending the ping (https://searchfox.org/mozilla-central/rev/edccfac5746704da49c1551aed8b79f57003bd68/toolkit/components/telemetry/app/TelemetryControllerParent.sys.mjs#1237)
• it's not clear that the new-profile ping testing would work unaltered
• but maybe these details should be cached and added to the environment? Generally we're trying to avoid adding to the environment.
• extracting the functionality from https://searchfox.org/mozilla-central/rev/edccfac5746704da49c1551aed8b79f57003bd68/browser/modules/BrowserUsageTelemetry.jsm#1244 into an async collection method for re-use shouldn't be hard.
• we'd also need to add quite a few scalars since we can't include arbitrary objects, AFAIK. (But maybe we can? That would be convenient.)

:chutten, can you advise on whether adding such scalars to this ping is acceptable? If it is, can you comment on a good approach to schedule collection before we send the ping? (Or redirect if somebody else is better positioned to advise.)

Thanks!

Comment 2

[Chris H-C :chutten]

1. I see you've already noticed that Events in Custom Pings isn't something that's supported by Legacy Telemetry (This is not the last time I will recommend you consider Glean for this collection)
2. The delay in sending a "new-profile" ping was added to ensure we didn't send something before the user had a chance to opt out of data upload. Now we delete previously-sent data on data upload opt-out, so we can be a little more flexible about things like that.
3. "new-profile" ping supporting Scalars means if you shape the new collections as Scalars you don't need to hand-edit the schema to add them. You could add arbitrary JSON... and then write the schema for how that'll work when it hits the pipeline. (Or you can use Glean and take advantage of many fine metrics types (including events))
4. You would definitely need to update the tests, if only to add new ones to cover your added instrumentation. (Testing is friendlier with Glean)
5. Please don't add anything to the Environment unless you really need them to be included in almost every ping that Legacy Telemetry sends.
6. Alas, I'm the only person positioned near Legacy Telemetry just now (Jan-Erik's on PTO, and Alessio's priority is manager things). The ownership of the "new-profile" ping could live with its instrumenting team (but that's Alessio and me as the final survivors there (see bug 1120370) and we're no longer under Firefox, which is where the work request came from)... but anyhoo. It's me.

This collection: it's not a datetime is it? checks Oh, it's a bunch of cool stuff. Why isn't this its own (Glean, obv) Custom Ping? I mean, you'd be starting from scratch for history, but from the sounds of things you're anticipating needing to do that anyway?

Overall, if my oh-so-subtle nudges towards The Better Data Collection System aren't swaying you, then yes: I'd recommend going with Scalars in the "new-profile" ping. I can help consult on any problems you run into on the instrumentation and testing front, but I don't expect your array of Promises and scheduling would negatively interact with any existing guarantees. But we'll still want to allocate effort for validating from the data-side after landing that the changes don't interfere with the normal operation of the ping. (Maybe couch them behind a Nimbus feature so we can turn it on/off remotely?)

Comment 3

[Chris H-C :chutten]

Oh, I guess I should sum up what :nalexander and I chatted about yesterday. This is seeming to be a "Scalars in the "new-profile" ping" sort of thing, the change in scheduling to ensure that the first_seen extras are present could induce some odd changes for "new-profile" pings with reason "shutdown" but nothing I'm worried about, and we should definitely make sure these changes are validated (and I maintain a Nimbus experiment+rollout is a good way to work it).

As always, I'm here to consult if you have any questions.

Comment 4

[Nick Alexander :nalexander [he/him]]

Attachment: WIP: Bug 1811374 - Pre: Use `add_setup` in `test_TelemetryController.js`. r?chutten

Comment 5

[Nick Alexander :nalexander [he/him]]

Attachment: WIP: Bug 1811374 - Initial patch from Max.

Depends on D176495

Comment 6

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten

Depends on D176496

Comment 7

[Nick Alexander :nalexander [he/him]]

Max: I applied your work and started fixing up the test, which was tricky because of pre-existing issues that prevented using .only(). I haven't tried this on Windows and obviously I've not tested the Telemetry events yet, so I'm not sure where the patch stands. But hopefully this is helpful!

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D176497	Bug 1811374 - Include installation.first_seen event details in new_profile ping, r=nalexander	mpohle	nalexander: Back Aug 21, 2023

:mpohle, could you please find another reviewer?

For more information, please visit BugBot documentation.

Comment 9

[Max Christian Pohle [:/dev/max]]

This is still on my radar, but has not progressed further due to other priorities.

Comment 10

[[e-mail]]

Wondering if this bug status is still accurate and what can be done to close it.

Comment 11

[Romain Testard [:RT]]

+1 on prioritizing it - this will help support unattributed funnel understanding. We need to get installer info to identify the installer type (full/stub/exe/msi) as well as whether it was run silently. These are seen as critical elements to segment our audience when attempting to measure retention.

Comment 12

[Nick Rishel [:nrishel]]

Clearing NI: will bring up in bug triage.

Comment 13

[Nipun Shukla]

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

3. "new-profile" ping supporting Scalars means if you shape the new collections as Scalars you don't need to hand-edit the schema to add them. You could add arbitrary JSON... and then write the schema for how that'll work when it hits the pipeline. (Or you can use Glean and take advantage of many fine metrics types (including events))

Sorry, I don't fully understand how this would work. I'm assuming we want all of the info inside first_seen, not just installer type and the silent bool. If I'm reading this right we can directly pass in the JSON (as a string I suppose) to one scalar and then deal with deserializing it on the pipeline side. Is that correct?

Comment 14

[Chris H-C :chutten]

That was a notice that all custom pings in Legacy Telemetry are arbitrary JSON and so adding or changing them is fraught with schema-related peril. Unless, like in the "new-profile" ping, there's been built a section that expects to be expanded, like for its scalars. Then you don't need to worry about changing the schema.

I do not recommend forcing JSON into a probe. (That's what Glean's object metric type is for. If using Glean is an option you wish to pursue.)

Comment 15

[Nick Alexander :nalexander [he/him]]

chutten: do we need new-profile ingestion changes to get the new fields into the relevant tables, or into Looker versions of those tables? Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d07e792e7623
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander

Comment 17

[Cristian Tuns]

Backed out for causing xpcshell failures in test_TelemetryController.js

• Backout link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryController.js | test_sendNewProfile - [test_sendNewProfile : 94] The ping must have the correct type. - "event" == "new-profile"

Comment 18

[Chris H-C :chutten]

Since these are Scalars in the usual manner there are no schema changes required (if interested, it's because the segment of the ping is expecting metrics). The schema generator knows to ask probe-scraper for scalars recorded in the "main" process into the "new-profile" store to tell bigquery what columns to generate in the table.

...as for Looker, that I'm not as sure about. I'm like 92% confident that the lookml generation will complete successfully and automatically to ensure that the scalar fields are available in explores and looks. If it's not there the same (or, at worst, the next) day that the columns are available in telemetry.new_profile, then definitely raise that question to Slack#data-help.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/313662a4f320
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander

Comment 20

[Nipun Shukla]

There's a race between which of the "event" and "new-profile" pings is received first within test_TelemetryController.js, if we get "event" as the first ping, discard it, get the next one, and check that is "new-profile." Also add a flag to not send the failureReason if we've already sent a new-profile ping in this session.

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8baece60b9c5
Backed out changeset 313662a4f320 for causing xpcshell failures on test_new_profile.js CLOSED TREE

Comment 22

[Aron Cseh]

Backed out for causing xpcshell failures on test_new_profile.js

• Backout link
• Push with failures
• Failure Log

Comment 23

[Nipun Shukla]

Newly added test fails on MSIX due to write restrictions.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d815518d57c6
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander

Comment 25

[Norisz Fay [:noriszfay]]

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