User counters using PostgreSQL JSON #5373

rhymes · 2020-01-06T12:31:09Z

What type of PR is this? (check all applicable)

Description

This PR is related to a discussion we had in #5310 about creating a proof of concept of how counters could work in a JSONB column in PostgreSQL.

A few details:

users has a 1:1 relationship with user_counters
counters can be accessed both as user.counters.data[:comments_these_7_days] or user.counters.comments_this_7_days both in read and write
there's a generic index on the JSONB column, indexes can be adjusted on specific paths if needed, but for now it should be enough, various types of indexes have different behaviors on JSONB columns.

Example of the column content (after running the included task):

PracticalDeveloper_development=# select id, user_id, data from user_counters;
 id | user_id |                           data
----+---------+----------------------------------------------------------
  1 |       1 | {"comments_prior_7_days": 0, "comments_these_7_days": 4}
  2 |       2 | {"comments_prior_7_days": 0, "comments_these_7_days": 3}
  3 |       3 | {"comments_prior_7_days": 0, "comments_these_7_days": 3}
  4 |       4 | {"comments_prior_7_days": 0, "comments_these_7_days": 2}
  5 |       5 | {"comments_prior_7_days": 0, "comments_these_7_days": 2}
  6 |       6 | {"comments_prior_7_days": 0, "comments_these_7_days": 3}
  7 |       7 | {"comments_prior_7_days": 0, "comments_these_7_days": 1}
  8 |       8 | {"comments_prior_7_days": 0, "comments_these_7_days": 1}
  9 |       9 | {"comments_prior_7_days": 0, "comments_these_7_days": 6}
 10 |      10 | {"comments_prior_7_days": 0, "comments_these_7_days": 5}

Related Tickets & Documents

#5310

rhymes

Left a few comments :)

rhymes · 2020-01-06T12:32:54Z

app/models/user.rb

@@ -148,6 +149,16 @@ class User < ApplicationRecord

  scope :dev_account, -> { find_by(id: ApplicationConfig["DEVTO_USER_ID"]) }

+  scope :with_this_week_comments, lambda { |number|


I included a few scopes as an example on how to use JSONB operators for queries

From a file length perspective, how do you feel about maybe moving all scopes to a separate file one way or another? If we lean in here I could see this getting long.

Oh I personally like the idea of at least counter scopes and even related methods being separate! We did something similar at Kenna with a model that had extensive status options and it worked out really well. Very easy to track down anything status-related bc it was all contained in a single file.

I'm also down for organizing files in a sane way, some of our models are way too big!

rhymes · 2020-01-06T12:33:33Z

app/models/user_counter.rb

+class UserCounter < ApplicationRecord
+  belongs_to :user
+
+  serialize :data, HashSerializer


This takes care of the conversion between hash and JSON

rhymes · 2020-01-06T12:34:17Z

app/models/user_counter.rb

+  belongs_to :user
+
+  serialize :data, HashSerializer
+  store_accessor :data, :comments_these_7_days, :comments_prior_7_days


This creates the accessor that allows us to do:

user.counters.comments_these_7_days

in addition to

user.counters.data[:comments_these_7_days]

rhymes · 2020-01-06T12:34:30Z

app/models/user_counter.rb

+  validates :comments_these_7_days, numericality: { only_integer: true }
+  validates :comments_prior_7_days, numericality: { only_integer: true }


we can add basic validators to the json fields

rhymes · 2020-01-06T12:35:00Z

db/migrate/20200106074859_create_user_counters.rb

+class CreateUserCounters < ActiveRecord::Migration[5.2]
+  def change
+    create_table :user_counters do |t|
+      t.references :user, index: { unique: true }


unique to make sure that we only have 1 row per user

mstruve

This is definitely the kind of data I would store in Elasticsearch but I love this postgres approach. We will see how it scales and once ES is up and running if we want to do performance tests and comparisons we can.

mstruve · 2020-01-06T17:08:41Z

lib/tasks/counters.rake

+  task update_users: :environment do
+    ActiveRecord::Base.transaction do
+      # NOTE: we could bypass Rails by using SQL directly to compute the counts and update these columns
+      User.includes(:counters).find_each do |user|


We should log the duration of this in Datadog so that we can track how long it takes and if it starts taking a really long time, then we might want to skip the Rails interface in an attempt to speed it up.

How often do you plan on running this?

I don't know yet, I copied the idea of the task from #5310 :D

It really is a POC for now

I think this can be run daily, or maybe split it up into sections and run it more often.

I see this as only having to be as accurate as we need it to be so we can play around with this.

It immediately enables us to ask the question "how many people had more comments this week than last?" and I think we can flexibly tie the functionality to the questions we want to ask and how often we want to ask them.

As a proof of concept this all looks good to me @rhymes.

@mstruve when we get up and running with Elastic we can start defining the useful borders of where to use each, and maybe we won't need this at all.

It immediately enables us to ask the question "how many people had more comments this week than last?" and I think we can flexibly tie the functionality to the questions we want to ask and how often we want to ask them.

This can probably be asked with a single SQL query :) We should probably start thinking about storing these questions in dedicated query objects outside the perimeter of the model, since they will mostly likely be used only for reports or dashboards

@mstruve how do we log the duration? :-) I'm not familiar with the DD library

Currently, we do not log the duration of the rake tasks bc we have turned off Honeycomb and Datadog for the schedulers. If you want to track the duration of this task I would put it in a worker and kick off the worker from the scheduler.

Let's move this to a Sidekiq worker when we have a chance so that we can track how long it takes and if we want send some of the count data to Datadog.

rhymes

Let me know what we want to do with this PR, it was meant as a POC but if we like this approach we can get it merged and then add the fields to it as we review the user counters code around the codebase

cc @mstruve @benhalpern

rhymes · 2020-01-20T12:54:47Z

lib/tasks/counters.rake

+  task update_users: :environment do
+    ActiveRecord::Base.transaction do
+      # NOTE: we could bypass Rails by using SQL directly to compute the counts and update these columns
+      User.includes(:counters).find_each do |user|


@mstruve how do we log the duration? :-) I'm not familiar with the DD library

mstruve

I'm game to give this a shot! Want to pull the WIP off the title?

mstruve · 2020-01-31T20:50:46Z

lib/tasks/counters.rake

+  task update_users: :environment do
+    ActiveRecord::Base.transaction do
+      # NOTE: we could bypass Rails by using SQL directly to compute the counts and update these columns
+      User.includes(:counters).find_each do |user|


Let's move this to a Sidekiq worker when we have a chance so that we can track how long it takes and if we want send some of the count data to Datadog.

rhymes added 6 commits January 6, 2020 09:58

Add UserCounter model and its table

ef6715d

Add validators and specs

197fabe

Add counters task

0b1f6a4

Add additional counter

bf77a5f

Add top commenters scope to User

a86385d

Fix grammar

45dbccf

rhymes requested a review from a team January 6, 2020 12:31

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 6, 2020

rhymes commented Jan 6, 2020

View reviewed changes

rhymes changed the title ~~[POC] User counters using in PostgreSQL json~~ [WIP] [POC] User counters using in PostgreSQL json Jan 6, 2020

pr-triage bot removed the PR: unreviewed bot applied label for PR's with no review label Jan 6, 2020

rhymes mentioned this pull request Jan 6, 2020

[WIP] Create SortableCount model for better rankable data caches #5310

Closed

5 tasks

Fix specs

2c7458b

mstruve reviewed Jan 6, 2020

View reviewed changes

benhalpern and others added 2 commits January 9, 2020 10:42

Merge branch 'master' into rhymes/users_counters_jsonb

ac33fa8

Merge branch 'master' into rhymes/users_counters_jsonb

9ca371a

rhymes commented Jan 20, 2020

View reviewed changes

Merge branch 'master' into rhymes/users_counters_jsonb

7afba36

mstruve requested review from joshpuetz and nickytonline January 31, 2020 20:09

mstruve approved these changes Jan 31, 2020

View reviewed changes

rhymes changed the title ~~[WIP] [POC] User counters using in PostgreSQL json~~ User counters using in PostgreSQL json Feb 1, 2020

pr-triage bot added the PR: reviewed-approved bot applied label for PR's where reviewer approves changes label Feb 1, 2020

rhymes changed the title ~~User counters using in PostgreSQL json~~ User counters using PostgreSQL JSON Feb 1, 2020

benhalpern approved these changes Feb 1, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

mstruve merged commit a1835ae into forem:master Feb 3, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 3, 2020

rhymes deleted the rhymes/users_counters_jsonb branch February 3, 2020 15:36

This was referenced Feb 3, 2020

Replace the initial migration for strong_migrations gem #5865

Merged

Have counters:update_users use a transaction per user and customizable batch size #5912

Merged

weekly-digest bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #5980

Closed

rhymes mentioned this pull request Jun 12, 2020

Remove user_counters table #8445

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User counters using PostgreSQL JSON #5373

User counters using PostgreSQL JSON #5373

rhymes commented Jan 6, 2020

rhymes left a comment

rhymes Jan 6, 2020

benhalpern Jan 6, 2020

mstruve Jan 6, 2020

rhymes Jan 7, 2020

rhymes Jan 6, 2020

rhymes Jan 6, 2020

rhymes Jan 6, 2020

rhymes Jan 6, 2020

mstruve left a comment

mstruve Jan 6, 2020

mstruve Jan 6, 2020

rhymes Jan 6, 2020

benhalpern Jan 9, 2020

rhymes Jan 9, 2020

rhymes Jan 20, 2020

mstruve Jan 20, 2020

mstruve Jan 31, 2020

rhymes left a comment •

edited

Loading

rhymes Jan 20, 2020

mstruve left a comment

mstruve Jan 31, 2020

		@@ -148,6 +149,16 @@ class User < ApplicationRecord

		scope :dev_account, -> { find_by(id: ApplicationConfig["DEVTO_USER_ID"]) }

		scope :with_this_week_comments, lambda { \|number\|

		validates :comments_these_7_days, numericality: { only_integer: true }
		validates :comments_prior_7_days, numericality: { only_integer: true }

User counters using PostgreSQL JSON #5373

User counters using PostgreSQL JSON #5373

Conversation

rhymes commented Jan 6, 2020

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

rhymes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhymes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhymes left a comment •

edited

Loading