Add commit hook to reject bucket count changes in Histograms.json

NEW
Unassigned

Status

()

P4
normal
3 years ago
2 years ago

People

(Reporter: gfritzsche, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

(Whiteboard: [measurement:client])

(Reporter)

Description

3 years ago
(In reply to Roberto Agostino Vitillo (:rvitillo) from bug 1261530, comment #3)
> We have considered histograms to be immutable until now and I would like
> this to continue to be the case. Since several systems depend on that
> assumption (cerberus, medusa, moztelemetry, mozaggregator,
> telemetry-batch-view and possibly others), could we add some checks on the
> client to avoid this from happening again? The alternative is to add checks
> on all the systems downstream that depend on that assumptions.

I don't see us covering that easily in a client-build, but we could look into a commit hook that enforces this on changes to Histograms.json.
(Reporter)

Comment 1

3 years ago
The idea would be to add a commit hook that:
* only looks at changes to Histograms.json
* (it could look only at modifications, not additions or deletions)
* parses the old & new Histograms.json
* for each histogram entry that is in both in the old & new Histograms.json, checks that the following fields didn't change:
  * kind, keyed, low, high, n_buckets, n_values
(Reporter)

Comment 2

3 years ago
... and if they changed, rejects the commit with a descriptive message.

Does that sound good to you Roberto?
Flags: needinfo?(rvitillo)
That sounds good to me, thanks.
Flags: needinfo?(rvitillo)
(Reporter)

Comment 4

3 years ago
gps, does that look like a feasible commit hook?
If yes, is that something you could mentor or do you know who could?
Flags: needinfo?(gps)
Yes, this is feasible.

See https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/mozhghooks/prevent_idl_change_without_uuid_bump.py and https://hg.mozilla.org/hgcustom/version-control-tools/file/tip/hghooks/tests/test-prevent-idl-change-without-uuid-bump.t for a similar hook and test that looks at file content.

One thing I'll warn you against for implementing pre-commit hooks is that if you ever change the format of the histogram file you'll need to update the hook. And this change will need to "ride the trains" which is a bit difficult in practice since hooks don't ride the trains since they aren't defined in the repo.
Flags: needinfo?(gps)
(Reporter)

Updated

3 years ago
See Also: → bug 1275743
(Reporter)

Updated

2 years ago
Priority: P3 → P4
You need to log in before you can comment on or make changes to this bug.