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.

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 Hackney pool saturation #971

Merged
merged 2 commits into from
Nov 6, 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
8 changes: 8 additions & 0 deletions .changesets/fix-check-ins-not-being-sent-after-some-time.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
bump: patch
type: fix
---

Fix an issue where, after a certain amount of time, check-ins would no longer be sent.

This issue also caused the default Hackney connection pool to be saturated, affecting other code that uses the default Hackney connection pool.
3 changes: 2 additions & 1 deletion lib/appsignal.ex
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ defmodule Appsignal do
{Appsignal.Tracer, []},
{Appsignal.Monitor, []},
{Appsignal.Probes, []},
{Appsignal.CheckIn.Scheduler, []}
{Appsignal.CheckIn.Scheduler, []},
:hackney_pool.child_spec(:appsignal_transmitter, [])
]

result = Supervisor.start_link(children, strategy: :one_for_one, name: Appsignal.Supervisor)
Expand Down
6 changes: 3 additions & 3 deletions lib/appsignal/check_in/scheduler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,11 @@ defmodule Appsignal.CheckIn.Scheduler do
config = Appsignal.Config.config()
endpoint = "#{config[:logging_endpoint]}/check_ins/json"

case @transmitter.transmit(endpoint, {Enum.reverse(events), :ndjson}, config) do
{:ok, status_code, _, _} when status_code in 200..299 ->
case @transmitter.transmit_and_close(endpoint, {Enum.reverse(events), :ndjson}, config) do
{:ok, status_code, _} when status_code in 200..299 ->
@integration_logger.trace("Transmitted #{description}")

{:ok, status_code, _, _} ->
{:ok, status_code, _} ->
@integration_logger.error(
"Failed to transmit #{description}: status code was #{status_code}"
)
Expand Down
2 changes: 2 additions & 0 deletions lib/appsignal/diagnose/report.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ defmodule Appsignal.Diagnose.Report do
case Transmitter.transmit(config[:diagnose_endpoint], {%{diagnose: report}, :json}, config) do
{:ok, 200, _, reference} ->
{:ok, body} = :hackney.body(reference)
:hackney.close(reference)

case Jason.decode(body) do
{:ok, response} -> {:ok, response["token"]}
Expand All @@ -21,6 +22,7 @@ defmodule Appsignal.Diagnose.Report do

{:ok, status_code, _, reference} ->
{:ok, body} = :hackney.body(reference)
:hackney.close(reference)
{:error, %{status_code: status_code, body: body}}

{:error, reason} ->
Expand Down
24 changes: 24 additions & 0 deletions lib/appsignal/transmitter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,12 @@ defmodule Appsignal.Transmitter do
def transmit(url, payload_and_format \\ {nil, nil}, config \\ nil)
def transmit(url, nil, config), do: transmit(url, {nil, nil}, config)

# This function calls `:hackney.request/5` -- it is the
# caller's responsibility to ensure that `:hackney.close/1` is
# called afterwards.
#
# If you're not interested in the body, only in the status code
# and headers, use `transmit_and_close/3` instead.
def transmit(url, {payload, format}, config) do
config = config || Appsignal.Config.config()

Expand All @@ -32,6 +38,17 @@ defmodule Appsignal.Transmitter do
request(:post, url, headers, body)
end

def transmit_and_close(url, payload_and_format \\ {nil, nil}, config \\ nil) do
case transmit(url, payload_and_format, config) do
{:ok, status, headers, reference} ->
:hackney.close(reference)
{:ok, status, headers}

{:error, reason} ->
{:error, reason}
end
end

defp encode_body(nil, _), do: ""
defp encode_body(payload, :json), do: Jason.encode!(payload)

Expand All @@ -41,6 +58,13 @@ defmodule Appsignal.Transmitter do
end

defp options do
ssl_options() ++
[
pool: :appsignal_transmitter
]
end

defp ssl_options do
ca_file_path = Appsignal.Config.ca_file_path()

options =
Expand Down
8 changes: 4 additions & 4 deletions lib/appsignal/utils/push_api_key_validator.ex
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ defmodule Appsignal.Utils.PushApiKeyValidator do
def validate(config) do
url = "#{config[:endpoint]}/1/auth"

case Transmitter.transmit(url, nil, config) do
{:ok, 200, _, _} -> :ok
{:ok, 401, _, _} -> {:error, :invalid}
{:ok, status_code, _, _} -> {:error, status_code}
case Transmitter.transmit_and_close(url, nil, config) do
{:ok, 200, _} -> :ok
{:ok, 401, _} -> {:error, :invalid}
{:ok, status_code, _} -> {:error, status_code}
{:error, reason} -> {:error, reason}
end
end
Expand Down
13 changes: 9 additions & 4 deletions test/appsignal/transmitter_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,11 @@ defmodule Appsignal.TransmitterTest do
end

test "uses the default CA certificate" do
[_method, _url, _headers, _body, [ssl_options: ssl_options]] =
[_method, _url, _headers, _body, options] =
Transmitter.request(:get, "https://example.com")

ssl_options = Keyword.get(options, :ssl_options)

assert ssl_options[:verify] == :verify_peer
assert ssl_options[:cacertfile] == Config.ca_file_path()

Expand Down Expand Up @@ -123,9 +125,10 @@ defmodule Appsignal.TransmitterTest do
path = "priv/cacert.pem"

with_config(%{ca_file_path: path}, fn ->
[_method, _url, _headers, _body, [ssl_options: ssl_options]] =
[_method, _url, _headers, _body, options] =
Transmitter.request(:get, "https://example.com")

ssl_options = Keyword.get(options, :ssl_options)
assert ssl_options[:cacertfile] == path
end)
end
Expand All @@ -136,8 +139,10 @@ defmodule Appsignal.TransmitterTest do
with_config(%{ca_file_path: path}, fn ->
log =
capture_log(fn ->
assert [_method, _url, _headers, _body, []] =
Transmitter.request(:get, "https://example.com")
[_method, _url, _headers, _body, options] =
Transmitter.request(:get, "https://example.com")

refute Keyword.has_key?(options, :ssl_options)
end)

# credo:disable-for-lines:2 Credo.Check.Readability.MaxLineLength
Expand Down
10 changes: 10 additions & 0 deletions test/support/appsignal/fake_transmitter.ex
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,16 @@ defmodule Appsignal.FakeTransmitter do
Agent.get(__MODULE__, & &1[:response]).()
end

def transmit_and_close(url, payload, config) do
case transmit(url, payload, config) do
{:ok, status, headers, _reference} ->
{:ok, status, headers}

{:error, reason} ->
{:error, reason}
end
end

def transmitted do
Agent.get(__MODULE__, &Enum.reverse(&1[:transmitted]))
end
Expand Down
Loading