Skip to content

Commit

Permalink
More comprehensively handle potential NaNs
Browse files Browse the repository at this point in the history
  • Loading branch information
sj26 committed Mar 6, 2023
1 parent 854f6f3 commit 32d3ed5
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 18 deletions.
43 changes: 25 additions & 18 deletions lib/sidekiq/cloudwatchmetrics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -152,31 +152,35 @@ def publish
},
]

# Only publish utilization once there is some capacity, otherwise we'll
# publish metrics with an invalid value of NaN.
if processes.any?
utilization = calculate_utilization(processes) * 100.0

unless utilization.nan?
metrics << {
metric_name: "Utilization",
timestamp: now,
value: calculate_utilization(processes) * 100.0,
value: utilization,
unit: "Percent",
}
end

processes.each do |process|
process_dimensions = [{name: "Hostname", value: process["hostname"]}]

if process["tag"]
process_dimensions << {name: "Tag", value: process["tag"]}
process_utilization = process["busy"] / process["concurrency"].to_f * 100.0

unless process_utilization.nan?
process_dimensions = [{name: "Hostname", value: process["hostname"]}]

if process["tag"]
process_dimensions << {name: "Tag", value: process["tag"]}
end

metrics << {
metric_name: "Utilization",
dimensions: process_dimensions,
timestamp: now,
value: process_utilization,
unit: "Percent",
}
end

metrics << {
metric_name: "Utilization",
dimensions: process_dimensions,
timestamp: now,
value: process["busy"] / process["concurrency"].to_f * 100.0,
unit: "Percent",
}
end

queues.each do |(queue_name, queue_size)|
Expand Down Expand Up @@ -222,10 +226,13 @@ def publish
end

# Returns busy / concurrency averaged across processes (for scaling)
# Avoid considering processes not yet running any threads
private def calculate_utilization(processes)
processes.map do |process|
process_utilizations = processes.map do |process|
process["busy"] / process["concurrency"].to_f
end.sum / processes.size.to_f
end.reject(&:nan?)

process_utilizations.sum / process_utilizations.size.to_f
end

def quiet
Expand Down
47 changes: 47 additions & 0 deletions spec/sidekiq/cloudwatchmetrics_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,53 @@
end
end
end

context "when the only process has no threads yet" do
let(:processes) { [Sidekiq::Process.new("busy" => 0, "concurrency" => 0, "hostname" => "foo")] }

it "does not publish Utilization (to avoid NaN values)" do
Timecop.freeze(now = Time.now) do
publisher.publish

expect(client).to have_received(:put_metric_data) { |metrics|
expect(metrics[:metric_data]).not_to include(hash_including(metric_name: "Utilization"))
}
end
end
end

context "when only one process has no threads yet" do
let(:processes) { [
Sidekiq::Process.new("busy" => 0, "concurrency" => 0, "hostname" => "foo"),
Sidekiq::Process.new("busy" => 2, "concurrency" => 4, "hostname" => "bar"),
] }

it "publishes partial Utilization (to avoid NaN values)" do
Timecop.freeze(now = Time.now) do
publisher.publish

expect(client).to have_received(:put_metric_data) { |metrics|
utilization_data = metrics[:metric_data].select { |data| data[:metric_name] == "Utilization" }

expect(utilization_data).to contain_exactly(
{
metric_name: "Utilization",
timestamp: now,
value: 50.0,
unit: "Percent",
},
{
metric_name: "Utilization",
dimensions: [{name: "Hostname", value: "bar"}],
timestamp: now,
unit: "Percent",
value: 50.0,
},
)
}
end
end
end
end

describe "#stop" do
Expand Down

0 comments on commit 32d3ed5

Please sign in to comment.