-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Switch HTTP client from hackney to finch #758
Open
savhappy
wants to merge
9
commits into
master
Choose a base branch
from
sav/Switch-hackney-to-finch
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
d76d3e5
add initial Finch file and test
savhappy 21392ce
replace config hackney with finch
savhappy c715f51
correct test and doc
savhappy 8eeb116
dialyzer
savhappy 11c2afe
Update lib/mix/tasks/sentry.send_test_event.ex
savhappy eadb1f0
Update lib/sentry/config.ex
savhappy 13a609f
Update lib/sentry/finch_client.ex
savhappy b686c0a
Update lib/sentry/finch_client.ex
savhappy 4d42eeb
fix test with vocab change and keep hackney options
savhappy File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -239,11 +239,11 @@ defmodule Sentry.Config do | |
client: [ | ||
type: :atom, | ||
type_doc: "`t:module/0`", | ||
default: Sentry.HackneyClient, | ||
default: Sentry.FinchClient, | ||
doc: """ | ||
A module that implements the `Sentry.HTTPClient` | ||
behaviour. Defaults to `Sentry.HackneyClient`, which uses | ||
[hackney](https://github.com/benoitc/hackney) as the HTTP client. | ||
behaviour. Defaults to `Sentry.FinchClient`, which uses | ||
[Finch](https://github.com/sneako/finch) as the HTTP client. | ||
""" | ||
], | ||
send_max_attempts: [ | ||
|
@@ -253,8 +253,35 @@ defmodule Sentry.Config do | |
The maximum number of attempts to send an event to Sentry. | ||
""" | ||
], | ||
finch_opts: [ | ||
type: :keyword_list, | ||
default: [pool: :sentry_pool], | ||
doc: """ | ||
Options to be passed to `finch`. Only | ||
applied if `:client` is set to `Sentry.FinchClient`. | ||
""" | ||
], | ||
finch_pool_timeout: [ | ||
type: :timeout, | ||
default: 5000, | ||
doc: """ | ||
The maximum time to wait for a | ||
connection to become available. Only applied if `:client` is set to | ||
`Sentry.FinchClient`. | ||
""" | ||
], | ||
finch_pool_max_connections: [ | ||
type: :pos_integer, | ||
default: 50, | ||
doc: """ | ||
The maximum number of | ||
connections to keep in the pool. Only applied if `:client` is set to | ||
`Sentry.FinchClient`. | ||
""" | ||
], | ||
hackney_opts: [ | ||
type: :keyword_list, | ||
deprecated: "Use Finch instead as default client.", | ||
default: [pool: :sentry_pool], | ||
doc: """ | ||
Options to be passed to `hackney`. Only | ||
|
@@ -263,6 +290,7 @@ defmodule Sentry.Config do | |
], | ||
hackney_pool_timeout: [ | ||
type: :timeout, | ||
deprecated: "Use Finch instead as default client.", | ||
default: 5000, | ||
doc: """ | ||
The maximum time to wait for a | ||
|
@@ -272,6 +300,7 @@ defmodule Sentry.Config do | |
], | ||
hackney_pool_max_connections: [ | ||
type: :pos_integer, | ||
deprecated: "Use Finch instead as default client.", | ||
default: 50, | ||
doc: """ | ||
The maximum number of | ||
|
@@ -470,11 +499,11 @@ defmodule Sentry.Config do | |
@spec environment_name() :: String.t() | nil | ||
def environment_name, do: fetch!(:environment_name) | ||
|
||
@spec max_hackney_connections() :: pos_integer() | ||
def max_hackney_connections, do: fetch!(:hackney_pool_max_connections) | ||
@spec max_finch_connections() :: pos_integer() | ||
def max_finch_connections, do: fetch!(:finch_pool_max_connections) | ||
|
||
@spec hackney_timeout() :: timeout() | ||
def hackney_timeout, do: fetch!(:hackney_pool_timeout) | ||
@spec finch_timeout() :: timeout() | ||
def finch_timeout, do: fetch!(:finch_pool_timeout) | ||
|
||
@spec tags() :: map() | ||
def tags, do: fetch!(:tags) | ||
|
@@ -512,8 +541,8 @@ defmodule Sentry.Config do | |
@spec sample_rate() :: float() | ||
def sample_rate, do: fetch!(:sample_rate) | ||
|
||
@spec hackney_opts() :: keyword() | ||
def hackney_opts, do: fetch!(:hackney_opts) | ||
@spec finch_opts() :: keyword() | ||
def finch_opts, do: fetch!(:finch_opts) | ||
Comment on lines
+544
to
+545
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do we use these? |
||
|
||
@spec before_send() :: (Sentry.Event.t() -> Sentry.Event.t()) | {module(), atom()} | nil | ||
def before_send, do: get(:before_send) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
defmodule Sentry.FinchClient do | ||
@behaviour Sentry.HTTPClient | ||
|
||
@moduledoc """ | ||
The built-in HTTP client. | ||
|
||
This client implements the `Sentry.HTTPClient` behaviour. | ||
|
||
It's based on the [Finch](https://github.com/sneako/finch) Elixir HTTP client, | ||
which is an *optional dependency* of this library. If you wish to use another | ||
HTTP client, you'll have to implement your own `Sentry.HTTPClient`. See the | ||
documentation for `Sentry.HTTPClient` for more information. | ||
|
||
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) | ||
""" | ||
@impl true | ||
def child_spec do | ||
if Code.ensure_loaded?(Finch) do | ||
case Application.ensure_all_started(:finch) do | ||
{:ok, _apps} -> :ok | ||
{:error, reason} -> raise "failed to start the :finch application: #{inspect(reason)}" | ||
end | ||
|
||
Finch.child_spec( | ||
name: __MODULE__, | ||
pools: %{ | ||
:default => [ | ||
size: Sentry.Config.max_finch_connections(), | ||
conn_max_idle_time: Sentry.Config.finch_timeout() | ||
] | ||
} | ||
) | ||
else | ||
raise """ | ||
cannot start the :sentry application because the HTTP client is set to \ | ||
Sentry.FinchClient (which is the default), but the :finch library is not loaded. \ | ||
Add :finch to your dependencies to fix this. | ||
""" | ||
end | ||
end | ||
|
||
@impl true | ||
def post(url, headers, body) do | ||
request = Finch.build(:post, url, headers, body) | ||
|
||
case Finch.request(request, __MODULE__) do | ||
{:ok, %Finch.Response{status: status, headers: headers, body: body}} -> | ||
{:ok, status, headers, body} | ||
|
||
{:error, error} -> | ||
{:error, error} | ||
end | ||
end | ||
end |
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We need to keep
hackney_opts
and deprecate them if we want this to not be a breaking change. NimbleOptions supports deprecating options, check out the docs for that.This also applies to the options below.
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.
Does this mean we need to keep the option:
and the module HackneyClient?? @whatyouhide