Skip to content

FIX: Ensure socket is closed when error is raised while opening socket #328

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

Merged
merged 2 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
2.2.0 - 2024-12-05

- FIX: Ensure socket is closed when error is raised while opening socket
- Feature: Add Dalli::Client memcache metrics for web_collector
Copy link
Member

@nbianca nbianca Dec 5, 2024

Choose a reason for hiding this comment

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

Nitpick:

- FEATURE: Add Dalli::Client memcache metrics for web_collector


2.1.1 - 2024-06-19

- FEATURE: improve good_job instrumentation
Expand Down
14 changes: 10 additions & 4 deletions lib/prometheus_exporter/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ def initialize(
json_serializer: nil,
custom_labels: nil,
logger: Logger.new(STDERR),
log_level: Logger::WARN
log_level: Logger::WARN,
process_queue_once_and_stop: false
)
@logger = logger
@logger.level = log_level
Expand All @@ -86,6 +87,7 @@ def initialize(
@json_serializer = json_serializer == :oj ? PrometheusExporter::OjCompat : JSON

@custom_labels = custom_labels
@process_queue_once_and_stop = process_queue_once_and_stop
end

def custom_labels=(custom_labels)
Expand Down Expand Up @@ -144,7 +146,7 @@ def process_queue
@socket.write("\r\n")
rescue => e
logger.warn "Prometheus Exporter is dropping a message: #{e}"
@socket = nil
close_socket!
raise
end
end
Expand All @@ -170,6 +172,11 @@ def worker_loop
end

def ensure_worker_thread!
if @process_queue_once_and_stop
worker_loop
return
end

unless @worker_thread&.alive?
@mutex.synchronize do
return if @worker_thread&.alive?
Expand Down Expand Up @@ -234,8 +241,7 @@ def ensure_socket!

nil
rescue StandardError
@socket = nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, we were just assigning the @socket to nil which causes a TCPSocket to remain open.

Copy link

Choose a reason for hiding this comment

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

Do we need an explicit ensure close_socket! in worker_loop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the worker loop is meant to ensure that a socket is opened and reuse the socket across loops. Is there a reason you think we should close the socket each time we process a message?

Copy link

@nattsw nattsw Dec 5, 2024

Choose a reason for hiding this comment

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

Not so much for each time we process a message, but I'm tracing the flow right now and I do see other places e.g. process_queue, where there's @socket = nil as well

logger.warn "Prometheus Exporter is dropping a message: #{e}"
@socket = nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch and I've made the change to call close_socket!. However, I don't think we can easily test for this and I think it is OK since the risk of dropping a message is low.

@socket_started = nil
close_socket!
@socket_pid = nil
raise
end
Expand Down
2 changes: 1 addition & 1 deletion lib/prometheus_exporter/version.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# frozen_string_literal: true

module PrometheusExporter
VERSION = "2.1.1"
VERSION = "2.2.0"
end
22 changes: 21 additions & 1 deletion test/client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,32 @@ def test_standard_values
assert_equal(expected_quantiles, summary_metric.standard_values("value", "key")[:opts])
end

def test_close_socket_on_error
logs = StringIO.new
logger = Logger.new(logs)
logger.level = :error

client =
PrometheusExporter::Client.new(logger: logger, port: 321, process_queue_once_and_stop: true)
client.send("put a message in the queue")

assert_includes(
logs.string,
"Prometheus Exporter, failed to send message Connection refused - connect(2) for \"localhost\" port 321",
)
end

def test_overriding_logger
logs = StringIO.new
logger = Logger.new(logs)
logger.level = :warn

client = PrometheusExporter::Client.new(logger: logger, max_queue_size: 1)
client =
PrometheusExporter::Client.new(
logger: logger,
max_queue_size: 1,
process_queue_once_and_stop: true,
)
client.send("put a message in the queue")
client.send("put a second message in the queue to trigger the logger")

Expand Down
Loading