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

Jump to bottom

Simplify RuntimeContainer #178

Merged
merged 1 commit into from
Jul 17, 2023
Merged

Simplify RuntimeContainer #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 5 additions & 20 deletions judoscale-ruby/lib/judoscale/config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,14 @@

module Judoscale
class Config
class RuntimeContainer
# E.g.:
# (Heroku) => "worker_fast", "3"
# (Render) => "srv-cfa1es5a49987h4vcvfg", "5497f74465-m5wwr"
def initialize(service_name = nil, instance = nil)
@service_name = service_name
@instance = instance
end

def to_s
# heroku: 'worker_fast.5'
# render: 'srv-cfa1es5a49987h4vcvfg.5497f74465-m5wwr'
"#{@service_name}.#{@instance}"
end

class RuntimeContainer < String
# Since Heroku exposes ordinal dyno 'numbers', we can tell if the current
# instance is redundant (and thus skip collecting some metrics sometimes)
# We don't have a means of determining that on Render though — so every
# instance must be considered non-redundant
def redundant_instance?
instance_is_number = Integer(@instance, exception: false)
instance_is_number && instance_is_number != 1
instance_number = split(".")[1].to_i
instance_number > 1
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

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

Is this working off the basis that nil.to_i is 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.

Yep!

Copy link
Member

Choose a reason for hiding this comment

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

Clever 😁

end
end

Expand Down Expand Up @@ -99,11 +85,10 @@ def reset

if ENV["RENDER_INSTANCE_ID"]
instance = ENV["RENDER_INSTANCE_ID"].delete_prefix(ENV["RENDER_SERVICE_ID"]).delete_prefix("-")
@current_runtime_container = RuntimeContainer.new ENV["RENDER_SERVICE_ID"], instance
@current_runtime_container = RuntimeContainer.new instance
@api_base_url ||= "https://adapter.judoscale.com/api/#{ENV["RENDER_SERVICE_ID"]}"
elsif ENV["DYNO"]
service_name, instance = ENV["DYNO"].split "."
@current_runtime_container = RuntimeContainer.new service_name, instance
@current_runtime_container = RuntimeContainer.new ENV["DYNO"]
else
# unsupported platform? Don't want to leave @current_runtime_container nil though
Copy link
Member

Choose a reason for hiding this comment

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

Might want to drop this comment now 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it no longer valid? I still found it helpful, assuming we don't want @current_runtime_container to be nil.

Copy link
Member

Choose a reason for hiding this comment

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

We don't, but the "unsupported platform" bit isn't a valid assumption anymore, now that we know there are platforms which the correct functionality is to have the bare RuntimeContainer.new 🤔 so maybe just the first 2 words of the comment 😆

@current_runtime_container = RuntimeContainer.new
Expand Down
10 changes: 5 additions & 5 deletions judoscale-ruby/test/config_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module Judoscale
use_env env do
config = Config.instance
_(config.api_base_url).must_equal "https://example.com"
_(config.current_runtime_container.to_s).must_equal "web.1"
_(config.current_runtime_container).must_equal "web.1"
_(config.log_level).must_be_nil
_(config.logger).must_be_instance_of ::Logger
_(config.max_request_size_bytes).must_equal 100_000
Expand Down Expand Up @@ -42,7 +42,7 @@ module Judoscale
use_env env do
config = Config.instance
_(config.api_base_url).must_equal "https://adapter.judoscale.com/api/srv-cfa1es5a49987h4vcvfg"
_(config.current_runtime_container.to_s).must_equal "srv-cfa1es5a49987h4vcvfg.5497f74465-m5wwr"
_(config.current_runtime_container).must_equal "5497f74465-m5wwr"
_(config.log_level).must_be_nil
_(config.logger).must_be_instance_of ::Logger
_(config.max_request_size_bytes).must_equal 100_000
Expand Down Expand Up @@ -70,7 +70,7 @@ module Judoscale
use_env env do
config = Config.instance
_(config.api_base_url).must_equal "https://custom.example.com"
_(config.current_runtime_container.to_s).must_equal "web.2"
_(config.current_runtime_container).must_equal "web.2"
_(config.log_level).must_equal ::Logger::Severity::DEBUG
end
end
Expand All @@ -92,7 +92,7 @@ module Judoscale
test_logger = ::Logger.new(StringIO.new)

Judoscale.configure do |config|
config.current_runtime_container = Config::RuntimeContainer.new("web", "3")
config.current_runtime_container = Config::RuntimeContainer.new("web.3")

config.api_base_url = "https://block.example.com"
config.log_level = :info
Expand All @@ -106,7 +106,7 @@ module Judoscale

config = Config.instance
_(config.api_base_url).must_equal "https://block.example.com"
_(config.current_runtime_container.to_s).must_equal "web.3"
_(config.current_runtime_container).must_equal "web.3"
_(config.log_level).must_equal ::Logger::Severity::INFO
_(config.logger).must_equal test_logger
_(config.max_request_size_bytes).must_equal 50_000
Expand Down
30 changes: 15 additions & 15 deletions judoscale-ruby/test/job_metrics_collector_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,27 @@ module Judoscale
describe JobMetricsCollector do
describe ".collect?" do
it "collects only from the first container in the formation (if we know that), to avoid redundant collection from multiple containers when possible" do
[
["web", "1"],
["worker", "1"],
["custom_name", "1"],
["srv-cfa1es5a49987h4vcvfg", "5497f74465-m5wwr"],
["srv-cfa1es5a49987h4vcvfg", "aaacff2165-m5wwr"]
].each do |args|
%w[
web.1
worker.1
custom_name.1
5497f74465-m5wwr
aaacff2165-m5wwr
].each do |container_id|
Judoscale.configure do |config|
config.current_runtime_container = Config::RuntimeContainer.new(*args)
config.current_runtime_container = Config::RuntimeContainer.new(container_id)
end

_(Test::TestJobMetricsCollector.collect?(Judoscale::Config.instance)).must_equal true
end

[
["web", "2"],
["worker", "8"],
["custom_name", "15"]
].each do |args|
%w[
web.2
worker.8
custom_name.15
].each do |container_id|
Judoscale.configure do |config|
config.current_runtime_container = Config::RuntimeContainer.new(*args)
config.current_runtime_container = Config::RuntimeContainer.new(container_id)
end

_(Test::TestJobMetricsCollector.collect?(Judoscale::Config.instance)).must_equal false
Expand All @@ -36,7 +36,7 @@ module Judoscale

it "skips collecting if the adapter has been explicitly disabled" do
Judoscale.configure { |config|
config.current_runtime_container = Config::RuntimeContainer.new("web", "1")
config.current_runtime_container = Config::RuntimeContainer.new("web.1")
config.test_job_config.enabled = true
}

Expand Down
4 changes: 2 additions & 2 deletions judoscale-ruby/test/reporter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module Judoscale
describe Reporter do
before {
Judoscale.configure do |config|
config.current_runtime_container = Config::RuntimeContainer.new("web", "1")
config.current_runtime_container = Config::RuntimeContainer.new("web.1")
config.api_base_url = "http://example.com/api/test-token"
end
}
Expand Down Expand Up @@ -86,7 +86,7 @@ def stub_reporter_loop

it "initializes the reporter only with registered web metrics collectors on subsequent runtime containers to avoid redundant worker metrics" do
Judoscale.configure do |config|
config.current_runtime_container = Config::RuntimeContainer.new("web", "2")
config.current_runtime_container = Config::RuntimeContainer.new("web.2")
end

run_loop_stub = proc do |config, metrics_collectors|
Expand Down
Loading