Add support for multiple HTTP client libraries #579
Conversation
|
The most recent test failures happen for me locally with following elixir/OTP version combos:
I also tried 1.15.8-otp-24 / 24.3.4.17 but this OTP appears to be missing some required telemetry-related capabilities so it wouldn't run at all. I found a few references to the errors seen here in issues on erlang/otp:
I read through the docs for test_server and cowboy but I haven't yet found a way to configure which TLS versions the test server supports so that I can test the hypotheses contained in the linked erlang/otp issues. |
|
@fastjames Will you re-push to trigger a new build? The logs are gone and I can't see what the original errors were. I'm confident there is some configuration or package changes we can make to ensure Elixir v1.15 support. |
fastjames
force-pushed
the
fastjames/feature/multi_http_client_support
branch
from
bee10eb to
5738314
Compare
I have reordered the last 2 commits and pushed, so I think that did the trick. |
|
@sorentwo Here is the downloaded log archive from the failing 1.15 test. Hopefully this contains the information we need to fix it up. |
lib/honeybadger/http_adapter.ex
Outdated
| {:ok, %{status: status} = resp} when status in 200..399 -> | ||
| {:ok, %{resp | http_adapter: http_adapter, request_url: url}} | ||
|
|
||
| {:ok, %{status: status} = resp} when status in 400..599 -> | ||
| {:error, %{resp | http_adapter: http_adapter, request_url: url}} | ||
|
|
||
| {:error, error} -> | ||
| {:error, error} |
There was a problem hiding this comment.
WDYT about inlining these into the case above, rather than piping into two cases?
There was a problem hiding this comment.
I took a shot at this and collapsing them into one case didn't feel right. It seemed to work out better as a with. That required me to flip the error handling around, so I might need to make more changes / cover more cases there.
| default_http_adapter = Application.get_env(:honeybadger, :http_adapter, @default_http_client) | ||
|
|
||
| case Keyword.get(opts, :http_adapter, default_http_adapter) do |
There was a problem hiding this comment.
The difference between default_http_client and default_http_adapter tripped me up briefly.
There was a problem hiding this comment.
Would @default_http_client_lib be a clearer name?
| test "handles SSL" do | ||
| TestServer.start(scheme: :https) | ||
| TestServer.add("/", via: :get) | ||
|
|
||
| req_opts = | ||
| Keyword.put( | ||
| @req_opts, | ||
| :connect_options, | ||
| transport_opts: [cacerts: TestServer.x509_suite().cacerts], | ||
| protocols: [:http2] | ||
| ) | ||
|
|
||
| assert {:ok, %HTTPResponse{status: 200, body: "HTTP/2"}} = | ||
| Req.request(:get, TestServer.url(), nil, [], req_opts) | ||
| end |
There was a problem hiding this comment.
This is testing the functionality of req, and thereby finch and mint, which seems outside the scope of what the honeybadger library should care about.
There was a problem hiding this comment.
OK, I can see removing this test for that reason. I don't mind it being there to keep parity with other adapter test suites (e.g. hackney) but you're right that this one isn't testing much code in the library itself.
I removed the test shown above.
Allow library consumers to choose an http client library. Code shamelessly cribbed from [assent](https://github.com/pow-auth/assent).
Since we are setting req as the default library, include it in our test application.
In order to speed up this iteration, defer implementation of an `:httpc` based adapter until a later time.
Co-authored-by: Parker Selbert <[email protected]>
Co-authored-by: Parker Selbert <[email protected]>
Co-authored-by: Parker Selbert <[email protected]>
Co-authored-by: Parker Selbert <[email protected]>
Since `req` is built on top of `finch` it is unlikely that most consumers would want to use `finch` directly. To reduce maintenance surface, remove the finch adapter.
fastjames
force-pushed
the
fastjames/feature/multi_http_client_support
branch
from
16ceca6 to
ac33150
Compare
Since req handles response body decoding for us, move the hackney-specific response handling code into the adapter module.
fastjames
force-pushed
the
fastjames/feature/multi_http_client_support
branch
from
5704500 to
fa3dde2
Compare
This happy-path test exercises code that lives outside of this library, so its value is low (changes in this repo are unlikely to cause it to fail).
|
Thanks for retriggering CI -- it looks like the failures on 1.16 / OTP26 are the same as before. |
In order to avoid library lock-in and allow customers to use the HTTP client library that best fits their needs, introduce the
HTTPAdapterpattern with initial support for req (default), finch, and :hackney.closes #467