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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

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:
https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/python/mozboot/mozboot/base.py#299

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:
https://searchfox.org/mozilla-central/rev/b096dcf0ea226af628fe03f7e7acb56a25853533/python/mozbuild/mozbuild/telemetry.py#233

I think the simple fix here is just to add the cwd to the list of directories to use for path-relativization.

Comment 2

[(not currently active) Ted Mielczarek]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d59f58567a429155c1fdf66d4a8162f53cb106

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: bug 1505325 - add a basic test for build telemetry. r?build

This change adds python/mach/mach/test/test_telemetry.py 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.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: bug 1505325 - refactor telemetry gathering slightly. r?build

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.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: bug 1505325 - fix build telemetry path filtering when cwd is outside of topsrcdir/objdir. r?build

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.

Comment 6

[(not currently active) Ted Mielczarek]

I spent a little time writing a test fixture and some basic tests to make it easier to test this and bug 1505205.

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #2)
> https://treeherder.mozilla.org/#/
> 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.

Comment 8

[(not currently active) Ted Mielczarek]

I pushed the fixed patches to try along with bug 1505205:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=424e0622630845ab25f4789f62f49f74900

Comment 9

[(not currently active) Ted Mielczarek]

I pushed the latest changes to try and the tests are failing on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb7e0fef15c241d644bf8980435ca3a9b5345e3

I'll run the tests locally on Windows and fix them and hopefully we can get this all landed.

Comment 10

[(not currently active) Ted Mielczarek]

This just needed a `mozpath.normpath` around the `getcwd` call to make the tests pass on Windows.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6711bfcf0972
add a basic test for build telemetry. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/471e97d7fa1d
refactor telemetry gathering slightly. r=firefox-build-system-reviewers,chmanchester
https://hg.mozilla.org/integration/autoland/rev/de91182ad1af
fix build telemetry path filtering when cwd is outside of topsrcdir/objdir. r=firefox-build-system-reviewers,chmanchester

Comment 12

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/6711bfcf0972
https://hg.mozilla.org/mozilla-central/rev/471e97d7fa1d
https://hg.mozilla.org/mozilla-central/rev/de91182ad1af