1282098 - Pull probe parsers out of m-c and into a separate package

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

Bug 1276190 adds parse_scalars.py, a script that adds scalar support as histogram_tools.py [2] does with scalars. They have a few functions in common that were put in shared_telemetry_utils.py.

We know that a.t.m.o uses histogram_tools.py to produce aggregates, through [1]. Unfortunately, could be other hidden services fetching histogram_tools.py and using the exposed API. For this reason, we cannot make it depend on another file without breaking these services.

We need a smarter way to handle this kind of situations. We could, for example, package histogra_tools.py, parse_scalars.py and shared_telemetry_utils.py in a python package and fetch it using pip. Unfortunately, we're not sure this could be used within our build system.

[1] - https://github.com/mozilla/python_moztelemetry/
[2] - https://dxr.mozilla.org/mozilla-central/rev/d1102663db10b3d4b9358f3cf4e16b7c56902352/toolkit/components/telemetry/histogram_tools.py

Comment 1

[Georg Fritzsche [:gfritzsche]]

We also have parse_events.py now.
This bug seems to have a few separate tasks, we should try to break it down.

Comment 3

[Alessio Placitelli [:Dexter]]

Hey :gps, is there any way to make the build system download a Python package before the build starts?
We're considering moving the parsers [1] to a separate Python package, so that upstream Telemetry dependencies can depend on this rather than importing the parsers themselves.

I'm not sure downloading a fresh package out of a public repo is a good idea (for security), but I'm not an expert of the build system.

A fallback option would be to still create a package, but to mirror the code here [2] whenever a new version of the Telemetry parsers package is released. Does this make more sense? Can you see any drawback about this other than the Telemetry team (us:D) forgetting to update the version in-tree?

[1] - https://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=telemetry%2Fparse_*.py
[2] - https://searchfox.org/mozilla-central/source/python/README

Comment 4

[Gregory Szorc [:gps]]

The Firefox build system should be hermetic and not make network requests. For Python dependencies, our policy is to vendor packages in the repo under third_party/python. It's worth noting we also have some Mozilla-authored Python packages that canonically live in mozilla-central and are published to PyPI. See every directory under testing/mozbase for example.

You can use `mach vendor python` to initiate a vendoring of a PyPI hosted package. See https://hg.mozilla.org/mozilla-central/rev/a566524dd127 for what other files you may want to change so the package is importable by the build system.

It's best to get a build peer review on any vendorings. But other people like ahal and davehunt can sign off on the vendoring bits that don't touch the build system. I've CCd them here in case they want to provide additional context.

Comment 5

[Alessio Placitelli [:Dexter]]

(In reply to Gregory Szorc [:gps] from comment #4)
> The Firefox build system should be hermetic and not make network requests.
> For Python dependencies, our policy is to vendor packages in the repo under
> third_party/python. It's worth noting we also have some Mozilla-authored
> Python packages that canonically live in mozilla-central and are published
> to PyPI. See every directory under testing/mozbase for example.
> 
> You can use `mach vendor python` to initiate a vendoring of a PyPI hosted
> package. See https://hg.mozilla.org/mozilla-central/rev/a566524dd127 for
> what other files you may want to change so the package is importable by the
> build system.
> 
> It's best to get a build peer review on any vendorings. But other people
> like ahal and davehunt can sign off on the vendoring bits that don't touch
> the build system. I've CCd them here in case they want to provide additional
> context.

Thanks :gps, that's very helpful! Given that, I think a way forward could be:

- Create a Github repo for the parsers.
- Enable CI on that.
- Enable automatic deployment on pypi from the "master" branch.
- Vendor the package in m-c (this requires a build peer to sign off).

Georg, does this sound like a plan? Can you see any drawback here or can we move on with this?

Comment 6

[Georg Fritzsche [:gfritzsche]]

That sounds solid to me.
Let's run this past some Python developer for feedback, maybe :jezdez?

Comment 7

[Alessio Placitelli [:Dexter]]

Jannis, does the plan in comment 5 sound good to you? We're planning on putting Telemetry parser in a separate pypi package so that upstream consumers (Telemetry pipeline services) can easily depend on that. Anything we should watch out for?

Comment 8

[Jannis Leidel [:jezdez]]

(In reply to Alessio Placitelli [:Dexter] from comment #7)
> Jannis, does the plan in comment 5 sound good to you? We're planning on
> putting Telemetry parser in a separate pypi package so that upstream
> consumers (Telemetry pipeline services) can easily depend on that. Anything
> we should watch out for?

Sounds good to me in general, although with the caveat we should follow best practices for Python package structure and setup, similar to how we've done for python_moztelemetry and python_mozaggregator.

Can you clarify if the extracted code could/should live in those packages to reduce redundancy, since any new package is a long-term investement in maintenance?

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Jannis Leidel [:jezdez] from comment #8)
> (In reply to Alessio Placitelli [:Dexter] from comment #7)
> > Jannis, does the plan in comment 5 sound good to you? We're planning on
> > putting Telemetry parser in a separate pypi package so that upstream
> > consumers (Telemetry pipeline services) can easily depend on that. Anything
> > we should watch out for?
> 
> Sounds good to me in general, although with the caveat we should follow best
> practices for Python package structure and setup, similar to how we've done
> for python_moztelemetry and python_mozaggregator.

Yes, I completely agree on this!

> Can you clarify if the extracted code could/should live in those packages to
> reduce redundancy, since any new package is a long-term investement in
> maintenance?

As of now, a number of services (probe-scraper [1], python_moztelemetry [2], ...) rely on the parsers which live in mozilla-central. The parsers are currently simply copied over the tree, when any update occurs. However, this approach can sometimes cause issues: if we forget to keep the version in sync, things might fail (and have failed in the past, see bug 1430115 et al).

We expect the number of services relying on the parsers to grow in the future, and we really need a better way to reduce the redundancy and keep things in sync (or, at least, to notify for updates!). For this reason I think the cost of maintaining a new package is balanced out by the time we loose each time to sync up the different copies of the parser or fix breakages :)

[1] - https://github.com/mozilla/probe-scraper/
[2] - https://github.com/mozilla/python_moztelemetry/

Comment 10

[Jannis Leidel [:jezdez]]

(In reply to Alessio Placitelli [:Dexter] from comment #9)

> > Can you clarify if the extracted code could/should live in those packages to
> > reduce redundancy, since any new package is a long-term investement in
> > maintenance?
> 
> As of now, a number of services (probe-scraper [1], python_moztelemetry [2],
> ...) rely on the parsers which live in mozilla-central. The parsers are
> currently simply copied over the tree, when any update occurs. However, this
> approach can sometimes cause issues: if we forget to keep the version in
> sync, things might fail (and have failed in the past, see bug 1430115 et al).

Gotcha, don't forget to open tickets in those other services/packages to use the new dependency.

> We expect the number of services relying on the parsers to grow in the
> future, and we really need a better way to reduce the redundancy and keep
> things in sync (or, at least, to notify for updates!). For this reason I
> think the cost of maintaining a new package is balanced out by the time we
> loose each time to sync up the different copies of the parser or fix
> breakages :)
> 
> [1] - https://github.com/mozilla/probe-scraper/
> [2] - https://github.com/mozilla/python_moztelemetry/

Understood, glad you found this maintenance risk! Since naming things is hard, let's stay consistent and call the library python_mozparsers maybe?

Comment 11

[Gregory Szorc [:gps]]

Unless you are targeting developing on GitHub and getting contributors on GitHub, I'd keep the source of truth in mozilla-central and release to PyPI from mozilla-central. Just like what mozbase does. Otherwise you are just creating headaches around synchronizing changes, forcing releases / vendoring due to minor changes, etc. That can add up to a bit of overhead.

Comment 12

[Georg Fritzsche [:gfritzsche]]

I think having it accessible on GitHub is one factor (i'm not sure how important of a factor it is though).
AFAICT, there are two main contributing parties here:
1) The Firefox Telemetry team, which does work in mozilla-central.
2) The Data platform team, which is not using mozilla-central and generally not set up for Firefox builds.

Maybe we should check back with the latter and confirm?

Comment 13

[Alessio Placitelli [:Dexter]]

Hey Mark and Frank, we're considering releasing the Telemetry parsers (the various parser_*.py scripts that currently live in the mozilla-central repo) as a PyPi package. We have two options for doing that (see comment 5 and comment 11):

1) Move the parsers to a GitHub repo, use that as the main development repo, and only vendor (read = import) the package in mozilla-central. This would be beneficial to developers not working on mozilla-central, as it wouldn't be required to actually change something in the parsers (or fix bugs :D).
2) Keep the development in mozilla-central and just release the scripts as PyPi packages so that upstream consumers can rely on that. This wouldn't make it easy for contribution from people who don't have a Firefox build locally.

Do you feel like (1) would be beneficial to you? Can you suggest anyone else who might have an opinion on this?

Comment 14

[Alessio Placitelli [:Dexter]]

From a quick discussion on IRC with Jan-Erik, there's another perspective to this:

> (github scenario, - option 1) if the parser is changed, but we forget to actually vendor the new version in time, then someone changes Histograms.yaml with incompatible data, we only catch that later when we update the vendored version; if it would be in-tree, we land that parser change and a change to histograms.yaml would fail

This makes me lean toward what :gps suggested: having this in tree and just pushed to pypi on updates.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #14)
> From a quick discussion on IRC with Jan-Erik, there's another perspective to
> this:
> 
> > (github scenario, - option 1) if the parser is changed, but we forget to actually vendor the new version in time, then someone changes Histograms.yaml with incompatible data, we only catch that later when we update the vendored version; if it would be in-tree, we land that parser change and a change to histograms.yaml would fail

Is that different from landing the changes in-tree?
Doesn't it just move adapting the Histograms.yaml to a later time instead of immediately?

On the package side, we need some form of backward compatibility to historic versions anyway.

Comment 16

[Georg Fritzsche [:gfritzsche]]

There are also two more kinds of probe registries that potentially live outside of the Firefox tree:
- Mobile probe registries.
- Add-on probe registries (not yet supported, but possibly in the future for e.g. Shield studies).

Comment 17

[Mark Reid [:mreid]]

Frank and I discussed this with the data platform team, and we would prefer the approach of keeping the code in mozilla-central and release to PyPI (Option 2 from Comment 13). This keeps the ownership/responsibility in Firefox, but makes it easier to manage dependencies from other projects. We don't see a need to contribute to that codebase outside of mozilla-central.

Comment 18

[Alessio Placitelli [:Dexter]]

(In reply to Mark Reid [:mreid] from comment #17)
> Frank and I discussed this with the data platform team, and we would prefer
> the approach of keeping the code in mozilla-central and release to PyPI
> (Option 2 from Comment 13). This keeps the ownership/responsibility in
> Firefox, but makes it easier to manage dependencies from other projects. We
> don't see a need to contribute to that codebase outside of mozilla-central.

I see, thanks for the feedback. Just to clarify: regardless of the location, the ownership of that would still be completely on the Telemetry team. In that regard, storage location and ownership are probably orthogonal problems IMHO.

(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> (In reply to Alessio Placitelli [:Dexter] from comment #14)
> > From a quick discussion on IRC with Jan-Erik, there's another perspective to
> > this:
> > 
> > > (github scenario, - option 1) if the parser is changed, but we forget to actually vendor the new version in time, then someone changes Histograms.yaml with incompatible data, we only catch that later when we update the vendored version; if it would be in-tree, we land that parser change and a change to histograms.yaml would fail
> 
> Is that different from landing the changes in-tree?
> Doesn't it just move adapting the Histograms.yaml to a later time instead of
> immediately?

Mh, you have a point here. If we're on github, update the parsers, and land that on m-c, we'd still run tests in-tree. So if there's any incompatible data (e.g. we reduce some limits for event strings), we would address that as we land the updated version of the parsers.

Comment 19

[Georg Fritzsche [:gfritzsche]]

(In reply to Mark Reid [:mreid] from comment #17)
> Frank and I discussed this with the data platform team, and we would prefer
> the approach of keeping the code in mozilla-central and release to PyPI
> (Option 2 from Comment 13). This keeps the ownership/responsibility in
> Firefox, but makes it easier to manage dependencies from other projects. We
> don't see a need to contribute to that codebase outside of mozilla-central.

I agree that ownership is in Firefox, specifically the Firefox Telemetry team.
Where the code lives seems orthogonal, but it is up to our teams process then.
We'll need to talk about how mobile will fit in here, then proceed from there.

Comment 20

[Alessio Placitelli [:Dexter]]

Hey Frank, Mark! Given the ownership clarification in comment 19 and comment 18, would you still strongly advise against having a github repo?

Comment 21

[Mark Reid [:mreid]]

I don't feel very strongly about it, I just don't see a ton of value in moving the canonical source out of mozilla-central. Regardless of where it is, the "backend" platform code will make use of the published package.

Having such a published package will definitely be useful, thanks!

Comment 22

[Frank Bertsch [:frank]]

(In reply to Alessio Placitelli [:Dexter] from comment #20)
> Hey Frank, Mark! Given the ownership clarification in comment 19 and comment
> 18, would you still strongly advise against having a github repo?

Given the ownership question is resolved it seems fine to have it be wherever the owners desire! No strong opinion here either.

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1282098 - Pull probe parsers out of m-c and into a separate package. r?chutten,gfritzsche

This patch moves the parsers' code to python_mozparsers, which is
the definitive name of the package on pypi. The data generation scripts
are adjusted accordingly. The README and setup files are added to
enable publishing on pypi.

Comment 24

[Alessio Placitelli [:Dexter]]

Here's the package's page, as shown on the test PyPi instance: https://test.pypi.org/project/python-mozparsers/

Comment 25

[Chris H-C :chutten]

Comment on attachment 9006589 [details]
Bug 1282098 - Pull probe parsers out of m-c and into a separate package. r?chutten,gfritzsche

Chris H-C :chutten has approved the revision.

Comment 26

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a9fdb694e311ebc3e240d240dbb04c489e89ca

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/28fb503aacbc
Pull probe parsers out of m-c and into a separate package. r=chutten

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/28fb503aacbc

Comment 29

[Alessio Placitelli [:Dexter]]

The published PyPi package lives at: https://pypi.org/project/python-mozparsers/

Comment 30

[Alessio Placitelli [:Dexter]]

The guide to publish or update the PyPi package lives here: https://packaging.python.org/tutorials/packaging-projects/

Comment 31

[Ben Campbell]

(In reply to Alessio Placitelli [:Dexter] from comment #29)

The published PyPi package lives at:
https://pypi.org/project/python-mozparsers/

Just for the benefit of anyone else landing here, I think the new package location is:
https://pypi.org/project/mozparsers/