Skip to content

add logic to perform site checks - #54

Open
ghost wants to merge 2 commits into
masterfrom
zjr/check_logic
Open

add logic to perform site checks#54
ghost wants to merge 2 commits into
masterfrom
zjr/check_logic

Conversation

@ghost

@ghost ghost commented Jun 7, 2022

Copy link
Copy Markdown

This PR implements the logic to check a site's status code and response time.

@ghost
ghost requested review from cpjolicoeur and isometriks June 7, 2022 15:28
@ghost
ghost force-pushed the zjr/check_logic branch 4 times, most recently from 5cdef1e to 0bb6a3f Compare June 8, 2022 14:29
Comment thread lib/situation_room/schema/site/site_outage.ex Outdated
@ghost
ghost force-pushed the zjr/check_logic branch 3 times, most recently from 63f5aaa to 62122af Compare June 9, 2022 18:06
Comment thread lib/situation_room/checks.ex Outdated
@ghost
ghost force-pushed the zjr/check_logic branch from 62122af to 7e2ff87 Compare June 9, 2022 18:22
Comment thread lib/situation_room/check/status_code.ex Outdated
Comment thread lib/situation_room/check/manager.ex Outdated
Comment thread lib/situation_room/check/manager.ex Outdated
@ghost
ghost force-pushed the zjr/check_logic branch 2 times, most recently from cbe1a12 to 5348f9d Compare June 9, 2022 18:38
Comment thread lib/situation_room/check/outage.ex Outdated
@behaviour SituationRoom.Check.Behaviour

@impl true
def run(%SituationRoom.Site.Check{} = check) do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use a more descriptive function name here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea on all these checks was to have them all have a @behaviour and thus have the same method name on all of them so we can do something like:

head_checks = [
  Checks.StatusCode,
  Checks.Outage
]

head_checks
|> Enum.map(fn module -> apply(module, "run", site))

but we could still probably rename them to something else

Comment thread lib/situation_room/check/status_code.ex Outdated
Comment thread lib/situation_room/check/outage.ex Outdated
{:ok}
end

def do_outage(%SituationRoom.Site.Check{} = check, _reason) do

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be called create_new_outage()? And then the method that would mark an outage resolved would be something like resolve_outage()?

add :site_id, references(:sites), null: false
add :status_code, :integer, null: false
add :response_time, :float, null: false
add :type, :string, default: "head", null: false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better practice to create a new migration file for this instead of editing a previous one. That way we don't have to all reset our databases to run this.

add :response_time, :float, null: false
add :type, :string, default: "head", null: false
add :headers, {:array, :map}
add :body, :string

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to store the whole body of the response from the check?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here? We definitely don't want/need to store any/all of that data.


case response do
{:ok, %Tesla.Env{} = env} ->
%Check{

@isometriks isometriks Jun 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These check structs don't need to be the the schema structs. You could do a defstruct somewhere and just use it as a DTO. Then each check could insert any information it wanted (by passing both to run), or you could just create the ecto struct somewhere else and not let the checks know about it.

The other option if we do want to pass around the ecto structs is you can make virtual fields on them so they can hold information that doesn't get persisted to the database. (Like Brian's comment about the body field)

@ghost
ghost force-pushed the zjr/check_logic branch from 5348f9d to 42f7e16 Compare June 14, 2022 18:15
@ghost
ghost force-pushed the zjr/check_logic branch from 42f7e16 to 0dd80ed Compare June 14, 2022 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants