Tag subscription counts wrong on stats page

[jywarren]

We just changed the query for this page and it looks like something went wrong, and perhaps the grouping is incorrect? There are so many tags with 219 followers and almost none w 1 follower.

https://publiclab.org/stats/subscriptions

Let's check the last change to that file and figure out what went wrong.

[dongskyler]

Why 219 subscribers?
Is 219 some special number in our code? Does it ring any bell?
Is the total number of subscribers 219?
Do you have any clues or guesses for debugging this issue?

[jywarren]

Hi @dongskyler! Sorry, i think we must've given you a mistake in our suggested code change! Thanks for looking into this! We SUPER appreciate your help, and please don't feel this was your fault in any way! 😄

Hmm. I wonder -- it's a pretty odd bug. The change was to add the .joins() statement on this line:

plots2/app/controllers/stats_controller.rb

Line 4 in 0e58e62

TagSelection.where(following: true).joins(:node_tags).each do |tag|

Is it possible we're joining incorrectly? What if we joined against something which has only 219 records... or something... ?

[jywarren]

TagSelection.count => 5006

So, it's not that. Could it be that... the returned records are not of the same type? So the logic in this block is not working as we had expected?

plots2/app/controllers/stats_controller.rb

Lines 2 to 9 in 0e58e62

	def subscriptions
	@tags = {}
	TagSelection.where(following: true).joins(:node_tags).each do |tag|
	@tags[tag.tagname] = @tags[tag.tagname] || 0
	@tags[tag.tagname] += 1
	end
	@tags = @tags.group_by { |_k, v| v / 10 }
	end

Let me test what TagSelection.where(following: true).joins(:node_tags) is versus TagSelection.where(following: true)...

[jywarren]

irb(main):003:0>  TagSelection.where(following: true).joins(:node_tags).last
=> #<TagSelection user_id: 994, tid: 13841, following: true, created_at: "2000-01-01 00:00:00", updated_at: "2019-06-15 17:58:42">
irb(main):004:0>  TagSelection.where(following: true).last
=> #<TagSelection user_id: 694777, tid: 25416, following: true, created_at: "2020-05-19 15:40:23", updated_at: "2020-05-19 15:40:23">
irb(main):005:0>  TagSelection.where(following: true).joins(:node_tags).count
=> 15755
irb(main):006:0>  TagSelection.where(following: true).count
=> 4366

Hmm. So the filtering works well, but, why is the logic not working to actually show counts? Let me try going through the logic step by step in the rails console.

[jywarren]

OK, so what I think is happening is that the relation is defined here:

plots2/app/models/tag_selection.rb

Line 4 in 0e58e62

has_many :node_tags, foreign_key: :tid

But, both tables have a tid column. So, it's actually fetching the node_tag with tid matching tag_selection.id? Or somehow that relation is wrong. I'm going to try manually writing the JOIN to test this theory: https://stackoverflow.com/questions/1509692/rails-activerecord-joins-with-left-join-instead-of-inner-join

[jywarren]

Aha!

I found that it's possible to use .group() to do this all in SQL!

TagSelection.where(following: true).joins("LEFT JOIN community_tags ON community_tags.tid = tag_selections.tid").joins("JOIN term_data ON term_data.tid = tag_selections.tid").group("term_data.name").count

Replaces the whole iterative loop, and is probably MUCH more efficient! No iterating over every tag... woohoo! I'm gonna replace this whole segment.

[jywarren]

Done in #7930! Let's see how that does.

[jywarren]

Merged it! Let's test at https://stable.publiclab.org/stats/subscriptions as soon as it's done building.

[jywarren]

OK, it kind of worked. It's no longer showing 219s... but the new query may suffer from other issues. Re-opening!

[jywarren]

I think i shouldn't have done LEFT JOIN?

[jywarren]

That's right, i think both should be INNER JOIN but this still returns too many:

tags2 = TagSelection.where(following: true).joins("INNER JOIN community_tags ON community_tags.tid = tag_selections.tid").joins("INNER JOIN term_data ON term_data.tid = tag_selections.tid").group("term_data.name").count['water-quality']
=> 34087

plots2/app/controllers/stats_controller.rb

Lines 3 to 7 in c0de1f0

	@tags = TagSelection.where(following: true)
	.joins("LEFT JOIN community_tags ON community_tags.tid = tag_selections.tid")
	.joins("JOIN term_data ON term_data.tid = tag_selections.tid")
	.group("term_data.name")
	.count

Here is the query being generated:

SELECT  `tag_selections`.* FROM `tag_selections` INNER JOIN term_data ON term_data.tid = tag_selections.tid INNER JOIN community_tags ON community_tags.tid = tag_selections.tid WHERE `tag_selections`.`tid` = 3838 AND `tag_selections`.`following` = TRUE LIMIT 11

[jywarren]

Ah, ok - so it's cross-maching and just returning a lot of duplicates. I can add a .distinct and this should be resolved i think?

[jywarren]

OK! So this works and returns proper counts:

    @tags = TagSelection.where(following: true)
      .joins("LEFT JOIN community_tags ON community_tags.tid = tag_selections.tid")
      .joins(:tag)
      .group("term_data.name")
      .count

However, it does not filter out spam. 😱

[jywarren]

We could TRY something like this:

TagSelection.where(following: true)
  .joins("LEFT JOIN community_tags ON community_tags.tid = tag_selections.tid")
  .joins(:tag)
  .joins("INNER JOIN node ON node.nid = community_tags.nid")
  .where("node.status = 1")
  .group("term_data.name")
  .count

However, it's a pretty slow query: maybe 3 seconds? We might be able to select fewer things to speed it up. But I dunno.

We could cache it every 24 hours?

[jywarren]

Caching would look like:

Rails.cache.fetch("stats/subscriptions/query", expires_in: 24.hours) do
  TagSelection.where(following: true)
    .joins("LEFT JOIN community_tags ON community_tags.tid = tag_selections.tid")
    .joins(:tag)
    .joins("INNER JOIN node ON node.nid = community_tags.nid")
    .where("node.status = 1")
    .group("term_data.name")
    .count
end

[dongskyler]

That sounds like a rather inefficient query. How big is the MySQL database? Do you have the database schema somewhere? Otherwise, my input might be limited.

[dongskyler]

I think adding server-side caching is a great idea. But it's also a good idea to improve the efficiency of SQL queries

[dongskyler]

Hi @dongskyler! Sorry, i think we must've given you a mistake in our suggested code change! Thanks for looking into this! We SUPER appreciate your help, and please don't feel this was your fault in any way! 😄

Hmm. I wonder -- it's a pretty odd bug. The change was to add the .joins() statement on this line:

plots2/app/controllers/stats_controller.rb

Line 4 in 0e58e62

TagSelection.where(following: true).joins(:node_tags).each do |tag|

Is it possible we're joining incorrectly? What if we joined against something which has only 219 records... or something... ?

Thank you very much for your kind words. I'm just trying to contribute here.

[jywarren]

Sure, some docs here:

• https://github.com/publiclab/plots2/tree/master/db/schema.rb.example
• https://github.com/publiclab/plots2/blob/master/doc/DATA_MODEL.md

I opened a PR with the cached version. We have about 5000 tag_selections, maybe 10000 nodes, maybe 68k NodeTags.

[jywarren]

So, the PR passed, and I'm tempted to merge this for now, but if you LOVE optimizing SQL, we can leave this issue open and see if there's another way to do it more efficiently! I'm not such a Rails SQL wizard, so I guess it doesn't seem like a very friendly place to start contributing 😅 but for sure some folks are great at databases so don't let me scare you off of it if you are!

If you're looking for something else to get into, there are a bunch of other issues I might suggest!

[jywarren]

I'm not sure why the caching caused trouble. But we could cache in the view template instead, which is easy too: https://guides.rubyonrails.org/caching_with_rails.html

We would just wrap these lines:

plots2/app/views/stats/subscriptions.html.erb

Line 14 in 9c726fd

<% @tags.reverse_each do |range,tags| %>

[emilyashley]

Just checking in that this came up in our weekly staff call today. We're getting ready to onboard a new staff member and this view will be very relevant to them.

Bumping this up in priority!

Important counts we're looking at right now:

• number of people following a tag
• unique contributors per topic area
• total amount of content in a topic area (questions, notes, wiki pages)

[emilyashley]

Do we have breakout issues for

1. tags appearing that have not been used in content?

or

2. ordering the tags in order of count? (edit: I see it here Subscription Tags Out of Order at https://publiclab.org/stats/subscriptions #7934) sorry catching up

and

3. having the correct counts on tags

or are these included in this issue?

[jywarren]

#7941 resolved the ordering issue. But i think we have the remaining issues, looking at https://stable.publiclab.org/stats/subscriptions:

• spam tags still seem to appear, for example tags with quickbooks in them
• the counts seem to be amplified. Those that ought to have the most do have the most, but spectrometer has 149526 but we don't have anywhere near that number of nodes, so i think some multiplication is happening somewhere, possibly in the table joining.

We'll spend some time digging in on this today!

[jywarren]

I think it's possible we need to create better test data for this so we can work the problem locally.

[jywarren]

I believe I've fixed the caching syntax and pushed a new PR. Let's fingers 🤞

[jywarren]

Merged, just need to confirm on https://stable.publiclab.org/stats/subscriptions now!

[jywarren]

OK, so, it's an improvement, but the counts are still inflated. For example, although https://stable.publiclab.org/tag/balloon-mapping is indeed a top tag, it has 80 followers, not 56700. I believe the number shown is not being grouped properly, and is repeating counts due to improper table joins and groupings. Next step is to debug on the console on the live server.

[jywarren]

OK, plan is to:

1. revert to original and just live with the spam
2. check the spam timestamps to see if it's even occurring anymore, maybe it was only possible during a specific window of time?
3. manually delete the spam

[jywarren]

OK so I have another attempt, manually tested on the live database, and I think this is correct:

plots2/app/controllers/stats_controller.rb

Lines 3 to 9 in 9b469ee

	@tags = TagSelection
	.select("DISTINCT tag_selections.tid, tag_selections.user_id")
	.where(following: true)
	.joins("INNER JOIN community_tags ON community_tags.tid = tag_selections.tid")
	.joins("INNER JOIN term_data ON term_data.tid = community_tags.tid")
	.group("term_data.name")
	.count

Note "DISTINCT" - it returns 109 for balloon-mapping, which is correct.

And it mostly works! The stats look much better: https://stable.publiclab.org/stats/subscriptions

But, balloon-mapping still has 100 instead of 80, which is weird because https://stable.publiclab.org/tag/balloon-mapping shows 80.

Aha, i wonder... we aren't filtering out banned people. Let's look:

plots2/app/models/tag.rb

Lines 182 to 186 in 9b469ee

	uids = TagSelection.joins(:tag)
	.where('term_data.name = ? AND following = ?', tagname, true)
	.collect(&:user_id)
	User.where(id: uids)
	.where(status: [1, 4])

[jywarren]

OK, added that too and re-added cache code, because that slowed it down a lot:

plots2/app/controllers/stats_controller.rb

Lines 3 to 13 in 8b0b3d5

	@tags = Rails.cache.fetch("stats-subscriptions-query", expires_in: 24.hours) do
	TagSelection
	.select("DISTINCT tag_selections.tid, tag_selections.user_id")
	.where(following: true)
	.joins("INNER JOIN community_tags ON community_tags.tid = tag_selections.tid")
	.joins("INNER JOIN term_data ON term_data.tid = community_tags.tid")
	.group("term_data.name")
	.joins("INNER JOIN rusers ON rusers.id = tag_selections.user_id")
	.where("rusers.status = 1 OR rusers.status = 4")
	.count
	end

[jywarren]

I think we're done here! Fully working on stable! Will publish soon.

[ebarry]

This page is looking great! https://publiclab.org/stats/subscriptions
I think we can close this issue

[jywarren]

Strangely this has re-broken, so re-opening:

8b0b3d5 was the last fix, a day before we closed this and had confirmed it on stable.

It's no longer working on stable either: https://stable.publiclab.org/stats/subscriptions

[jywarren]

On stable:

counts = TagSelection.select("DISTINCT tag_selections.tid, tag_selections.user_id").where(following: true).joins("INNER JOIN community_tags ON community_tags.tid = tag_selections.tid").joins("INNER JOIN term_data ON term_data.tid = community_tags.tid").group("term_data.name").joins("INNER JOIN rusers ON rusers.id = tag_selections.user_id").where("rusers.status = 1 OR rusers.status = 4").count["balloon-mapping"]
=> 88

which seems correct...????? vs. 61336 on https://stable.publiclab.org/stats/subscriptions ???

[ebarry]

Just came across this myself today on https://publiclab.org/stats/subscriptions. Compare the difference between what's listed at that URL vs on the Tag's own page:

• 
• 

[jywarren]

I recall someone suggesting we need much better test data to reproduce this and I wanted to suggest we develop some code snippets that can be used to generate a ton of users, tags, and subscriptions, to try to simulate the failure.

[17sushmita]

Part of #9827