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

optionally avoid Sidekiq::Stats#fetch_stats! #10

Merged

Conversation

adamvduke
Copy link
Contributor

#9 adds the option to not report global sidekiq stats, but still initializes a Sidekiq::Stats instance which makes the calls to redis. The Sidekiq::Stats instance should only be initialized if the sidekiq_stats option is true.

@phstc
Copy link
Owner

phstc commented Oct 1, 2015

@nhocki do you mind reviewing it? 🍻

I don't have a Sidekiq + StatsD setup to test it right now. 🐼

@adamvduke
Copy link
Contributor Author

I also did some testing and noticed that the change didn't have a significant impact on the timings that get reported to statsd. It seems that is because fetching the stats is currently outside the scope of the timer. The middleware doesn't seem to be reporting how long it takes for a worker to run in total. It only reports how long the #perform method takes. Is that something that should be changed?

@adamvduke
Copy link
Contributor Author

This is an identical set of 100000 jobs that just sleep for 0.03 seconds each with the only difference being changing the initializer from true to false. The second batch processed 7 minutes faster, and roughly 50% more jobs processed per time interval. Also note, ~75% drop in redis operations.

screen shot 2015-10-01 at 3 17 46 pm

phstc added a commit that referenced this pull request Oct 7, 2015
…kiq-fetch-stats

optionally avoid Sidekiq::Stats#fetch_stats!
@phstc phstc merged commit 25302ab into phstc:master Oct 7, 2015
@phstc
Copy link
Owner

phstc commented Oct 7, 2015

@adamvduke thanks. Added to master 🍻

@adamvduke adamvduke deleted the adamdvuke/optionally-avoid-sidekiq-fetch-stats branch October 7, 2015 19:19
@adamvduke
Copy link
Contributor Author

🍻

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

2 participants