Migrate HTTPoison to Finch

[manusajith]

trafficstars

[bundlemon[bot]]

BundleMon

Unchanged files (7)

Status	Path	Size	Limits
:white_check_mark:	static/css/app.css	515.1KB	-
:white_check_mark:	static/js/dashboard.js	287.69KB	-
:white_check_mark:	static/js/app.js	12.13KB	-
:white_check_mark:	static/js/embed.host.js	5.58KB	-
:white_check_mark:	static/js/embed.content.js	5.06KB	-
:white_check_mark:	tracker/js/plausible.js	748B	-
:white_check_mark:	static/js/applyTheme.js	314B	-

No change in files bundle size

Final result: :white_check_mark:

View report in BundleMon website ➡️

---

Current branch size history | Target branch size history

[vinibrsl]

I was talking to @ukutaht about this change, and I think that https://github.com/elixir-tesla/tesla would be a better replacement for HTTPoison. Tesla supports multiple adapters, meaning that we could use Tesla + Finch. It ships with built-in support for test mocks without having to do dependency injection, and most importantly a request retrying feature that can be really useful for us. The con is having another layer of abstraction, but I see more pros than cons.

What are your thoughts?

[manusajith]

I was talking to @ukutaht about this change, and I think that https://github.com/elixir-tesla/tesla would be a better replacement for HTTPoison. Tesla supports multiple adapters, meaning that we could use Tesla + Finch. It ships with built-in support for test mocks without having to do dependency injection, and most importantly a request retrying feature that can be really useful for us. The con is having another layer of abstraction, but I see more pros than cons.

What are your thoughts?

I had considered Tesla and also Req

Req is still in early phases of development, so it could introduce breaking changes.

Tesla has too many abstractions imho, and the community is also somewhat shifting towards using Finch(for at least the newer code/libraries etc).

re: request retying, I am not sure some of the places where we use http libraries would benefit from retries.

re: tests/mocks - I was considering to use bypass, but did not add it to this PR, as I wanted to keep the changes specific to the adapter.

[ukutaht]

Tesla has too many abstractions imho

In general I'm quite averse to introducing new libraries and layers of indirection as well. I liked the Tesla suggestion because it would simplify testing and retries (which I think would be useful in many cases).

@manusajith Bypass seems to send stuff over an actual TCP connection? I really like that because it also exercises the client library itself as opposed to mocking it out. I have way more confidence in tests like that. The only thing we'd need to inject is the URL of the server to talk to which is a good idea to parameterize in any case.

It's a cool suggestion, even if we use Tesla I would prefer to use something like Bypass to get more of the system under test. Then Tesla starts losing some of its value.

I suggested retries as a way to reduce timeout errors:

https://sentry.plausible.io/organizations/sentry/issues/640 https://sentry.plausible.io/organizations/sentry/issues/295 https://sentry.plausible.io/organizations/sentry/issues/1799

etc.

Network requests can never be expected to be 100% reliable. If we're updating someone's subscription for example, I think it's a better experience for the customer if we retry the request automatically instead of them seeing an error and having to retry manually. It would also fall under the umbrella of 'reducing sentry errors'. I don't think every single HTTP request should be retried, we need to decide case-by-case.

I've found that for the GA imports we already do 5 retries before giving up. Even if we don't change any behaviour, it would be nice to encapsulate that logic in the Plausible.HTTPClient so we can reuse it elsewhere.

So in the end, if we use Bypass, then the only thing we would get by using Tesla is retry logic which we already have implemented in ~5 lines of code. I don't think it's worth it.

[manusajith]

@ukutaht this is ready for another review now.

[ukutaht]

Thanks @manusajith