-
-
Notifications
You must be signed in to change notification settings - Fork 202
Switch HTTP Client from Hackney to Finch #897
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
base: master
Are you sure you want to change the base?
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Switch HTTP Client from Hackney to Finch ([#897](https://github.com/getsentry/sentry-elixir/pull/897)) If none of the above apply, you can opt out of this check by adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely work. Left a few first-pass comments, but they're mostly nits to clean this up for a nice release. Let me know if you have questions and otherwise let me know when this is ready for another review pass 🙃
|
||
```elixir | ||
defp deps do | ||
[ | ||
# ... | ||
|
||
{:sentry, "~> 10.8"}, | ||
{:hackney, "~> 1.20"} | ||
{:jason, "~> 1.4"}, | ||
{:finch, "~> 0.19"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but let's suggest a version requirement that wouldn't technically allow breaking versions (since this is before 1.0):
{:finch, "~> 0.19"} | |
{:finch, "~> 0.19.0"} |
@@ -67,7 +67,7 @@ defmodule Mix.Tasks.Sentry.SendTestEvent do | |||
end | |||
|
|||
Mix.shell().info("current environment_name: #{inspect(to_string(Config.environment_name()))}") | |||
Mix.shell().info("hackney_opts: #{inspect(Config.hackney_opts())}\n") | |||
Mix.shell().info("finch_pool_opts: #{inspect(Config.finch_pool_opts())}\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're touching this... a nit! 😉
Mix.shell().info("finch_pool_opts: #{inspect(Config.finch_pool_opts())}\n") | |
Mix.shell().info("Finch pool options: #{inspect(Config.finch_pool_opts(), pretty: true)}\n") |
lib/sentry/config.ex
Outdated
applied if `:client` is set to `Sentry.HackneyClient`. | ||
""" | ||
] ++ | ||
if(Mix.env() == :test, do: [], else: [deprecated: "Use Finch instead as default client."]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
if(Mix.env() == :test, do: [], else: [deprecated: "Use Finch instead as default client."]), | |
if(Mix.env() == :test, do: [], else: [deprecated: "Use Finch as the default HTTP client instead."]), |
lib/sentry/config.ex
Outdated
type: :keyword_list, | ||
default: [pool: :sentry_pool], | ||
default: [size: 50, conn_max_idle_time: 5000], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the 5000
idle time? I think Finch's default of :infinity
is a good choice here?
@behaviour Sentry.HTTPClient | ||
|
||
@moduledoc """ | ||
The built-in HTTP client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shows up correctly in the module doc summary with ExDoc:
The built-in HTTP client. | |
The built-in HTTP client. | |
see "Pool Configuration Options" in the Finch documentation for details on the possible map values. | ||
[finch configuration options](https://hexdocs.pm/finch/Finch.html#start_link/1-pool-configuration-options) | ||
""" | ||
@impl true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
@impl true | |
@impl true |
Finch is built on top of [NimblePool](https://github.com/dashbitco/nimble_pool). If you need to set other pool configuration options, | ||
see "Pool Configuration Options" in the Finch documentation for details on the possible map values. | ||
[finch configuration options](https://hexdocs.pm/finch/Finch.html#start_link/1-pool-configuration-options) | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
""" | |
""" | |
@moduledoc since: "10.11.0" |
lib/sentry/http_client.ex
Outdated
@@ -2,7 +2,7 @@ defmodule Sentry.HTTPClient do | |||
@moduledoc """ | |||
A behaviour for HTTP clients that Sentry can use. | |||
|
|||
The default HTTP client is `Sentry.HackneyClient`. | |||
The default HTTP client is `Sentry.FinchClient`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default HTTP client is `Sentry.FinchClient`. | |
The default HTTP client is `Sentry.FinchClient` since v10.11.0 | |
(it was based on Hackney before that). |
mix.exs
Outdated
@@ -97,6 +97,7 @@ defmodule Sentry.Mixfile do | |||
|
|||
# Optional dependencies | |||
{:hackney, "~> 1.8", optional: true}, | |||
{:finch, "~> 0.19", optional: true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can even go a couple of versions below this to allow more people to use this in case they're stuck on an older Finch version. Something like ~> 0.17
. You can look at Finch's changelog and figure out a good balance there 🙃
@whatyouhide made all the feedback changes! One thing I forgot to mention is the http_client module is still using Finch as an example to add your own.. Should this be updated to hackney or req? Or good as is? |
@savhappy updated to use Req (in another PR) would be great I think 🙃. We also have:
|
Switch default HTTP client from Hackney to Finch
Changes
Sentry.HackneyClient
toSentry.FinchClient
finch_pool_opts
: For pool configuration (defaults to[size: 50, conn_max_idle_time: 5000]
)finch_request_opts
: For request configuration (defaults to[receive_timeout: 5000]
)Migration
Users who want to continue using Hackney can:
Dependencies
Testing
Documentation