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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 16, 2024
+8 −2
Merged

fix: Allow customizing the rake task regex to avoid starting the reporter #220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f36030
fix: Allow customizing the rake task regex to avoid starting the repo…
adamlogic Oct 10, 2024
92a34b9
standard formatting
adamlogic Oct 10, 2024
ba932c2
Merge branch 'main' into config-rake-task
adamlogic Oct 15, 2024
c649d9c
Fix frozen string for Ruby 2.x
adamlogic Oct 15, 2024
e54cf5a
Fix frozen string for Ruby 2.x
adamlogic Oct 15, 2024
f4f9fd9
rake_task_ignore_regex is a Rails-specific option
adamlogic Oct 15, 2024
83ef97a
standard
adamlogic Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion judoscale-rails/lib/judoscale/rails/railtie.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def judoscale_config
config.after_initialize do
if in_rails_console_or_runner?
logger.debug "No reporting since we're in a Rails console or runner process"
elsif in_rake_task?(/assets:|db:/)
elsif in_rake_task?(judoscale_config.rake_task_ignore_regex)
logger.debug "No reporting since we're in a build process"
elsif judoscale_config.start_reporter_after_initialize
Reporter.start
Expand Down
3 changes: 2 additions & 1 deletion judoscale-ruby/lib/judoscale/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def self.expose_adapter_config(config_instance)
end
end

attr_accessor :api_base_url, :report_interval_seconds,
attr_accessor :api_base_url, :report_interval_seconds, :rake_task_ignore_regex,
:max_request_size_bytes, :logger, :log_tag, :current_runtime_container
attr_reader :log_level

Expand All @@ -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:/
Copy link
Member

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)

Copy link
Collaborator Author

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.


self.log_level = ENV["JUDOSCALE_LOG_LEVEL"] || ENV["RAILS_AUTOSCALE_LOG_LEVEL"]
@logger = ::Logger.new($stdout)
Expand Down
2 changes: 1 addition & 1 deletion judoscale-ruby/lib/judoscale/job_metrics_collector.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

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)

Copy link
Collaborator Author

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'.

Copy link
Collaborator Author

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.

log_msg << " with busy job tracking support" if track_busy_jobs?
logger.info log_msg
end
Expand Down
2 changes: 1 addition & 1 deletion judoscale-ruby/lib/judoscale/logger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class LoggerProxy < Struct.new(:logger, :log_level)
private

def tag(msgs, tag_level: nil)
tag = +"[#{Config.instance.log_tag}]"
tag = "[#{Config.instance.log_tag}]"
tag << " [#{tag_level}]" if tag_level
msgs.map { |msg| "#{tag} #{msg}" }.join("\n")
end
Expand Down
1 change: 1 addition & 0 deletions sample-apps/rails-sample/config/initializers/judoscale.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/
Copy link
Member

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?

Copy link
Collaborator Author

@adamlogic adamlogic Oct 15, 2024 •
edited
Loading

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, sounds good.

# config.start_reporter_after_initialize = false
end
Loading