stats_exporter: Refactor and add docstrings to graph data export script
ClosedPublic
Actions

Authored by ardumont on Aug 10 2019, 6:03 PM.

Details

Reviewers

ftigeot
vlorentz

Group Reviewers

Reviewers

Commits

rSPSITE4bc0f1926fb2: script: Make the script parametric
rSPSITEccdc9e0e2a40: export_archive_counters: Update required python dependencies
rSPSITE17fa041a6d66: stats_exporter: Refactor and add docstrings to export script
rSPSITE55f052b6bcc6: export-stats: Rename script with consistent _ for python filename
rSPSITEb75f6e58b2ea: export: Make the command reuse the manifest's parameters

Summary

Now that the script is ok, i thought it was the right time to improve it a bit.

So here it goes:

refactor a bit the script
Add docstrings and comments to clarify some part
simplify the data computation (no if, simply inline the call for history data solely for content).
Make the script parametric to reuse the setup from the manifest (avoiding future problems, hard-coding values in the long run is not what we want)

This also:

export-stats: Rename script with consistent _ for python filename
extract variables from puppet manifest to avoid duplication
export_archive_counters: Update required python dependencies (requests was not mentioned)

Test Plan

prior version script run.
current version script run.
Check output are the same and it is.

$ python ./export-archive_counters.py > test.origin.json
$ python ./export_archive_counters.py --history-data-file ./history-counters.munin.json > test.json
$ stat test.json
  File: test.json
  Size: 85468           Blocks: 168        IO Block: 4096   regular file
Device: fe02h/65026d    Inode: 43916075    Links: 1
Access: (0644/-rw-r--r--)  Uid: ( 1000/    tony)   Gid: ( 1000/    tony)
Access: 2019-08-10 23:32:28.555767531 +0200
Modify: 2019-08-10 23:31:41.050503170 +0200
Change: 2019-08-10 23:31:41.050503170 +0200
 Birth: -
$ diff test.json test.origin.json  # <- no diff
$ sha512sum test.json
147670ea7b4d8d388adfcf09586e5c143113d26f6eee265ba2d0b88185b3809b6429668c3ba96a46829a68149a2355ff069e8018935eebc2ebd923069e2f17bb  test.json
$ sha512sum test.origin.json
147670ea7b4d8d388adfcf09586e5c143113d26f6eee265ba2d0b88185b3809b6429668c3ba96a46829a68149a2355ff069e8018935eebc2ebd923069e2f17bb  test.origin.json

Diff Detail

Repository

rSPSITE puppet-swh-site

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Aug 10 2019, 6:03 PM

Harbormaster completed remote builds in B7219: Diff 6191.Aug 10 2019, 6:03 PM

script: Make the script parametric
export: Make the command reuse the manifest's parameters
export_archive_counters: Update required python dependencies

Harbormaster completed remote builds in B7220: Diff 6192.Aug 10 2019, 11:33 PM

ardumont edited the test plan for this revision. (Show Details)Aug 10 2019, 11:36 PM

ardumont edited the test plan for this revision. (Show Details)

ardumont edited the summary of this revision. (Show Details)

ardumont retitled this revision from stats_exporter: Refactor and add docstrings to export script to stats_exporter: Refactor and add docstrings to graph data export script.Aug 10 2019, 11:38 PM

vlorentz accepted this revision.Aug 12 2019, 1:25 PM

vlorentz added a subscriber: vlorentz.

vlorentz added inline comments.

site-modules/profile/files/stats_exporter/export_archive_counters.py
78–79 ↗	(On Diff #6192)	Nitpick: result = [adapt_format(i) for i in data['data']['result'][0]['values']]
92 ↗	(On Diff #6192)	I don't think there should be a default.
site-modules/profile/manifests/export_archive_counters.pp
24	Is this the right place for a data file? (although it's immutable)

This revision is now accepted and ready to land.Aug 12 2019, 1:25 PM

ardumont edited the test plan for this revision. (Show Details)Aug 12 2019, 4:50 PM

ardumont added inline comments.Aug 26 2019, 3:11 PM

site-modules/profile/files/stats_exporter/export_archive_counters.py
78–79 ↗	(On Diff #6192)	yes, more readable, thx
92 ↗	(On Diff #6192)	Yep, i forgot to remove those after the last commit.
site-modules/profile/manifests/export_archive_counters.pp
24	i did not think this through, i kept the original value. What you say makes sense but i don't know where to put that anyway ;)

The changes look fine to me.
The important thing is to get the same output.
With regard to the data directory, share/swh-data seemed to be a logical place and compatible with the usual hier(7) filesystem hierarchy.

With regard to the data directory, share/swh-data seemed to be a logical place and compatible with the usual hier(7) filesystem hierarchy.

Thanks for the reference!

Quoting the man page:

/usr/local/share
       Local application data that can be shared among different architectures of the same OS.

So it seems fine enough there indeed.

Rebase and adapt according to review

Harbormaster completed remote builds in B7461: Diff 6421.Aug 26 2019, 3:55 PM

Closed by commit rSPSITEccdc9e0e2a40: export_archive_counters: Update required python dependencies (authored by ardumont). · Explain WhyAug 26 2019, 3:58 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

site-modules/

profile/

manifests/

export_archive_counters.pp

2 lines

Diff 6422

View Options

site-modules/profile/manifests/export_archive_counters.pp

stats_exporter: Refactor and add docstrings to graph data export scriptClosedPublicActions