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)
Firefox Build System
General
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. :)
Reporter | ||
Updated•6 years ago
|
Blocks: buildmetrics
Assignee | ||
Comment 1•6 years ago
|
||
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
Assignee | ||
Comment 2•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c45c83693671ed9e78c8ac04cce9b39087e11a56
Assignee | ||
Comment 4•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb69aa76e837b59b4697c158be736d3648e1bbc5
Assignee | ||
Comment 5•6 years ago
|
||
(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.
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb7e0fef15c241d644bf8980435ca3a9b5345e3
Assignee | ||
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9ad5a85417edc47d25c33c8b75e87fc8a826bab
Assignee | ||
Comment 8•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9f5520e3c8461280f9071ef4ce05e1bbea241c6
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
Comment 10•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/008ce258d380
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in
before you can comment on or make changes to this bug.
Description
•