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
Add time tracking to http status agent #1517
Conversation
cc/ @cantino |
if result = ping(url) | ||
create_event payload: { 'url' => url, 'status' => result.status.to_s, 'response_received' => true } | ||
memory['last_status'] = result.status.to_s | ||
measured_result = TimeTracker.track { ping(url) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TimeTracker
seems useful. I'm curious what the advantages over something like this are, though:
start = Time.now.to_f
result = ping(url)
total_time = Time.now.to_f - start
if result
create_event payload: { 'url' => url, 'status' => measured_result.status.to_s, 'response_received' => true, 'elapsed_time' => total_time }
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a point regarding the source of time. I think I'll swap it for a MONOTONIC one so we have the time only going forward.
The use of an object is basically to allow simple reutilization and not pollute the check code with the timing itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I prefer this just because it's significantly less code than pulling it into an object, unless we have a few places to reuse it.
Edit: if you do want to keep the object, it should be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes total sense and I was being extremely lazy for not covering with tests 😄
Good idea! |
@dsander, do you have any thoughts on the Docker spec failure? |
@cantino Looks like a random failure, I restarted the job and it worked. |
Looks good @pcarranza, thanks! |
I missed having how much time it took to get the http status from the url, so I added it myself.
The idea is to use a peak detector to check availability of a site.