1379184 - Enforce Telemetry Histogram Definition Immutability at Build Time

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Chris H-C :chutten]

Telemetry Histogram Definitions (the JSON things in toolkit/components/telemetry/Histograms.json) are supposed to have an immutable "n_values" property. If this is ever increased, the aggregator and other server-side components stop being able to properly handle the histogram.

This is something that is mentioned in docs and conversation, but the wide appeal and use of Telemetry means we can't be in everyone's ears all the time.

Would it be possible to error out a build when someone changed any n_values field in Histograms.json?

Comment 1

[Georg Fritzsche [:gfritzsche]]

This probably would require to (at build time) compare the remote and local Histograms.json file versions and check for differences in specific values.

Comment 2

[Nick Alexander :nalexander [he/him]]

I'm not sure this is something you want to self serve, but I think the key words are "hg hooks", and the documentation at http://mozilla-version-control-tools.readthedocs.io/en/latest/hgmods/hooks.html will get you started.

I'm concerned that "remote [...] Histograms.json" introduces a dependency between hg/autoland and the data ingestion pipeline, but perhaps I'm not well informed, or we can cache our way around the dependency, etc.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Commit hooks were being proposed in bug 1261865, but if we'd be able to do this in the client build, this would be much better.
Per Chris, there are commands like "mach lint --outgoing", so we may be able to do this client-side?

With "remote" here i just meant the status vs. the "remote repository" - e.g. differences between local and the mozilla-central source state.
There are no pipeline dependencies proposed here.

Comment 4

[(not currently active) Ted Mielczarek]

How is the build system to know what the proper value of n_values is?

Comment 5

[Chris H-C :chutten]

The proper value is whatever it was before someone tried to change it. So... whatever the value was (if present) in the m-c state the user is working atop.

Comment 6

[Nick Alexander :nalexander [he/him]]

(In reply to Chris H-C :chutten from comment #5)
> The proper value is whatever it was before someone tried to change it. So...
> whatever the value was (if present) in the m-c state the user is working
> atop.

One wrinkle: the underlying VCS can be hg or git.  Artifact builds do the "walk backwards to find upstream" (with wrinkles related to the pushlog) -- perhaps this code or approach can be shared to find "upstream".  (It might just be the first _public_ ancestor in hg; but I don't know what that looks like in git.)

In general, having the build system know about the VCS state is an undesirable coupling, so I'd much prefer this to be handled by _local_ commit hooks (and remote commit hooks, I guess) rather than baking knowledge about VCS previous states into the build system.

Comment 7

[Chris H-C :chutten]

The work done in mach lint[1] has some of that coupling[2] which could provide the scaffolding for such a tool.

(May I officially applaud/accuse whoever's responsible for the LintRoller[3] type?)

[1]: http://searchfox.org/mozilla-central/source/python/mozlint/mozlint/cli.py#46
[2]: http://searchfox.org/mozilla-central/source/python/mozlint/mozlint/vcs.py
[3]: http://searchfox.org/mozilla-central/source/python/mozlint/mozlint/roller.py

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Nick Alexander :nalexander from comment #6)
> In general, having the build system know about the VCS state is an
> undesirable coupling, so I'd much prefer this to be handled by _local_
> commit hooks (and remote commit hooks, I guess) rather than baking knowledge
> about VCS previous states into the build system.

Local commit hooks could be great too.
Do we have any pattern to do them across hg & git?

Or is it feasible to use the work from comment 7 on the build side?

The earlier in the development process we can catch these issues, the better.
If it's not sensible to do it locally, we could fall back to remote commit hooks (bug 1261865).

Comment 9

[Gregory Szorc [:gps]]

The only ways you absolutely detect this are a commit hook on hg.mozilla.org that rejects pushes or CI that is VCS aware and can detect bad changes post commit. I'm not sure if we have anything of the latter. Although automation scheduling is aware of the "relevant" changesets and files for a given push/scheduling run. It could feed this metadata to a task, which could then call into VCS and compare old and new file content.

In either case, you want a local lint (probably integrated with VCS hooks) to detect this as early as possible during development. That almost certainly means writing some Python code that takes a string of old and new content and tells you if things are acceptable. You then plug that generic function into linters, hooks, etc.