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

Run customerio in background #422

Merged
merged 1 commit into from
Feb 22, 2017
Merged

Conversation

drogus
Copy link
Contributor

@drogus drogus commented Feb 21, 2017

From commit message:

At the moment we need to wait for a customerio's response in order to
log a user in. I think that this might be causing problems for some
users. I'm not 100% certain that this is customerio's problem, but I
think that we should be running it in the background anyway. It's not
crucial for signing in, so we shouldn't be making a handshake call
longer than it needs to be.

This commit creates a customerio worker, which will run the needed
request using sidekiq.

@drogus
Copy link
Contributor Author

drogus commented Feb 21, 2017

I've tested this on staging and it works correctly. I'd like to give it a go on production soon.

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Contributor

  1. If we get a lot of requests to a handshake endpoint that would end up
    in one instance, we will potentially fire a lot of threads. I don't
    think that's a huge problem, either, as we usually run at least a few
    Heroku dynos and we have API anti-ddos protection, which should prevent
    it.

IMO it would be a good idea to put a sidekiq queue in between, so that this kind of slowdown does not have the potential to bring down the whole web dyno.

@joecorcoran
Copy link
Contributor

I don't want to be obstructive, but I tend to agree with @igorwwwwwwwwwwwwwwwwwwww – we have Sidekiq running for cancellations and restarts in API anyway, so maybe we could just add another queue?

@drogus
Copy link
Contributor Author

drogus commented Feb 21, 2017

IMO it would be a good idea to put a sidekiq queue in between, so that this kind of slowdown does not have the potential to bring down the whole web dyno.

That's what I would like to avoid as it's a lot of setup for such a simple call.

we have Sidekiq running for cancellations and restarts in API anyway, so maybe we could just add another queue?

I think that this was changed at some point and all of the sidekiq jobs land in hub

~/ code/travis/travis-api $ heroku ps -rproduction
=== web (Performance-L): ./script/server (5)
web.1: up 2017/02/20 16:00:01 +0100 (~ 22h ago)
web.2: up 2017/02/20 15:59:58 +0100 (~ 22h ago)
web.3: up 2017/02/20 15:59:59 +0100 (~ 22h ago)
web.4: up 2017/02/20 15:59:59 +0100 (~ 22h ago)
web.5: up 2017/02/20 15:59:58 +0100 (~ 22h ago)

=== cron (Standard-1X): bin/cron (1)
cron.1: up 2017/02/20 15:59:56 +0100 (~ 22h ago)
~/ code/travis/travis-pro-api $ heroku ps -rproduction
=== web (Performance-L): ./script/server (2)
web.1: up 2017/02/21 12:16:10 +0100 (~ 2h ago)
web.2: up 2017/02/21 12:16:11 +0100 (~ 2h ago)

=== cron (Standard-1X): ./bin/cron (1)
cron.1: up 2017/02/21 12:16:08 +0100 (~ 2h ago)
cron.1: up 2017/02/20 15:59:56 +0100 (~ 22h ago)

That said, I guess I could run a sidekiq worker in API to handle that, as I agree it's just safer to do that.

@drogus
Copy link
Contributor Author

drogus commented Feb 21, 2017

Thanks for the input. After thinking it through again I think that I was too optimistic about using threads directly. Other solutions not involving sidekiq, like creating a thread pool would complicate things and then why not just use sidekiq in the first place?

@drogus
Copy link
Contributor Author

drogus commented Feb 21, 2017

I tested it on staging and it works correctly, so I'll probably give it a shot tomorrow if no one objects.

@drogus drogus force-pushed the ps-run-customerio-in-background branch from c62bd71 to f7145a4 Compare February 22, 2017 08:16
At the moment we need to wait for a customerio's response in order to
log a user in. I think that this might be causing problems for some
users. I'm not 100% certain that this is customerio's problem, but I
think that we should be running it in the background anyway. It's not
crucial for signing in, so we shouldn't be making a handshake call
longer than it needs to be.

This commit creates a customerio worker, which will run the needed
request using sidekiq.
@drogus drogus force-pushed the ps-run-customerio-in-background branch from f7145a4 to f562ec9 Compare February 22, 2017 08:17
@drogus drogus merged commit 37f09f5 into master Feb 22, 2017
@renee-travisci renee-travisci deleted the ps-run-customerio-in-background branch February 22, 2017 23:19
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