- Notifications
You must be signed in to change notification settings - Fork352
Notebook query elapsed time#644
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Notebook query elapsed time#644
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pgml-dashboard/templates/cell.html Outdated
| <divclass="markdown-body"> | ||
| <divclass="flex flex-row-reverse"> | ||
| <code>TODO</code> | ||
| <code>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.
Did you consider moving this inside thesql.html<div>? I think that might be better to track each statement independently, but I only thought it through because of the warning about an unused variable in the template.
Otherwise, I'd move it to the cell template that uses it.
SilasMarvinMay 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Sorry I totally missed the unused variable error. I switched back to stable from nightly and can see it now.
That was my original plan, but then I saw in themodels.rs theCell struct already had anexecution_time attribute and thecell.html had a TODO section for it.
I just pushed a commit that also shows the timing for each sql statement independently. Let me know what you think! Do you want me to remove the rendering of theCell execution_time in thecell.html? If so, should I completely remove theexecution_time attribute for theCell struct and update the migrations to not include that column?
montanalow left a comment
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 is looking pretty good to me, with a couple cleanups.
pgml-dashboard/templates/cell.html Outdated
| microseconds @ 1000000.. => format!("{}s", (microseconds as f64) / 1000000.), | ||
| microseconds @ 1000..=999999 => format!("{}ms", (microseconds as f64) / 1000.), | ||
| microseconds @ 0..=999 => format!("{}μs", microseconds), | ||
| _ => "0".to_string(), |
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 is unreachable, and unnecessary since your conditions are exhaustive (according the the compiler).
pgml-dashboard/templates/sql.html Outdated
| </table> | ||
| <code> | ||
| Time: | ||
| <%- match execution_duration.as_micros() { |
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.
It'd be nice to have a util function to convert a duration to a string consistently throughout the app that can be reused across these placements.
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.
Added a utils.rs module with a function for doing this. I made it take an f64, as we convert it to that regardless of whether we are working with a std::time::Duration or postgres Interval.
pgml-dashboard/templates/sql.html Outdated
| <% } %> | ||
| </tbody> | ||
| </table> | ||
| <code> |
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.
It would be nice to hide this block if there is only a single statement, so that we don't double display the timing in that case.
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.
Done!
Added support for dashboard notebooks showing server side time elapsed for queries