Conversation
ghost
requested review from
| """ | ||
| use Tesla | ||
|
|
||
| adapter(Tesla.Adapter.Hackney) |
There was a problem hiding this comment.
I'd be curious to try the Finch adapter here instead of Hackney. Unless there was specific reason that Hackney was chosen?
There was a problem hiding this comment.
No reason Hackney was chosen - I can certainly use Finch. What kind of perks does Finch have over Hackney?
There was a problem hiding this comment.
I am currently using Finch and I must say - I did like the error messages that Hackney would give me when I passed invalid URLs into requests. This should not be that much of a problem because in order for a site to be checked, it has to pass an ecto validation when the site is created. Since this is the case, is there any value in testing for invalid URLs when performing site checks?
| {:ok, response} -> | ||
| {:ok, %{"status" => response.status, "response_time" => time / 1000}} | ||
|
|
||
| {:error, {reason}} -> |
There was a problem hiding this comment.
In what case is this the return tuple vs the pattern on line 19? Looking at the Tesla.Env.result() typing, this pattern doesn't seem to be a possible return type.
There was a problem hiding this comment.
There were random cases where a tuple was being returned in my tests, so I added this in to pattern match those cases.
There was a problem hiding this comment.
After switching adapters, I think that response tuple may have been specific to Hackney? Now I do not receive them.
| {:error, reason} -> | ||
| {:error, reason} | ||
| end | ||
| rescue |
There was a problem hiding this comment.
What is being rescued here and why? Why the rescue vs graceful handling and/or just let the process die and restart?
There was a problem hiding this comment.
Prior to this, I thought of rescue as a 'catchall' to fallback on. In hindsight this is a poor way of dealing with an error that I was expecting. When given a bad input for the run function, such as a number or an empty string, the process will die. I suppose a function spec or a guard clause could catch this error as well. I will also look into graceful handling.
|
I think you might be tackling too many issues in this PR too. Maybe we can just make it about the checks and the GenServer can come later? You aren't fully finishing either feature so maybe just create a way to trigger a single check, or some kind of check manager that can call multiple checks, and handle all of the success and failures. Also take a look at implementing |
I like this idea. I will split this up into different PRs. One for the general check logic and one for the supervisor/genserver implementation. |
2 participants
This PR starts running checks for all sites that exist when the server is started.
Each site gets its own genserver that will run a check on the sites specified interval.
So Far, this PR does NOT: