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

HTTP client options #467

Open
danschultzer opened this issue Feb 6, 2023 · 10 comments · May be fixed by #579
Open

HTTP client options #467

danschultzer opened this issue Feb 6, 2023 · 10 comments · May be fixed by #579
Assignees

Comments

@danschultzer
Copy link
Contributor

danschultzer commented Feb 6, 2023

I've experienced issues with honeybadger not working in a IPv6 only network. This is due to a long running issue in hackney of how IP addresses are resolved: benoitc/hackney#206 (in particular this logic).

Looking at how honeybadger deals with HTTP it would be good to:

  1. Allow options to be passed to hackney (this won't solve my particular issue though edit: should work)
  2. Allow option to chose HTTP client
    a. Switch to a flexible library like Tesla
    b. Add an abstraction in honeybadger (this is how I handle it in https://github.com/pow-auth/assent/tree/main/lib/assent to support built-in httpc by default)

A third option, which is about solving the IPv6 issue, is to switch to something other than hackney since upstream fix seems unlikely to happen anytime soon. I solved my issue by switching to Finch and pass in conn_opts: https://github.com/danschultzer/honeybadger-elixir/commit/14bdbe21f6ea754c335478ccb82bb6fa312993a7

@sorentwo sorentwo self-assigned this Feb 6, 2023
@sorentwo
Copy link
Collaborator

sorentwo commented Feb 6, 2023

@danschultzer Thanks for opening this issue; it's something I've thought about a lot recently. An adapter that uses httpc by default, with an easy switch to finch, is perfect.

@joshuap I'll take this one.

@danschultzer
Copy link
Contributor Author

Sounds great @sorentwo, thanks for looking at this!

I did make a quick PR in #468 to pass hackney opts which solves part of the issue. But it would be nice to just pass in an adapter to deal with edge cases and have more control over the connection egress.

Just a warning on :httpc that the cert check has to be handled explicitly (not built-in 😞). But apart from that and no HTTP/2 support, it works fine, and the benefit is that it helps limit the dependency graph size.

@fastjames
Copy link

Is there still any interest in implementing this change? As of today hackney has a low-priority CVE with no fix listed (and no obvious fix activity on github). I'd love to swap it out for a different client if possible.

@stympy
Copy link
Member

stympy commented Feb 11, 2025

@fastjames Yup!

@fastjames
Copy link

@fastjames Yup!

Well, it's good to know that someone else is interested as well. I read over some of the earlier links on this issue, and @danschultzer 's work in assent looked like it might work here too. I took a stab at shamelessly copying what he did, and I think I'm getting somewhere (see https://github.com/fastjames/honeybadger-elixir/tree/fastjames/feature/multi_http_client_support for my work so far).

Important questions still to be answered:

  1. Is hackney support still important? I'm working on that now, but if literally no one wants it then 💥 .
  2. How important is pool tuning? The original hackney implementation created its own pool with a connection limit of 30. It looks like finch supports pooling (which means req does), but for :httpc we would have to provide our own pool implementation.

@sorentwo
Copy link
Collaborator

Is there still any interest in implementing this change?

I'm still interested in this change, but I dropped the ball implementing it.

@fastjames Thanks for taking a shot at this. As to your questions:

Is hackney support still important? I'm working on that now, but if literally no one wants it then

It should be possible to use other adapters. A httpc base is nice, but people should be able to use Req or other upcoming options.

How important is pool tuning?

Switching to httpc without a pool option could cause a problem in clients that experience a high volume of errors. This is tricky to answer, because using a custom pooler would negate most of the benefit of using httpc over finch.

@fastjames
Copy link

It should be possible to use other adapters. A httpc base is nice, but people should be able to use Req or other upcoming options.

How important is pool tuning?

Switching to httpc without a pool option could cause a problem in clients that experience a high volume of errors. This is tricky to answer, because using a custom pooler would negate most of the benefit of using httpc over finch.

Thanks for the feedback! Based on these answers, it sounds like the best course of action is to do the following:

  1. For now, drop httpc as an out-of-the-box supported client. Someone can add it later, but the complexity might get in the way of shipping the larger multi-client feature.
  2. Keep hackney in for now, at least for historical purposes.
  3. Ensure that each supported client has a proper pool configuration.

@sorentwo
Copy link
Collaborator

Thanks for the feedback! Based on these answers, it sounds like the best course of action is to do the following:

I agree with your assessment, except for ensuring each supported client has a pool. As long as we ensure the default has a pool for backward compatibility, people should be able to plug in any other http adapter they like. WDYT?

@fastjames
Copy link

Thanks for the feedback! Based on these answers, it sounds like the best course of action is to do the following:

I agree with your assessment, except for ensuring each supported client has a pool. As long as we ensure the default has a pool for backward compatibility, people should be able to plug in any other http adapter they like. WDYT?

That's fair, and a good narrowing of scope (which I always like).

Since req seems to be pretty solid and since it already handles pools for HTTP/1, I would like to propose we make that the default. Does that sound reasonable? The elixir application I'm currently working on is not high-volume so I would like to have input from others who may be more affected by this change.

@fastjames fastjames linked a pull request Feb 13, 2025 that will close this issue
@fastjames
Copy link

I have opened the PR. I'm happy to squash it if everyone is OK with dropping :httpc for this iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants