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

adds view counts to many objects on site, so we can see what items are popular when #6692

Merged
merged 12 commits into from
May 27, 2020

Conversation

owocki
Copy link
Contributor

@owocki owocki commented May 20, 2020

Description

adds view counts to many objects on site, so we can see what items are popular when

this is important bc it allows us to provide analytics data to users about whats popular, and visualize the funnel

Refers/Fixes

https://gitcoincore.slack.com/archives/CBWA6A75M/p1590015181146800

Testing

tested locally, bounties, hackathons, kudos, grants all load

key = f"{self.content_type}_{self.pk}"
return key

def increment_view_count(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gitcoinco/engineers im consdidering shifting from storing as a total count in redis, to a ObjectView django database object, which would allow us to

  1. store time series data
  2. also keep track of who views what (useful for segmenting purposes)
  3. differentiate index page views (like on /grants) from specific viewings of individual objects (like /grants/35/sustainability fund)

any objection if i do this? seems like the only counter-point would be DB bloat.

Copy link
Contributor

Choose a reason for hiding this comment

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

There will no doubt be a performance impact in reading and tallying this data - if you want to store that data for later data analysis I think it should be done in parallel with storing the total count in redis. Only the redis count should ever be read from during a web request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea.

as a rule, i think we shouldnt write to the DB from most HTTP requests. factoring into a task is almost always a good idea

@codecov
Copy link

codecov bot commented May 20, 2020

Codecov Report

Merging #6692 into master will increase coverage by 0.39%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6692      +/-   ##
==========================================
+ Coverage   26.75%   27.15%   +0.39%     
==========================================
  Files         293      292       -1     
  Lines       27538    27237     -301     
  Branches     4069     4000      -69     
==========================================
+ Hits         7369     7397      +28     
+ Misses      19902    19567     -335     
- Partials      267      273       +6     
Impacted Files Coverage Δ
app/dashboard/helpers.py 14.19% <0.00%> (-0.07%) ⬇️
app/dataviz/d3_views.py 10.10% <ø> (ø)
app/townsquare/views.py 9.86% <ø> (ø)
app/dashboard/router.py 40.54% <20.00%> (-0.36%) ⬇️
app/dashboard/views.py 10.95% <20.00%> (+0.01%) ⬆️
app/grants/views.py 16.42% <20.00%> (+0.02%) ⬆️
app/kudos/views.py 14.45% <20.00%> (+0.06%) ⬆️
app/economy/models.py 53.48% <26.08%> (-5.95%) ⬇️
app/dashboard/tasks.py 36.98% <35.71%> (-1.35%) ⬇️
app/grants/admin.py 43.70% <50.00%> (+0.08%) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e4d789...0c1f257. Read the comment docs.

:param content_type:
:return:
"""
redis = RedisService().redis
Copy link
Contributor

Choose a reason for hiding this comment

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

A good opportunity here to optimize performance is to swap out the redis_service.py code we use for just using a proper connection pool

@owocki
Copy link
Contributor Author

owocki commented May 21, 2020

82ab79f adds time series and user specific data to this PR.

marking as rdy for review

@owocki owocki marked this pull request as ready for review May 21, 2020 18:14
@owocki
Copy link
Contributor Author

owocki commented May 26, 2020

rdy for review

@danlipert danlipert merged commit 5c21326 into master May 27, 2020
@thelostone-mc thelostone-mc deleted the kevin/view_counts branch June 27, 2020 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants