Skip to content

Migrate ipstats from cron to graphite #7745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 6, 2023
Merged

Migrate ipstats from cron to graphite #7745

merged 4 commits into from
Apr 6, 2023

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Mar 29, 2023 •
edited
Loading

Closes #7622

Related to:

  1. Query a graphite endpoint to get the number of unique visitors (IP addresses) for the past 60 days.
  2. Store the data points in a Python dict by {datetime: count, ...}
  3. KeyError will be caught and printer if a requested datetime is not in that dict.

Should we also remove this occurrence of "ipstats"?

openlibrary/openlibrary/admin/stats.py

Lines 191 to 193 in 201ba7e

if numbers.sqlitefile:
logger.info("Removing sqlite file used for ipstats")
os.unlink(numbers.sqlitefile)

Technical

Testing

Screenshot

Stakeholders

@cclauss cclauss added Priority: 1 Do this week, receiving emails, time sensitive, . [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed] Affects: Operations Affects the IA DevOps folks labels Mar 29, 2023
@cclauss cclauss requested review from hornc and cdrini March 29, 2023 17:04
@cclauss cclauss force-pushed the fix-unique-visitors branch from 27abb4b to 6283087 Compare March 29, 2023 20:34
@cclauss cclauss marked this pull request as ready for review March 30, 2023 09:45
@cclauss cclauss mentioned this pull request Mar 30, 2023
Closed
28 tasks
@cclauss cclauss marked this pull request as draft March 30, 2023 10:33
@cclauss cclauss force-pushed the fix-unique-visitors branch from e29acb2 to 294b83c Compare March 30, 2023 14:15
@cclauss cclauss changed the title Fix count_unique_ips_for_day() Migrate ipstats from cron to graphite Mar 30, 2023
@cclauss cclauss marked this pull request as ready for review March 30, 2023 16:10
mekarpeles
mekarpeles approved these changes Mar 30, 2023
View reviewed changes
Copy link
Member

@mekarpeles mekarpeles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, reviewed with @cclauss -- one Q: db file deletion/unlink for @cdrini

cdrini
cdrini reviewed Mar 31, 2023
View reviewed changes
@cclauss cclauss force-pushed the fix-unique-visitors branch from d008e4f to c2f8ac0 Compare April 1, 2023 21:33
@cclauss cclauss requested a review from cdrini April 1, 2023 21:40
hornc
hornc approved these changes Apr 3, 2023
View reviewed changes
Copy link
Collaborator

@hornc hornc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@internetarchive internetarchive deleted a comment Apr 3, 2023
@cclauss cclauss force-pushed the fix-unique-visitors branch from f23250f to e6f8797 Compare April 4, 2023 18:52
@cclauss cclauss mentioned this pull request Apr 4, 2023
Closed
@mekarpeles
Copy link
Member

I'm on with @cclauss right now for 1:1 and he's trying to get unblocked on PRs -- he told me he already received feedback + implemented and I believe the code looks good and shouldn't have major implications. Re-assigning and merging to help him out.

@mekarpeles mekarpeles merged commit b51e02b into internetarchive:master Apr 6, 2023
@cclauss cclauss deleted the fix-unique-visitors branch April 6, 2023 16:22
Eds-Dbug pushed a commit to Eds-Dbug/openlibrary that referenced this pull request Apr 28, 2023
@cclauss @Eds-Dbug
Migrate ipstats from cron to graphite (internetarchive#7745)
ff5d1f8 
* Simplify get_visitors_per_day() logic
* remove sqlfile logic
* Bring back the VisitorStats class
* Cache function calls for 5 minutes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hornc hornc hornc approved these changes

@mekarpeles mekarpeles mekarpeles approved these changes

@cdrini cdrini Awaiting requested review from cdrini

Assignees

@cdrini cdrini

Labels
Affects: Operations Affects the IA DevOps folks Priority: 1 Do this week, receiving emails, time sensitive, . [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove ipstats, use graphite instead
4 participants
@cclauss @mekarpeles @hornc @cdrini