Skip to content
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.

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

Add rubygem download ranking #3812

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

juankuquintana
Copy link
Contributor

This PR adds a ranking based on downloads for rubygems and displays it on the rubygems page, below the amount of total downloads. It caches the result once, and refreshes it every 24 hours, if the rubygem is not ranked yet (i.e. is new), it will refresh the ranking.

Testing

  1. Go to a gems show page
  2. See the rank below the amount of downloads
Screen Shot 2023-05-20 at 21 33 00 Screen Shot 2023-05-20 at 21 32 46

Closes #3658

@juankuquintana juankuquintana force-pushed the add-rubygem-download-rank branch 3 times, most recently from a946093 to eeff20f Compare May 21, 2023 09:02
@simi simi self-requested a review May 21, 2023 15:47
@simi
Copy link
Member

simi commented May 21, 2023

Hello @juankuquintana! Thanks for opening PR with this feature. I was thinking about the implementation. If I understand it well, currently there is output of related query transformed into hash with rubygem_id and position values. This hash is cached for 24 hours. I have 2 suggestions to improve this.

There is no need to cache those values for 24 hours. GemDownload changes in FastlyLogProcessor. It could be possible to recalculate ranking in there as well during log processing. That way number of gem downloads would be in sync with ranking.

Each page render needs to get whole hash from Rails.cache with ~180k entries to find out 1 related value to render the page. Wouldn't be better performance wise to store ranked gems somehow it would be able to query just for related gem rank easily without fetching ranks of all gems? Maybe add new table for this purpose? Ideally it could be materialized view just refreshed as part of FastlyLogProcessor, but there is no way to manage DB views in app now (even there are some nice gems like scenic).

Btw. feel free to open styling related changes in separate PR.

PS: 🤔 This could be added to API into rubygem payload next to downloads as well. But that could be added in following PR once this is merged.

@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Merging #3812 (88a569e) into master (75540cb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3812   +/-   ##
=======================================
  Coverage   98.81%   98.82%           
=======================================
  Files         214      214           
  Lines        5244     5255   +11     
=======================================
+ Hits         5182     5193   +11     
  Misses         62       62           
Impacted Files Coverage Δ
app/models/rubygem.rb 98.76% <100.00%> (+0.05%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Show rank for number of download on gem pages
2 participants