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

logging controls on a per-worker basis #1521

Closed
wants to merge 1 commit into from

Conversation

scootklein
Copy link

Refactored as per our previous discussions. Is there anything you'd like me to add to to the wiki/readme for how to use this functionality?

Also, would monkeypatching be the best way to inject into Sidekiq::Extensions::DelayedClass?

module Sidekiq
  module Extensions
    class DelayedClass
      sidekiq_logger do |logger|
        logger.level = Logger::WARN
        logger.execution_threshold = 5
      end
    end
  end
end

@mperham
Copy link
Collaborator

mperham commented Mar 1, 2014

It's been months since we chatted. Could you summarize the changes here and what you're looking to accomplish? I'm old and my memory is fading...

@scootklein
Copy link
Author

We're running > 5k background jobs per minute, most of which are ephemeral and we don't need logging for. Paying for a hosted rsyslog service can get costly when most of the chatter from Sidekiq isn't necessary. The PR lets you customize the logger per-worker, to raise the log level (WARN only shows failures), and to optionally set an execution threshold to help debug jobs or queues that are taking a long time to complete.

@jonhyman
Copy link
Contributor

jonhyman commented Mar 1, 2014

What we did was remove the middleware that adds the "started" and "finished" logging and kept the log level at info for all workers. We do about 50-100k jobs per minute during normal daily peak load and had the same motivation regarding cloud logging. IMO I don't know if this is needed, instead just do

config.server_middleware do |chain|
  chain.remove(Sidekiq::Middleware::Server::Logging)
end

@scootklein
Copy link
Author

I'd rather not remove all of the logging, seems that could introduce unintended consequences and will give us less visibility, especially for jobs or queues that are causing congestion among the worker pool.

@jonhyman
Copy link
Contributor

jonhyman commented Mar 3, 2014

So then add it around your perform method for those jobs? Is that a
reasonable solution for you?

Sent from my mobile device
On Mar 3, 2014 12:58 AM, "Scott Klein" notifications@github.com wrote:

I'd rather not remove all of the logging, seems that could introduce
unintended consequences and will give us less visibility, especially for
jobs or queues that are causing congestion among the worker pool.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1521#issuecomment-36483623
.

@jonhyman
Copy link
Contributor

jonhyman commented Mar 3, 2014

Or write your own Middleware for logging.

Sent from my mobile device
On Mar 3, 2014 7:47 AM, "Jonathan Hyman" hyman.jon@gmail.com wrote:

So then add it around your perform method for those jobs? Is that a
reasonable solution for you?

Sent from my mobile device
On Mar 3, 2014 12:58 AM, "Scott Klein" notifications@github.com wrote:

I'd rather not remove all of the logging, seems that could introduce
unintended consequences and will give us less visibility, especially for
jobs or queues that are causing congestion among the worker pool.

Reply to this email directly or view it on GitHubhttps://github.com//pull/1521#issuecomment-36483623
.

@mperham
Copy link
Collaborator

mperham commented Mar 9, 2014

I've gotta side with Jon here. This feels overly complicated for Sidekiq core.

@mperham mperham closed this Mar 9, 2014
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.

None yet

3 participants