1275743 - Add a hook to prevent changes to Histograms.json without data collection peer r+

Status: NEW

(Developer Services :: Mercurial: hg.mozilla.org, defect, P3)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Recently my team landed a change to Histograms.json to add a telemetry probe, but we forgot to get review from a data collection peer. This resulted in us landing a telemetry probe that was poorly designed, and caused a significant performance hit to our data collection infrastructure.

To enforce that all new telemetry probes we add will not cause performance problems and also will respect our users' privacy, we should add a hook to hg.mozilla.org to prevent changes to Histograms.json from landing in mozilla repositories without r+ from a data collection peer.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I think there is no established form of the data collection review yet.
I've seen data collection peer f+, r+ and data-review=...

Comment 2

[Gregory Szorc [:gps]]

A similar hook is https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/whitelist_releng.py.

I would just implement it requiring the parsed r= reviewer to be in a whitelist.

There is code in version-control-tools for parsing reviewers. See pylib/mozautomation/mozautomation/commitparser.py and example usage in hgext/hgmo/__init__.py.

Comment 3

[Gregory Szorc [:gps]]

Will the requesting party be writing this code?

Comment 4

[Georg Fritzsche [:gfritzsche]]

From a reviewers perspective, it would be nice to require a separate "data-review=<peer>" for these.

:chutten brougth up some things to watch out for:
> We'll need to watch for tooling support. Mozreview sometimes munges data-r or data-review,
> converting to r= on submit. Bugzilla has no concept of data-review for bare attachments.
> Phabricator may or may not have customizable review roles, but the team was really interested
> in keeping it just as a review+ flag for the bugzilla integration portion.
> 
> So long as we can find a construct that is ignored by the tooling (data-review= seems like it
> ought to work), sounds good to me.

Comment 5

[Chris H-C :chutten]

We can even be clever and allow remove-only commits without a data-review. Only trigger the commit hook if the number of added or changed lines in the files is > 0.

Comment 6

[Georg Fritzsche [:gfritzsche]]

It looks like the hook that enforces WebIDL and IPC peer review already does most of what we want here:
https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/prevent_webidl_changes.py
This already takes care of some other concerns (uplifts, ...).

We could just extend the webidl hook with data review peers and add a check for "data-review=<data peer>" in the commit message.
From a separate mail thread, we'd like a separate "data-review=..." instead of "r=..." as data review peers.

gps, does that make sense to you?
How can that kind of change be tested locally?

Comment 7

[Gregory Szorc [:gps]]

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmods/index.html talks about how to get the dev environment up and running. To test the hooks, run `./run-tests -j8 hghooks`.

sheehan is working on unifying the "who can push to what" hooks this quarter. But as a stop-gap, extending prevent_webidl_changes.py to work for data reviews seems reasonable.

The use of "data-review=" for reviewer annotation could be a hiccup. We'd need to teach autoland about that so it knows to add it. That means teaching MozReview/Phabricator about it. That's a rabbit hole. IMO it is best to to capture everything through normal "review" annotations and establish a policy around who can change what as opposed to inventing a new review flavor.

Comment 8

[:glob ✱]

(In reply to Gregory Szorc [:gps] from comment #7)
> The use of "data-review=" for reviewer annotation could be a hiccup. We'd
> need to teach autoland about that so it knows to add it. That means teaching
> MozReview/Phabricator about it. That's a rabbit hole. IMO it is best to to
> capture everything through normal "review" annotations and establish a
> policy around who can change what as opposed to inventing a new review
> flavor.

+1