1621512 - Adhere to `record_in_processes` values for telemetry events in artifact builds

Status: NEW

(Toolkit :: Telemetry, enhancement, P3)

Description

[Bianca Danforth [:bdanforth]]

Attachment: bug-1616356-wip.diff

Currently, it is possible to register telemetry events in Events.yaml and record the event in an artifact build, though to see the events in tests (e.g. xpcshell or mochitest), a full build is required (I know there is some work in progress to change this, like Bug 1619990).

I encountered an issue where, for the exact same patch, the telemetry was recorded in an artifact build, but was not recorded in a full build. It turns out I mistakenly added "content" to the record_in_processes list for the wrong event in Events.yaml, yet the event was still recorded in the artifact build when it should not have been.

STR:

1. Apply the attached hg patch to mozilla-central (applies cleanly on 616ea1d7d24d).
2. Create an artifact build.
3. Run the artifact build.
4. Go to the password manager test page.
5. Click on the "new-password" field under "Registration Form" (second input field from the left at the top of the page).
6. Observe a password generation option is offered in the autocomplete popup that displays.
7. In a new tab, open about:telemetry#events-tab.
8. Click on "Content" in the sidebar, and observe the "autocomplete_shown" method for the "pwmgr" category is recorded.
9. Close Firefox.
10. Create a full build.
11. Run the full build.
12. Repeat steps 4-7.
13. Observe there is no "Content" submenu item in the sidebar, and therefore no "autocomplete_shown" telemetry event or any other event recorded in a content process.

Comment 1

[Jan-Erik Rediger [:janerik]]

Without running any test I think I found the place where this happens: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/TelemetryEvent.cpp#356-357

We check only the dynamic bool, that one is true for "dynamic builtins" (those loaded in artifact builds).
I can take this on later this week to fix.
(We're kinda chasing these bugs since we introduced "dynamic builtins" :/).

Comment 2

[Jan-Erik Rediger [:janerik]]

I spoke to soon, we're not even parsing the information for dynamic-builtins:
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/core/TelemetryEvent.cpp#356

So if we want to support this it would be a bigger refactor for a not-the-main-thing feature. What makes me less worried is that it does record, so you can at least test that you record something (and a proper test on try can then verify that it runs from a full build).

Wdyt, :chutten? At the least we can update the docs to note this gotcha.

(Dropping priority and assignment due to above)

Comment 3

[Chris H-C :chutten]

You are correct.

Fixing this would require

1. Adding the processes to generate_JSON_definitions in the build scripts
2. Either switching dynamic builtins to CommonEventInfo or adding processes to DynamicEventInfo
3. Reworking those CanRecordInProcesses call sites
4. So Many Tests

This isn't a small bug. Gonna P3 it because other work's taking precedence at the moment.