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




3 years ago
3 years ago


(Reporter: gfritzsche, Unassigned)



Firefox Tracking Flags

(firefox48 affected)


(Whiteboard: [measurement:client])

(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.
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
... 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)
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 and 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)
See Also: → 1275743
Priority: P3 → P4
You need to log in before you can comment on or make changes to this bug.