Fix #104: Manage, throttle comments with Redis #131

openjck · 2015-07-31T02:52:40Z

Testing these changes:

Push this to your personal Heroku
heroku addons:create heroku-redis:hobby-dev (you'll need to enter a credit card)
heroku ps:scale web=1 worker=1
Push this .doiuse file to your test repo master branch
Submit a pull request to your test repo that adds this file

You should see 20 comments appear within a few seconds. There should be one comment for each property.

To run tests locally, see the updates in CONTRIBUTING.md.

darkwing · 2015-07-31T12:28:15Z

hook.js

+                        filename: file.filename,
+                        line: line,
+                        comment: comment
+                    }).save(function(error) {


.save(logger.error)

The error parameter is usually null, so we need to check it before logging it.

facepalms

Don't facepalm. 😄

I did exactly the same thing last night and was confused for way too long about why I was seeing "null" in the log.

darkwing · 2015-07-31T12:46:29Z

Is there any type of test we can add here? Seems like a good candidate for one.

openjck · 2015-07-31T14:54:56Z

Is there any type of test we can add here? Seems like a good candidate for one.

Good question. We could definitely test that items are successfully added to the Redis queue. I would also love to test whether all comments are accepted by GitHub. That would take a lot of work (and testing infrastructure). I would love to help add that but wouldn't want to block on that in the meantime.

groovecoder · 2015-07-31T15:02:03Z

Does someone really need to use a credit card to test the redis feature? Couldn't we share a dev-hobby REDIS_URL and credentials?

openjck · 2015-07-31T15:09:47Z

Does someone really need to use a credit card to test the redis feature? Couldn't we share a dev-hobby REDIS_URL and credentials?

Good question. I would love to help find a way to test the feature with entering a credit card. Unfortunately the REDIS_URL environment variable can change so copying it wouldn't be enough.

In order for Heroku to manage this add-on for you and respond to a variety of operational situations, the REDIS config vars may change at any time. Relying on the config var outside of your Heroku app may result in you having to re-copy the value if it changes.

—https://devcenter.heroku.com/articles/heroku-redis#create-a-new-instance

Thankfully the credit card is only needed to verify identity. If I understand correctly, there should be no charges.

Credit card information is not required for free apps without add-ons. It becomes a requirement once you wish to own more than 5 apps at a time, or to use add-ons other than postgresql:dev or pgbackups:plus –– even if the add-ons are free.

As a business, we need to be able to reliably identify and contact our users in the event of an issue. We have found that a credit card on file provides the most reliable way of obtaining verified contact information.

—https://devcenter.heroku.com/articles/account-verification#verification-requirement

openjck · 2015-07-31T15:28:00Z

Another challenge with using a shared Redis instance is that another worker might process the queue before the worker being tested.

groovecoder · 2015-07-31T15:40:48Z

Dang, you're already way ahead of me.

openjck · 2015-07-31T15:51:03Z

Dang, you're already way ahead of me.

It's a good point. Unfortunately it looks like entering a credit card for verification is going to be the easiest solution. If it's not Redis, there's going to be free add-ons in the future that will require it.

darkwing · 2015-07-31T18:38:39Z

redisQueue.js

+var kue = require('kue');
+var url = require('url');
+
+var config = {};


What about this? https://gist.github.com/darkwing/929bc313c69ac307d49c

Faeranne · 2015-07-31T20:00:46Z

How many seconds are we waiting on each comment?

openjck · 2015-07-31T20:03:29Z

How many seconds are we waiting on each comment?

It depends on the COMMENT_WAIT environment variable. A value of 250 (milliseconds) has been working well for me. If a comment fails it will be re-attempted COMMENT_ATTEMPTS times with an exponentially longer wait between attempts.

Faeranne · 2015-07-31T20:06:58Z

Is there a way to make it wait longer after several comments? GitHub started blocking after about 20 comments in a row during my tests, and adding a 2-second delay after about 20 comments prevented the spam-protection from triggering at all.

I believe hitting the spam protection multiple times in a row can cause a temporary ban, but I haven't seen anything that suggest how long the ban lasts, or how many times we can hit the spam-protect before the ban.

openjck · 2015-07-31T20:19:13Z

Good question. I noticed that, too.

It looks like submitting lots of comments in quick succession will trigger temporary spam protection, at which point all future comments will be rejected for around 30-60 seconds. If there's a delay between comments, the spam protection system appears not to be triggered at all. In my testing, waiting for 250 milleseconds between comments is enough for all of the comments to go through successfully.

If a comment does fail, the wait between re-attempts will increase, first to 500 milleseconds, then to 1 second, then 2, then 4, etc.

openjck · 2015-07-31T20:35:33Z

Hey, nice job figuring out that GitHub sends back a 403 when a comment is rejected. I was wondering why the Error posting pull request error message wasn't showing up in the logs previously.

openjck · 2015-07-31T23:22:49Z

So I've been thinking about tests. I love writing tests, and love having tests, but to be honest I'm having a hard time thinking of a test that would be valuable here.

Our tests already cover the Redis queueing/processing logic, so those results are definitely still valuable. We could additionally test that the queue itself works as expected, but Kue already covers that.

The real test is whether GitHub accepts all of the comments Discord sends, but we don't have a way of automating tests like that at the moment. I would love to help us build that testing infrastructure down the road, but would prefer not to block on it right now.

Faeranne · 2015-08-03T03:33:30Z

I doubt there is a good way to test that, but we probably should monitor the success vs reject rate. Is there a way to do that with new relic?

darkwing · 2015-08-03T17:29:58Z

Would redis be good storage for the success/reject rate?

Faeranne · 2015-08-03T18:16:07Z

If we can get persistent storage on it, I can't see why not.

openjck · 2015-08-03T18:24:15Z

Would redis be good storage for the success/reject rate?

Hm. I'm not sure I understand. Do you mean using Redis to log how often comments are rejected as compared to being accepted? Maybe @groovecoder would be able to say more about that. My experience with Redis so far has been that it specializes in job queuing, but I know it can be used for more than that. We're probably going to add Postres this week for record-keeping. Maybe that would be better.

darkwing · 2015-08-03T18:26:02Z

Oh, right, Postgres would likely be better.

groovecoder · 2015-08-03T18:51:17Z

Could raise rejections as errors that would bubble up in the New Relic error reporting. That would give us the built-in reporting tools of New Relic around rejections. (Whether that's good or bad.)

Could use Redis for storing data, just be aware that it needs some extra configuration if we want a degree of data safety comparable to Postrgres.

openjck · 2015-08-03T18:54:42Z

Could raise rejections as errors that would bubble up in the New Relic error reporting. That would give us the built-in reporting tools of New Relic around rejections. (Whether that's good or bad.)

Great point. It would be good to have that information in New Relic. Maybe it would make sense to fire something off to New Relic whenever logger.error is called. What do you think @darkwing?

openjck · 2015-08-03T19:44:36Z

Come to think of it, while New Relic reporting would be great, do we need to block these improvements on that? If anything, we should see fewer errors with the addition of Redis.

darkwing · 2015-08-04T15:22:42Z

Excellent! R+!

Fix #104: Manage, throttle comments with Redis

openjck assigned darkwing Jul 31, 2015

openjck mentioned this pull request Jul 31, 2015

Fix #104: Rate-limit comment submissions #112

Closed

openjck force-pushed the redis-for-comments branch 2 times, most recently from 4f8d633 to 91a3bc4 Compare July 31, 2015 03:23

darkwing reviewed Jul 31, 2015
View reviewed changes

openjck force-pushed the redis-for-comments branch 2 times, most recently from 2b7926e to 6d3d270 Compare July 31, 2015 19:58

openjck force-pushed the redis-for-comments branch from 6d3d270 to cff3a76 Compare July 31, 2015 20:34

openjck force-pushed the redis-for-comments branch 3 times, most recently from f75b2cb to bdc279a Compare July 31, 2015 23:03

Fix groovecoder#104: Manage, throttle comments with Redis

9c98eb2

openjck force-pushed the redis-for-comments branch from bdc279a to 9c98eb2 Compare July 31, 2015 23:09

darkwing added a commit that referenced this pull request Aug 4, 2015

Merge pull request #131 from openjck/redis-for-comments

58489f8

Fix #104: Manage, throttle comments with Redis

darkwing merged commit 58489f8 into groovecoder:master Aug 4, 2015

openjck mentioned this pull request Aug 4, 2015

add queue management for comments. fixes #104 #124

Closed

openjck mentioned this pull request Dec 31, 2015

React to rate-limit being reached #87

Open

groovecoder unassigned darkwing Mar 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #104: Manage, throttle comments with Redis #131

Fix #104: Manage, throttle comments with Redis #131

openjck commented Jul 31, 2015

darkwing Jul 31, 2015

openjck Jul 31, 2015

darkwing Jul 31, 2015

openjck Jul 31, 2015

darkwing commented Jul 31, 2015

openjck commented Jul 31, 2015

groovecoder commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

groovecoder commented Jul 31, 2015

openjck commented Jul 31, 2015

darkwing Jul 31, 2015

Faeranne commented Jul 31, 2015

openjck commented Jul 31, 2015

Faeranne commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

Faeranne commented Aug 3, 2015

darkwing commented Aug 3, 2015

Faeranne commented Aug 3, 2015

openjck commented Aug 3, 2015

darkwing commented Aug 3, 2015

groovecoder commented Aug 3, 2015

openjck commented Aug 3, 2015

openjck commented Aug 3, 2015

darkwing commented Aug 4, 2015

Fix #104: Manage, throttle comments with Redis #131

Fix #104: Manage, throttle comments with Redis #131

Conversation

openjck commented Jul 31, 2015

darkwing Jul 31, 2015

Choose a reason for hiding this comment

openjck Jul 31, 2015

Choose a reason for hiding this comment

darkwing Jul 31, 2015

Choose a reason for hiding this comment

openjck Jul 31, 2015

Choose a reason for hiding this comment

darkwing commented Jul 31, 2015

openjck commented Jul 31, 2015

groovecoder commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

groovecoder commented Jul 31, 2015

openjck commented Jul 31, 2015

darkwing Jul 31, 2015

Choose a reason for hiding this comment

Faeranne commented Jul 31, 2015

openjck commented Jul 31, 2015

Faeranne commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

openjck commented Jul 31, 2015

Faeranne commented Aug 3, 2015

darkwing commented Aug 3, 2015

Faeranne commented Aug 3, 2015

openjck commented Aug 3, 2015

darkwing commented Aug 3, 2015

groovecoder commented Aug 3, 2015

openjck commented Aug 3, 2015

openjck commented Aug 3, 2015

darkwing commented Aug 4, 2015