From 83a74960d8d9595dab25dc091f09fe61d469a495 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luismi=20Rami=CC=81rez?= Date: Wed, 26 Feb 2025 15:57:36 +0100 Subject: [PATCH] Switch Hackney to Finch Transition the HTTP client usage from Hackney to Finch to optimize requests performance. --- .changesets/switch-hackney-with-finch.md | 6 ++++ lib/appsignal.ex | 2 +- lib/appsignal/check_in/scheduler.ex | 4 +-- lib/appsignal/diagnose/report.ex | 9 ++---- lib/appsignal/transmitter.ex | 21 ++++++------ lib/appsignal/utils/push_api_key_validator.ex | 2 +- mix.exs | 10 ++---- mix_helpers.exs | 32 +++++++++++-------- test/appsignal/diagnose/report_test.exs | 2 +- test/appsignal/transmitter_test.exs | 2 +- .../utils/push_api_key_validator_test.exs | 2 +- test/support/fake_finch.ex | 9 ++++++ test/support/fake_hackney.ex | 5 --- 13 files changed, 56 insertions(+), 50 deletions(-) create mode 100644 .changesets/switch-hackney-with-finch.md create mode 100644 test/support/fake_finch.ex delete mode 100644 test/support/fake_hackney.ex diff --git a/.changesets/switch-hackney-with-finch.md b/.changesets/switch-hackney-with-finch.md new file mode 100644 index 000000000..b6ea5d132 --- /dev/null +++ b/.changesets/switch-hackney-with-finch.md @@ -0,0 +1,6 @@ +--- +bump: minor +type: change +--- + +Switch Hackney to Finch as the bundled HTTP client to operate within the integration diff --git a/lib/appsignal.ex b/lib/appsignal.ex index 38878a7ae..a4282fd39 100644 --- a/lib/appsignal.ex +++ b/lib/appsignal.ex @@ -49,7 +49,7 @@ defmodule Appsignal do {Appsignal.Monitor, []}, {Appsignal.Probes, []}, {Appsignal.CheckIn.Scheduler, []}, - :hackney_pool.child_spec(:appsignal_transmitter, []) + {Finch, name: AppsignalFinch} ] result = Supervisor.start_link(children, strategy: :one_for_one, name: Appsignal.Supervisor) diff --git a/lib/appsignal/check_in/scheduler.ex b/lib/appsignal/check_in/scheduler.ex index f28000b77..989e852b3 100644 --- a/lib/appsignal/check_in/scheduler.ex +++ b/lib/appsignal/check_in/scheduler.ex @@ -99,10 +99,10 @@ defmodule Appsignal.CheckIn.Scheduler do endpoint = "#{config[:logging_endpoint]}/check_ins/json" case @transmitter.transmit_and_close(endpoint, {Enum.reverse(events), :ndjson}, config) do - {:ok, status_code, _} when status_code in 200..299 -> + {:ok, %{status: status_code}, _} when status_code in 200..299 -> @integration_logger.trace("Transmitted #{description}") - {:ok, status_code, _} -> + {:ok, %{status: status_code}, _} -> @integration_logger.error( "Failed to transmit #{description}: status code was #{status_code}" ) diff --git a/lib/appsignal/diagnose/report.ex b/lib/appsignal/diagnose/report.ex index bc8ecb8c3..b6789334d 100644 --- a/lib/appsignal/diagnose/report.ex +++ b/lib/appsignal/diagnose/report.ex @@ -11,18 +11,13 @@ defmodule Appsignal.Diagnose.Report do @spec send(map(), map()) :: {:ok, String.t()} | {:error, map()} def send(config, report) do case Transmitter.transmit(config[:diagnose_endpoint], {%{diagnose: report}, :json}, config) do - {:ok, 200, _, reference} -> - {:ok, body} = :hackney.body(reference) - :hackney.close(reference) - + {:ok, %{status: 200, body: body}} -> case Jason.decode(body) do {:ok, response} -> {:ok, response["token"]} {:error, _} -> {:error, %{status_code: 200, body: body}} end - {:ok, status_code, _, reference} -> - {:ok, body} = :hackney.body(reference) - :hackney.close(reference) + {:ok, %{status: status_code, body: body}} -> {:error, %{status_code: status_code, body: body}} {:error, reason} -> diff --git a/lib/appsignal/transmitter.ex b/lib/appsignal/transmitter.ex index dc0e0a48b..280642f0a 100644 --- a/lib/appsignal/transmitter.ex +++ b/lib/appsignal/transmitter.ex @@ -4,19 +4,21 @@ defmodule Appsignal.Transmitter do require Logger def request(method, url, headers \\ [], body \\ "") do - http_client = Application.get_env(:appsignal, :http_client, :hackney) - :application.ensure_all_started(http_client) - - http_client.request(method, url, headers, body, options()) + http_client = Application.get_env(:appsignal, :http_client, Finch) + {:ok, pid} = Finch.start_link(name: AppsignalFinch) + + try do + method + |> http_client.build(url, headers, body) + |> http_client.request(AppsignalFinch, options()) + after + Process.exit(pid, :normal) + end end 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 @@ -40,8 +42,7 @@ defmodule Appsignal.Transmitter do 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: status, headers: headers}} -> {:ok, status, headers} {:error, reason} -> diff --git a/lib/appsignal/utils/push_api_key_validator.ex b/lib/appsignal/utils/push_api_key_validator.ex index 3f884b7b0..e4a50d6a7 100644 --- a/lib/appsignal/utils/push_api_key_validator.ex +++ b/lib/appsignal/utils/push_api_key_validator.ex @@ -9,7 +9,7 @@ defmodule Appsignal.Utils.PushApiKeyValidator do {:ok, 200, _} -> :ok {:ok, 401, _} -> {:error, :invalid} {:ok, status_code, _} -> {:error, status_code} - {:error, reason} -> {:error, reason} + {:error, %{reason: reason}} -> {:error, reason} end end end diff --git a/mix.exs b/mix.exs index 0647db90e..145ceadd2 100644 --- a/mix.exs +++ b/mix.exs @@ -103,12 +103,6 @@ defmodule Appsignal.Mixfile do system_version = System.version() otp_version = System.otp_release() - hackney_version = - case otp_version >= "21" do - true -> "~> 1.6 and <= 1.21.0" - false -> "1.18.1" - end - decorator_version = case Version.compare(system_version, "1.5.0") do :lt -> "~> 1.2.3" @@ -140,9 +134,11 @@ defmodule Appsignal.Mixfile do end [ + {:castore, "~> 1.0"}, + {:certifi, "~> 2.14"}, {:decimal, "~> 2.0"}, {:benchee, "~> 1.0", only: :bench}, - {:hackney, hackney_version}, + {:finch, "~> 0.19"}, {:jason, "~> 1.0"}, {:decorator, decorator_version}, {:plug, plug_version, only: [:test, :test_no_nif]}, diff --git a/mix_helpers.exs b/mix_helpers.exs index bbd751b00..3636cc08f 100644 --- a/mix_helpers.exs +++ b/mix_helpers.exs @@ -144,15 +144,18 @@ defmodule Mix.Appsignal.Helper do false -> Mix.shell().info("Downloading agent release") - :application.unset_env(:hackney, :mod_metrics) - :application.ensure_all_started(:hackney) + {:ok, pid} = Finch.start_link(name: AppsignalFinchDownload) - case do_download_file!(filename, local_filename, Appsignal.Agent.mirrors()) do - {:ok, url} -> - {:ok, {local_filename, merge_report(report, %{download: %{download_url: url}})}} + try do + case do_download_file!(filename, local_filename, Appsignal.Agent.mirrors()) do + {:ok, url} -> + {:ok, {local_filename, merge_report(report, %{download: %{download_url: url}})}} - {error, url} -> - {:error, {error, merge_report(report, %{download: %{download_url: url}})}} + {error, url} -> + {:error, {error, merge_report(report, %{download: %{download_url: url}})}} + end + after + Process.exit(pid, :normal) end end end @@ -210,17 +213,18 @@ defmodule Mix.Appsignal.Helper do end defp do_download_file!(url, local_filename) do - case :hackney.request(:get, url, [], "", download_options()) do - {:ok, 200, _, reference} -> - case :hackney.body(reference) do - {:ok, body} -> File.write(local_filename, body) - {:error, reason} -> {:error, reason} - end + request = + Finch.build(:get, url, [], "") + |> Finch.request(AppsignalFinchDownload, download_options()) + + case request do + {:ok, %{status: 200, body: body}} -> + File.write(local_filename, body) response -> message = """ - URL: #{url} - - Error (hackney response): + - Error (Finch response): #{inspect(response)} """ diff --git a/test/appsignal/diagnose/report_test.exs b/test/appsignal/diagnose/report_test.exs index a4410e113..4ee17e566 100644 --- a/test/appsignal/diagnose/report_test.exs +++ b/test/appsignal/diagnose/report_test.exs @@ -66,7 +66,7 @@ defmodule Mix.Tasks.Appsignal.Diagnose.ReportTest do end test "sends the diagnostics report to AppSignal and returns an error" do - assert send() == {:error, %{body: "woops", status_code: 500}} + assert send() == {:error, %{status_code: 500, body: "woops"}} end end diff --git a/test/appsignal/transmitter_test.exs b/test/appsignal/transmitter_test.exs index fd9604627..63b41f3e7 100644 --- a/test/appsignal/transmitter_test.exs +++ b/test/appsignal/transmitter_test.exs @@ -5,7 +5,7 @@ defmodule Appsignal.TransmitterTest do import ExUnit.CaptureLog setup do - Application.put_env(:appsignal, :http_client, FakeHackney) + Application.put_env(:appsignal, :http_client, FakeFinch) on_exit(fn -> Application.delete_env(:appsignal, :http_client) diff --git a/test/appsignal/utils/push_api_key_validator_test.exs b/test/appsignal/utils/push_api_key_validator_test.exs index 90cfad21d..b3d73bbb0 100644 --- a/test/appsignal/utils/push_api_key_validator_test.exs +++ b/test/appsignal/utils/push_api_key_validator_test.exs @@ -64,7 +64,7 @@ defmodule Appsignal.Utils.PushApiKeyValidatorTest do end test "returns an error", %{config: config} do - assert PushApiKeyValidator.validate(config) == {:error, :econnrefused} + assert PushApiKeyValidator.validate(config) == {:error, :econn_refused} end end end diff --git a/test/support/fake_finch.ex b/test/support/fake_finch.ex new file mode 100644 index 000000000..c877cfafb --- /dev/null +++ b/test/support/fake_finch.ex @@ -0,0 +1,9 @@ +defmodule FakeFinch do + def build(method, url, headers, body) do + [method, url, headers, body] + end + + def request([method, url, headers, body], _, options) do + [method, url, headers, body, options] + end +end diff --git a/test/support/fake_hackney.ex b/test/support/fake_hackney.ex deleted file mode 100644 index 7a4246a03..000000000 --- a/test/support/fake_hackney.ex +++ /dev/null @@ -1,5 +0,0 @@ -defmodule FakeHackney do - def request(method, url, headers, body, options) do - [method, url, headers, body, options] - end -end