Closed Bug 1811374 Opened 2 years ago Closed 2 months ago

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

Categories

(Firefox :: Installer, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
128 Branch
Tracking Status
firefox128 --- fixed

People

(Reporter: nalexander, Assigned: nshukla)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fidedi-attribution])

Attachments

(3 files)

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.

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

: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!

Depends on: 1570652
Flags: needinfo?(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?)

Flags: needinfo?(chutten)
Assignee: nobody → mpohle

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.

Priority: -- → P3
Blocks: 1821806
Blocks: 1825669

Depends on D176495

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!

Attachment #9330298 - Attachment description: WIP: Bug 1811374 - First round of tweaks. → WIP: Bug 1811374 - new-profile ping test is running
Attachment #9330298 - Attachment description: WIP: Bug 1811374 - new-profile ping test is running → WIP: Bug 1811374 - new-profile ping test is running, r=nalexander
Attachment #9330298 - Attachment description: WIP: Bug 1811374 - new-profile ping test is running, r=nalexander → Bug 1811374 - new-profile ping test is running, r=nalexander
Attachment #9330298 - Attachment description: Bug 1811374 - new-profile ping test is running, r=nalexander → Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=nalexander

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.

Flags: needinfo?(mpohle)
Attachment #9330298 - Attachment description: Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=nalexander → Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten
Severity: -- → S4

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

Flags: needinfo?(mpohle)

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

Flags: needinfo?(nalexander)

+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.

Clearing NI: will bring up in bug triage.

Flags: needinfo?(nalexander)
Attachment #9330298 - Attachment description: Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten → WIP: Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten

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

  1. "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?

Flags: needinfo?(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.)

Flags: needinfo?(chutten)
Attachment #9330298 - Attachment description: WIP: Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten → Bug 1811374 - Include `installation.first_seen` event details in `new_profile` ping, r=chutten

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!

Flags: needinfo?(chutten)
Pushed by nshukla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d07e792e7623
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander
Regressions: 1899158

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"
Flags: needinfo?(nshukla)

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.

Flags: needinfo?(chutten)
Pushed by nshukla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/313662a4f320
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander

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.

Flags: needinfo?(nshukla)
Backout by acseh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8baece60b9c5
Backed out changeset 313662a4f320 for causing xpcshell failures on test_new_profile.js CLOSED TREE

Backed out for causing xpcshell failures on test_new_profile.js

Flags: needinfo?(mpohle)

Newly added test fails on MSIX due to write restrictions.

Flags: needinfo?(mpohle)
Pushed by nshukla@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d815518d57c6
Include `installation.first_seen` event details in `new_profile` ping, r=chutten,nalexander
Assignee: mpohle → nshukla
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 128 Branch
See Also: → 1900786
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: