ixti / sidekiq-throttled

Concurrency and rate-limit throttling for Sidekiq
MIT License
710 stars 76 forks source link

Jobs do not seem to be throttled. Setting concurrency to `limit: 10` but moving all jobs to `busy`. #88

Closed csalvato closed 1 year ago

csalvato commented 4 years ago

Hi! Thanks for writing and maintaining this gem. 🙏

It seemed like problems/questions for this repo should be posted as an issue here. If that's not true, please let me know and I will move this elsewhere.

Summary

  1. I currently have sidekiq running with a concurrency of 40 total jobs on my local development environment.
  2. I would like to limit one particular job, our BatchUsersImportWorker, from queuing more than 5 of this type of worker at once. (it seems like this is just what sidekiq-throttled does!)
  3. I've gone through the steps in the README to limit this job to run only 5 instances concurrently

Actual Behavior

When I queue up more than 5 of these jobs, they all enter my busy queue, up to the max of 40 total jobs.

Expected Behavior

When I queue up more than 5 of these jobs, only 5 should enter my busy queue, and the other 35 should stay enqueued and not consume slots in my sidekiq busy list or in my queue of other jobs.

The environment

# running sidekiq with a concurrency of 40 jobs
bin/sidekiq -v -c40 -q critical -q default -q low
# The job we want to limit to only 5 concurrent jobs
class BatchUserImportWorker
  include Sidekiq::Worker
  include Sidekiq::Throttled::Worker
  include Importers::BatchUserImportWorkerReporting

  sidekiq_options retry: 3, customer_aware: true, queue: 'low'

  # Allow maximum 5 concurrent BatchUserImportWorker jobs at a time
  sidekiq_throttle({
    :concurrency => { :limit => 5 },
  })

  def perform(some_args)
    # implementation redacted
  end
end
# initializers/sidekiq.rb

require 'erb'
require 'yaml'
require 'sidekiq/cron'
require "sidekiq/throttled"
require_relative './sidekiq/customer_aware_server'
require_relative './sidekiq/customer_aware_client'

jobs = Rails.application.config_for(:redis)['jobs']

Sidekiq.configure_server do |config|
  config.redis = {
    url: "redis://#{jobs['host']}:#{jobs['port']}/0",
    namespace: 'sidekiq',
    network_timeout: 5
  }
  config.server_middleware do |chain|
    chain.add ::Sidekiq::CustomerAwareServer
  end

  config.client_middleware do |chain|
    chain.add ::Sidekiq::CustomerAwareClient
  end

  config.death_handlers << ->(job, _ex) do
    SidekiqUniqueJobs::Digests.del(digest: job['unique_digest']) if job['unique_digest']
  end

  schedule_file = "config/scheduled_workers.yml"
  if File.exist?(schedule_file)
    Sidekiq::Cron::Job.load_from_hash YAML.load_file(schedule_file)
  end
end

Sidekiq.configure_client do |config|
  config.redis = {
    url: "redis://#{jobs['host']}:#{jobs['port']}/0",
    namespace: 'sidekiq',
    network_timeout: 5
  }
  config.client_middleware do |chain|
    chain.add ::Sidekiq::CustomerAwareClient
  end
end

Sidekiq.default_worker_options = { :retry => 3 } if Rails.env.staging?

if Rails.env.test?
  Sidekiq.logger.level = Logger::FATAL
end

Sidekiq::Throttled.setup!
# in routes.rb
# ...
  Sidekiq::Throttled::Web.enhance_queues_tab!
  mount Sidekiq::Web, at: '/sidekiq'
# ...

In Gemfile.lock:

    sidekiq (5.2.8)
      connection_pool (~> 2.2, >= 2.2.2)
      rack (< 2.1.0)
      rack-protection (>= 1.5.0)
      redis (>= 3.3.5, < 5)
    sidekiq-cron (1.0.4)
      fugit (~> 1.1)
      sidekiq (>= 4.2.1)
    sidekiq-failures (1.0.0)
      sidekiq (>= 4.0.0)
    sidekiq-history (0.0.9)
      sidekiq (>= 3.0.0)
    sidekiq-limit_fetch (3.4.0)
      sidekiq (>= 4)
    sidekiq-pro (4.0.5)
      concurrent-ruby (>= 1.0.5)
      sidekiq (>= 5.2.1)
    sidekiq-throttled (0.13.0)
      concurrent-ruby
      redis-prescription
      sidekiq
    sidekiq-unique-jobs (6.0.12)
      concurrent-ruby (~> 1.0, >= 1.0.5)
      sidekiq (>= 4.0, < 7.0)
      thor (~> 0)

What I see in the throttled tab

Screen Shot 2020-09-16 at 12 04 33 PM

What I see in the Busy tab (notice all 40 jobs are the one we are throttling

sidekiq

The only further debugging step I can think of is to remove other sidekiq gems and see if there's some kind of conflict. However, I think it's more likely that I've overlooked something simple, but I just can't see it as I have re-read the README 5 times now 😂

If you have any leads on how I can get this to work, that would be appreciated 🙏

Thanks again for writing and maintaining this gem!

lloydwatkin commented 4 years ago

Hey @csalvato we see the same issue and our setup is pretty much the same. We both use sidekiq-cron perhaps that's the issue?

lloydwatkin commented 4 years ago

Experimented with uninstalling sidekiq-cron on a staging environment and that wasn't the issue

csalvato commented 4 years ago

We were unable to figure this out, and opted to upgrade to Sidekiq Enterprise and use their rate limiting API, instead.

ixti commented 4 years ago

It's hard for me to tell or debug what's wrong when commercial sidekiq (sidekiq-pro or sidekiq-enterprise) is enabled. sidekiq-throttled implements throttling via having its own fetcher, and if you use different fetcher - it simply won't work.

I've extracted throttling logic into a separate gem though if anybody is interested: https://gitlab.com/ixti/redis-throttle/

That gem is totally sidekiq-agnostic. And thus you control how to handle throttling:

class MyWorker
  include Sikdeiq::Worker

  THROTTLE = Redis::Throttle
    .new(:redis => Sidekiq.method(:redis))
    .concurrency(:limit => 10, :ttl => 15.minutes)
    .freeze

  def perform(*args)
    THROTTLE.call(:token => jid) do
      # withing throttling limits
      return
    end

    # throttling is in action
    self.class.perform_in(15.minutes, *args)
  end
csalvato commented 4 years ago

@ixti It sounds like this gem is not compatible with sidekiq enterprise or sidekiq pro, is that right?

If so, is it worthwhile to add that to the README? That would have saved me considerable time, as we must use at least Sidekiq Pro in our app.

ixti commented 4 years ago

@csalvato compatibility depends on whenever you're using custom fetcher, and it's not limited by pro/enterprise. Gem heavily depends on using its own fetcher, yes.

heaven commented 3 years ago

Hi @ixti, we don't use anything that'd override the fetcher, just a single extension we use is sidekiq-batch gem.

Here's our sidekiq.rb

require "sidekiq/throttled"
Sidekiq::Throttled.setup!
Sidekiq::Extensions.enable_delay!

And the job:

class ThrottledJob

  include Sidekiq::Worker
  include Sidekiq::Throttled::Worker

  sidekiq_throttle({
    concurrency: {
      limit: 1,
      key_suffix: -> (page_id) {
        page_id
      }
    }
  })

  def perform(page_id)
    logger.info("Processing.")
  end

end

When putting a debugger into the #perform method it shows

(byebug) Sidekiq.options[:fetch]
=> Sidekiq::Throttled::Fetch

So it is not overridden for sure but still doesn't work. Was wondering if I am doing anything wrong or it is a bug.

Redis server 6.0.10 Sidekiq 6.0.5

longkt90 commented 3 years ago

if you use Rails' ActiveJob job with sidekiq queue adapter the throttling is not working, we workaround by using Sidekiq Workers directly. We use sidekiq-pro

ElliottRoche commented 3 years ago

I'm seeing the same problem @heaven, and the worker in our case is a true Sidekiq::Worker and not an ActiveJob.

ixti commented 1 year ago

As of 1.0.0.alpha, this gem should work fine with sidekiq-pro (I've removed all the incompatible parts - to be optional). But I can't verify if it works or not.