Page MenuHomeSoftware Heritage

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

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

Details

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
ardumont updated this revision to Diff 6192.Aug 10 2019, 11:33 PM
  • script: Make the script parametric
  • export: Make the command reuse the manifest's parameters
  • export_archive_counters: Update required python dependencies
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
25

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
25

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 ;)

ftigeot accepted this revision.Aug 26 2019, 3:44 PM

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.

ardumont updated this revision to Diff 6421.Aug 26 2019, 3:55 PM

Rebase and adapt according to review

This revision was automatically updated to reflect the committed changes.