-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: Allow customizing the rake task regex to avoid starting the reporter #220
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, had a small suggestion and also there seems to be some issue with standard and different Ruby versions we might need to tackle.
@@ -27,7 +27,7 @@ def self.adapter_config | |||
def initialize | |||
super | |||
|
|||
log_msg = +"#{self.class.adapter_name} enabled" | |||
log_msg = "#{self.class.adapter_name} enabled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like Ruby 2.x didn't like this change... it appears they consider interpolated strings as frozen, whereas 3+ doesn't: (which is probably why standard/rubocop suggested it?)
% chruby 3.3.4
% ruby --enable-frozen-string-literal -e 'puts [RUBY_VERSION, "foo".frozen?, "#{%q(zomg)}".frozen?].inspect'
["3.3.4", true, false]
% chruby 2.7.8
% ruby --enable-frozen-string-literal -e 'puts [RUBY_VERSION, "foo".frozen?, "#{%q(zomg)}".frozen?].inspect'
["2.7.8", true, true]
I'm not sure I have a good suggestion that doesn't include a check on RUBY_VERSION to dup
the string for now... (or always dup
it and leave a note about when dropping older versions or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RedundantInterpolationUnfreeze
check was literally added to standard
two days ago and subsequently released in 1.41.0.
Another approach here is to leave our code as it was before this PR, and lock our lint
GH Action to gem install standardrb -v '~> 1.40.0'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drafted a fix in #221.
@@ -87,6 +87,7 @@ def reset | |||
@log_tag = "Judoscale" | |||
@max_request_size_bytes = 100_000 # ignore request payloads over 100k since they skew the queue times | |||
@report_interval_seconds = 10 | |||
@rake_task_ignore_regex = /assets:|db:/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be more Rails specific than Ruby specific, I guess, since it's only something we use there... maybe we should only expose it within Rails, like we have start_reporter_after_initialize
?
judoscale-ruby/judoscale-rails/lib/judoscale/rails/config.rb
Lines 4 to 6 in 92a34b9
module Rails | |
module Config | |
attr_accessor :start_reporter_after_initialize |
(btw, it does make me wonder if we should have config.rails.foo
like we have with the background workers, but that's probably another topic for discussion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call! I'm moving this to be Rails-specific.
@@ -3,5 +3,6 @@ | |||
Judoscale.configure do |config| | |||
# Open https://judoscale-adapter-mock.requestcatcher.com in a browser to monitor requests | |||
config.api_base_url = ENV["JUDOSCALE_URL"] || "https://judoscale-adapter-mock.requestcatcher.com" | |||
config.rake_task_ignore_regex = /assets:|db:|middleware/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it seem that "middleware" is default enough to warrant we skipping that by default as well? Or do you think we could run into some unexpected issue if we were to add it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'd run into any issues by including middleware
by default, but for now I think I'd rather leave it out. It's not typically run in build processes or prod environments, and it's a quick task as opposed to a task that could take a while.
I don't think our default rake_task_ignore_regex
needs to be exhaustive. The goal is really to avoid starting the reporter during common build processes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,5 +3,6 @@ | |||
Judoscale.configure do |config| | |||
# Open https://judoscale-adapter-mock.requestcatcher.com in a browser to monitor requests | |||
config.api_base_url = ENV["JUDOSCALE_URL"] || "https://judoscale-adapter-mock.requestcatcher.com" | |||
config.rake_task_ignore_regex = /assets:|db:|middleware/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, sounds good.
Fixes #219
Before adding
config.rake_task_ignore_regex = /assets:|db:|middleware/
to the sample app:After adding
config.rake_task_ignore_regex = /assets:|db:|middleware/
to the sample app: