-
Notifications
You must be signed in to change notification settings - Fork 161
Feat; add a duration column for benchmark_request's #2220
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
Feat; add a duration column for benchmark_request's #2220
Conversation
database/src/pool/postgres.rs
Outdated
| MIN(EXTRACT('epoch' FROM job_queue.started_at))) * 1000 AS duration_ms | ||
| FROM | ||
| job_queue | ||
| LEFT JOIN benchmark_request ON job_queue.request_tag = benchmark_request.tag |
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.
This has to be done with some other way, right? This would just take a look at the largest completed_at and oldest started_at in the job queue, regardless of the benchmark_request(s) that we're completing. Maybe we can do it in two steps? First finish the requests, and then fill in the duration for the steps that were completed, but have no duration set yet.
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 you're right the query is incorrect. I think we can add;
WHERE benchmark_request.tag = $<the_tag>Then we will only get the jobs associated with the request. Which means we can keep it as one query.
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.
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.
fceeab8 I've simplified it further, all it needs is the WHERE not the LEFT JOIN
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.
Ah, I forgot that we try to finish a specific tag, I was somehow assuming that we just finish all requests that are done, but that's no longer the case. Yeah, looks good! Could you please add a test?
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.
I forgot that we try to finish a specific tag, I was somehow assuming that we just finish all requests that are done
We are doing this as this function is called inside a loop in the cron job. Just we're not doing it as one query (though we could, with some effort, probably do that in 1 query).
For the test I've modified a pre-existing one; 030948b.
Ideally we'd add roughly 3 requests, add 3 jobs per request, mark 2 of the requests as complete, modify the dates for started_at and completed_at per job belonging to the completed requests and then assert the duration_ms is set as expected for the 2 completed requests and that the duration_ms is NULL for the in_progress job. I can't see how we can do something like this with our current setup however.
1f070c0 to
37b3460
Compare
… jobs associated with the request matching the tag
012457b to
d21659f
Compare
Adds a
duration_mscolumn to thebenchmark_requesttable. It takes the difference from the latestcompleted_atand the earlieststarted_atfrom jobs associated with the request. This column is populated when marking a request as completed.