Closed Bug 1505205 Opened 6 years ago Closed 5 years ago

mach recursively calls itself resulting in extra telemetry pings

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox65 fixed)

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: sheehan, Assigned: ted)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When scanning through the first few submitted data points for build telemetry, I noticed that `mach artifact` was the most frequently run command. Looking closer, I realized that my own client ID had been registered as running mach artifact quite a few times! Since I have never run this command, I concluded that `mach build` must be calling mach recursively and those inner mach calls are generating their own build telemetry pings. This was confirmed by a few others (ted, chmanchester) who pointed out a few places we are making calls to mach from within the build system.

We call mach artifact from this Makefile: https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/Makefile.in#167

Anything that calls this function: https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/python/mach/mach/registrar.py#108 will invoke a Mach command. This is according to chmanchester's investigations, I have yet to verify. :)
Blocks: buildmetrics
Additionally we call `mach artifact` from `mach bootstrap`:
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/python/mozboot/mozboot/base.py#299

I proposed that we should set an env var like `MACH_PID` on mach startup to the pid of the Python process only if it doesn't already exist. Then we can skip writing telemetry if the value of the env var doesn't match that of the current process.
Assignee: nobody → ted
This change tries to ensure that we don't write telemetry data for mach
commands invoked recursively as part of other mach commands. The intent of
build system telemetry is to only collect data about commands that users are
invoking directly.

There are two ways that we found mach commands can be recursively invoked:
* By running a python subprocess to recursively invoke mach (used in
  `mach bootstrap` to call `mach artifact toolchain`)
* By using `Registrar.dispatch` to delegate to a sub-command (used by many
  build system commands to invoke `mach build`).

The subprocess case is handled here by having mach set a `MACH_MAIN_PID`
environment variable whose value is the current process' pid on startup if it
does not already exist in the environment. Telemetry code then checks that the
value of that variable matches the current pid and skips writing telemetry data
if not.

The dispatch case is handled by making `MachRegistrar` store the current depth
of the command stack and pass it to the `post_dispatch_handler` which will skip
writing telemetry data if depth != 1.

Additionally the `should_skip_dispatch` function in mach_bootstrap is renamed
to `should_skip_telemetry_submission`, which was its original intent. The
combination of checks added in this change should be sufficient for deciding
when to write telemetry data, and we were not collecting telemetry for the set
of mach commands in that function (which included `mach bootstrap`).

In order to facilitate writing a test for the dispatch case this change adds a
`mach python --exec-file` option to execute Python code directly in the context
of the `mach python` command.
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #4)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=cb69aa76e837b59b4697c158be736d3648e1bbc5

This is green now. I had to fix yet another unicode-in-the-environment issue for Windows.
Pushed by tmielczarek@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/008ce258d380
don't write telemetry for recursive mach command invocations. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/mozilla-central/rev/008ce258d380
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1506888
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: