Rename "histogram_tools.py" to "parse_histograms.py"

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dexter, Assigned: leakey94, Mentored)

Tracking

(Blocks 1 bug)

Trunk
mozilla59
Points:
1

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Renaming this file [1] may look trivial, but there are some interesting dependencies: some upstream jobs (TMO aggregator?) might be fetching a copy of this file to do the parsing. As a consequence, when renaming this file, we should also point the dependent jobs to the new file name.

[1] - https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/toolkit/components/telemetry/histogram_tools.py
(Reporter)

Updated

2 years ago
Blocks: 1201022
Points: --- → 1
Priority: -- → P3
(Reporter)

Comment 1

2 years ago
Do you know if TMO (or any other project like the regression detector) fetches a copy of histogram_tools.py from mozilla-central?
Flags: needinfo?(chutten)
python_moztelemetry sorta does. It vendors a copy in using this script: https://github.com/mozilla/python_moztelemetry/blob/master/bin/update_histogram_tools The script will need updating, but as it appears to be run manually its failures would be noticed and rectified.

The aggregator uses moztelemetry's version: https://github.com/mozilla/python_mozaggregator/search?utf8=%E2%9C%93&q=histogram&type=

telemetry-batch-view doesn't use it.

mozilla/moztelemetry seems to not use it, though I'm not 100% sure.

ni?frank for any projects I'm missing.
Flags: needinfo?(chutten) → needinfo?(fbertsch)
Yup, the only dependency server-side should be python_moztelemetry; everywhere else should fetch it from there. We don't retrieve it in any Scala code, the histogram bucket generation is done separately in t-b-v.
Flags: needinfo?(fbertsch)
(Reporter)

Comment 4

2 years ago
Hey Ryan, would you be interested in taking this one as well? It is slightly more involved than the previous one since it has a depedency.
Flags: needinfo?(leakey94)
(Assignee)

Comment 5

2 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #4)
> Hey Ryan, would you be interested in taking this one as well? It is slightly
> more involved than the previous one since it has a depedency.

Sure Alessio, I'd like to give it a go.
Flags: needinfo?(leakey94) → needinfo?(alessio.placitelli)
(Reporter)

Updated

2 years ago
Assignee: nobody → leakey94
Flags: needinfo?(alessio.placitelli)
(Reporter)

Updated

2 years ago
Mentor: alessio.placitelli
(Assignee)

Comment 6

2 years ago
Once "histogram_tools.py" has been renamed and all textual occurrences have been updated, how can I update the python_moztelemetry script to reflect the changes?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 7

2 years ago
(In reply to Ryan Leake from comment #6)
> Once "histogram_tools.py" has been renamed and all textual occurrences have
> been updated, how can I update the python_moztelemetry script to reflect the
> changes?

You can file a PR against https://github.com/mozilla/python_moztelemetry/ to change these files: https://github.com/mozilla/python_moztelemetry/search?utf8=%E2%9C%93&q=histogram_tools.py&type=
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 8

2 years ago
Thanks Alessio. Do the files in python_moztelemetry need renaming themselves( e.g. update_histogram_tools renamed to update_parse_histogram)?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 9

2 years ago
(In reply to Ryan Leake from comment #8)
> Thanks Alessio. Do the files in python_moztelemetry need renaming
> themselves( e.g. update_histogram_tools renamed to update_parse_histogram)?

Yes please. Let's also change the README file if needed.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 10

2 years ago
Do the changes to python_moztelemetry look ok? https://github.com/RyanLeake/python_moztelemetry/commit/c4265ddd79a908d1e8486dd910e7ff663b4b6544
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 11

2 years ago
(In reply to Ryan Leake from comment #10)
> Do the changes to python_moztelemetry look ok?
> https://github.com/RyanLeake/python_moztelemetry/commit/
> c4265ddd79a908d1e8486dd910e7ff663b4b6544

From a quick look they make sense. Let's make a PR so that I can take a better look :-)
Flags: needinfo?(alessio.placitelli)
Comment hidden (mozreview-request)
(Reporter)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8931640 [details]
Bug 1419761 - Rename 'histogram_tools.py' to 'parse_histograms.py'.

https://reviewboard.mozilla.org/r/202808/#review208136

This looks good, thank you!
Attachment #8931640 - Flags: review?(alessio.placitelli) → review+
(Reporter)

Comment 15

2 years ago
Ryan, looks like the changes are failing code linting:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=194823f0a3ba65b1343f99ce383aceac275c5ebf&selectedJob=147413387

Could you please fix it and update your patch? If you want to check for linting errors locally, you can run the following command:

./mach lint -l flake8 toolkit/components/telemetry
Flags: needinfo?(leakey94)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> Ryan, looks like the changes are failing code linting:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=194823f0a3ba65b1343f99ce383aceac275c5ebf&selectedJob=1
> 47413387
> 
> Could you please fix it and update your patch? If you want to check for
> linting errors locally, you can run the following command:
> 
> ./mach lint -l flake8 toolkit/components/telemetry

Should be good to go now!
Flags: needinfo?(leakey94) → needinfo?(alessio.placitelli)
(Reporter)

Comment 18

2 years ago
Thank you!
Flags: needinfo?(alessio.placitelli)

Comment 19

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c5d69465077c
Rename 'histogram_tools.py' to 'parse_histograms.py'. r=Dexter

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5d69465077c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 21

2 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #18)
> Thank you!

DO YOU HAVE SOME EASY BUGS
I KNOW JAVA, C, A LITTLE ANDROID
(Reporter)

Comment 22

2 years ago
(In reply to Dailin Zhang from comment #21)
> (In reply to Alessio Placitelli [:Dexter] from comment #18)
> > Thank you!
> 
> DO YOU HAVE SOME EASY BUGS
> I KNOW JAVA, C, A LITTLE ANDROID

Please have a look at http://www.joshmatthews.net/bugsahoy/ or http://whatcanidoformozilla.org/
You need to log in before you can comment on or make changes to this bug.