Closed Bug 1505325 Opened 10 months ago Closed 9 months ago

Telemetry path filtering doesn't work right when cwd is neither topsrcdir nor objdir


(Firefox Build System :: General, defect)

Not set


(firefox65 fixed)

Tracking Status
firefox65 --- fixed


(Reporter: ted, Assigned: ted)


(Blocks 1 open bug)



(3 files)

Connor noticed while looking at some telemetry data that the arguments to `mach artifact` were all "<path omitted>". The fact that `mach artifact` was showing up at all is bug 1505205, FWIW. I looked at my local telemetry data and saw the same thing:
  "argv": [
    "<path omitted>",
    "<path omitted>",
    "<path omitted>"

I looked around a bit and noticed that when we invoke it from `mach bootstrap` we do so with cwd=~/.mozbuild:

Because of the way I implemented the path filtering logic, every argument gets treated as a path relative to cwd, so they'll all get turned into ~/.mozbuild/... and then omitted:

I think the simple fix here is just to add the cwd to the list of directories to use for path-relativization.
Duplicate of this bug: 1505207
This change adds python/mach/mach/test/ which contains
a simple test that running mach with the build telemetry setting enabled
causes us to write a telemetry file. The test fixture in the file should
make it easy to write additional tests.

A necessary precursor to make the tests work was to change mach_bootstrap's
`should_skip_dispatch` function, which would refuse to write telemetry data
if stdout was not a terminal (which it isn't in the tests) and also in
automation. The latter test is moved to ensure that we don't *submit*
telemetry data from automation, but we can still write it to disk. Machines
in automation should never have the telemetry setting enabled outside of
these tests anyway, so this should not change anything in practice.
After bug 1497638 the method by which build telemetry data gets written
to disk is slightly convoluted. Since we're now invoking `gather_telemetry`
from `post_dispatch_handler`, we just make that function return the data
it gathers and inline the contents of `telemetry_handler` after the call
to it.
The build telemetry code attempts to filter paths to avoid PII from usernames
and other things. It does this by converting every commandline argument to
an absolute path and then making them relative to topsrcdir or topobjdir and
omitting any that fail. This meant that running a mach command with a cwd
outside of the topsrcdir or objdir would omit all arguments since they were
converted to absolute paths from the cwd.

This change fixes this by adding the cwd to the list of paths used to create
relative paths, and also adds tests for this behavior.
I spent a little time writing a test fixture and some basic tests to make it easier to test this and bug 1505205.
(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #2)
> jobs?repo=try&revision=19d59f58567a429155c1fdf66d4a8162f53cb106

The tests failed on Windows with our old friend "TypeError: environment can only contain strings". I'll fix that and give it another go on try.
I pushed the latest changes to try and the tests are failing on Windows:

I'll run the tests locally on Windows and fix them and hopefully we can get this all landed.
This just needed a `mozpath.normpath` around the `getcwd` call to make the tests pass on Windows.
Pushed by
add a basic test for build telemetry. r=firefox-build-system-reviewers,chmanchester
refactor telemetry gathering slightly. r=firefox-build-system-reviewers,chmanchester
fix build telemetry path filtering when cwd is outside of topsrcdir/objdir. r=firefox-build-system-reviewers,chmanchester
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.