Page MenuHomeSoftware Heritage
Differential D1841

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
Branch
staging
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7461
Build 10642: arc lint + arc unit

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

Nitpick:

result = [adapt_format(i)
          for i in data['data']['result'][0]['values']]
92

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

yes, more readable, thx

92

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

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.

ardumont added a comment.Aug 26 2019, 3:50 PM

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

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

PathSize
site-modules/
profile/
files/
stats_exporter/
117 lines
manifests/
25 lines

Diff 6421

View Options

site-modules/profile/files/stats_exporter/export-archive_counters.py

Loading...
View Options

site-modules/profile/files/stats_exporter/export_archive_counters.py

Loading...
View Options

site-modules/profile/manifests/export_archive_counters.pp

Loading...
About · Developer information · Legal