1177798 - Ordering of contributors in contributor bar

Status: RESOLVED FIXED

(developer.mozilla.org Graveyard :: General, enhancement)

Description

[Florian Scholz (Open Web Docs)]

What feature should be changed? Please provide the URL of the feature if possible.
==================================================================================
Currently the contributors appear to be ordered by user id. This puts Janet, Sheppy and myself in front most of the time. (as we registered early)

What problems would this solve?
===============================
As we have no order (by amount contribution or so) we should be random for real.

Who would use this?
===================
Readers, contributors.

What would users see?
=====================
Randomly ordered contributor bar.

What would users do? What would happen as a result?
===================================================
Equal credits to contributors.

Is there anything else we should know?
======================================

Comment 1

[Jannis Leidel [:jezdez]]

It's *not* ordered by user id but by number of revisions per user: https://github.com/mozilla/kuma/blob/30876ecc79ca2496417fe1214b73ff035ae1d81c/kuma/wiki/models.py#L1509-L1511

"randomizing" database query results is hard, mysql is slow with those and server side randomization without limiting the number of contributors to display is a guarantee to create a memory overflow at some point. In short, if you really want this, tell me the upper limit of contributors to show and we can implement it on the server side.

Comment 2

[Justin Crawford [:hoosteeno] [:jcrawford]]

I know this bug is about randomizing, but I would like to suggest a potentially less expensive and more valuable sort: Unique, unbanned users sorted by most recent edit. 

This is more valuable than random because the current state of the article most likely reflects the effort of its most recent editors, who are the most likely to be seen in this sort.

shobson: 
1) What do you think of the above?
2) Does the UI suggest any limit to the number of editors we should show? 20? 200?

Comment 3

[Florian Scholz (Open Web Docs)]

I thought it is ordered by id. So, forget the whole random idea :)

Leaving open if we want to discuss the ordering still. Feel free to close, if we are all good, though.

Comment 4

[Justin Crawford [:hoosteeno] [:jcrawford]]

This bug could be called, "give other contributors besides staff a premiere place in the contributor bar". Whether ordered by ID or frequency, admins (or long-gone users) end up first. But if we order by recency, any active editor could be first.

Comment 5

[Stephanie Hobson [:shobson]]

I like the idea of ordering by recent edits but I am concerned this will lead to small useless edits to get one's picture there on prominent pages.

Jannis, would it be easier if we asked Django to randomly pick the 13 users displayed by default before the "show all" link? (instead of randomizing the query or randomizing all of them).

Comment 6

[Florian Scholz (Open Web Docs)]

(In reply to Jannis Leidel [:jezdez] from comment #1)
> It's *not* ordered by user id but by number of revisions per user

Looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String this is not true I think.

lmorchard is the first one here, and myself, contributor "Sevenspade" and many others have a lot more edits on this doc than lmorchard.

Comment 7

[Jannis Leidel [:jezdez]]

:fscholz You're totally right, this seems to not have worked correctly, sadly.

:shobson Sadly, this is a limitation of our underlying database MySQL, not an issue with Django. Randomization is contrary to the way how it stored indexes for ordering, so it has to go through all the items to find a "random" selection. Given the fact we're talking about the revision table with *lots* of items I'm not in favor of that.

:hoosteeno I agree that showing the recent authors is less of an impact, I've opened https://github.com/mozilla/kuma/pull/3300 with a possible implementation which also caches the contributor list for 6 hours and invalidates it when a document is saved. I haven't implemented invalidation when a user is changed (e.g. banned) as that would imply we would have to invalidate the cache of all the user's documents, which can backfire if a spammer creates lots of revisions and we ban them, essentially creating a big load for the Celery workers to invalidate the contributor bar cache.

Comment 8

[Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)]

> :shobson Sadly, this is a limitation of our underlying database MySQL, not an issue with
> Django. Randomization is contrary to the way how it stored indexes for ordering, so it has to
> go through all the items to find a "random" selection. Given the fact we're
> talking about the revision table with *lots* of items I'm not in favor of that.

What about doing the randomization in Django (or even in JS), instead of at the DB level?

Comment 9

[Jannis Leidel [:jezdez]]

:jsx Server side randomization would work but badly. It implies that we'd have to load *all* revision authors of a document at once to be able to shuffle the list. That's not scalable as we're adding more and more contributors and at some point the cost of fetching all users will make the memory usage inefficient. Admittedly when looking at all the things the document view does this seems small but bear in mind that it's the single busiest code of MDN. So adding new potentially expensive code needs to be considered carefully.

Randomization in JS only moves the problem, we'd still need to provide some data structure so it can build the contributor bar during page load time. It would also defeat the load balancer level caching we have in place and distribute the load to each and every document page load. In other words, the worst of both worlds.

Comment 10

[Justin Crawford [:hoosteeno] [:jcrawford]]

(In reply to Jannis Leidel [:jezdez] from comment #7)

> I've opened https://github.com/mozilla/kuma/pull/3300 with a possible
> implementation which also caches the contributor list for 6 hours and
> invalidates it when a document is saved.

Sweet!

>  I haven't implemented invalidation
> when a user is changed (e.g. banned) 

I think this is OK. Thanks!

Comment 11

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/4ebb3e9fc3ab6e67947717bbbf5608ac23ac3679
Bug 1177798 - Order contributors by recency of last edit.

In other words, the active user with the most recent edit shows up first.

This also moves the fetching of the contributors into a cacheback job to be able to invalidate the cache asynchronously.

https://github.com/mozilla/kuma/commit/9713582bbbfcadba88ff6a7877676be6224a233e
Merge pull request #3300 from mozilla/bug1177798

Bug 1177798 - Order contributors by recency of last edit.

Comment 12

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/4d015da4d1fd5a989dc816c0fa05d03be1b6f1fa
Bug 1177798 - Only cache a dictionary of user data instead of full ORM objects.

This should reduce the amount spent in Python a bunch.

https://github.com/mozilla/kuma/commit/87eea6dc9b4f056cb00adbe14df2ea4994f86fa0
Merge pull request #3318 from mozilla/bug1177798-2

Bug 1177798 - Only cache a dictionary of user data instead of full ORM objects.

Comment 13

[MDN Team (:mdn-dev)]

Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/57db9fe079dd04b8f3f056dc78d0787aa2f819d4
Revert "Bug 1177798 - Only cache a dictionary of user data instead of full ORM objects."

This reverts commit 4d015da4d1fd5a989dc816c0fa05d03be1b6f1fa.

https://github.com/mozilla/kuma/commit/521ed18851c0a846786d4f81ec73467f2c42ca03
Revert "Bug 1177798 - Order contributors by recency of last edit."

This reverts commit 4ebb3e9fc3ab6e67947717bbbf5608ac23ac3679.

Comment 14

[Jannis Leidel [:jezdez]]

This was rolled back in https://github.com/mozilla/kuma/pull/3325

Comment 15

[Luke Crouch [:groovecoder]]

Okay, got back to this today. We learned that the excessive cache calls from *product details* (i.e., not the contributor bar) caused the major outage. But, when I check the thread profile [1], it looked like product details was using locmem, not memcache?

If that's the case, I'm still a little worried about re-introducing a memcache cacheback job for every document view. :/

Now that we have a memcache server on staging (bug 1183839), we could try this again and watch the memcache behavior/activity on the stage server before pushing to production. Or, should we wait until we have load-testing going on stage deploys? (bug 1186081)

[1] https://rpm.newrelic.com/accounts/263620/applications/3172075/profiles/1710887

Comment 16

[Jannis Leidel [:jezdez]]

:groovecoder Yes, product-details uses locmem again since we rolled back the change in https://github.com/mozilla/kuma/pull/3326. That said the issue with memcache was increased because due to bad programming we actually fetched data from product-details on every rendering of a page to populate the choices of the language selector. Basically a lazy code loading issue. I've fixed that in https://github.com/mozilla/kuma/pull/3328. Together with the use of locmem we're basically working around the sore point of increased memcache traffic. 

I'm not so worried re-introducing the call to memcache for fetching the contributor bar payload, I'll make sure to only cache a dict of data to not have to deserialize Djagno ORM objects which would increase the load.

Testing this on stage makes sense to me, we can further write this defensively and not issue any memcache or db query if the waffle flag is off.

Comment 17

[MDN Team (:mdn-dev)]

Commit pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/3684318bf0adf82611589fb7a2cde035de29325b
Merge pull request #3369 from mozilla/bug1177798-3

Bug 1177798 - Order contributors by recency of last edit.

Comment 18

[Luke Crouch [:groovecoder]]

This is rolled out to everyone on MDN now and is looking good.