cli: show client-server latency on startup of interactive sql connection #50250
Conversation
Previously, we did not show client-server latency for interactive sql connections. Now, a line is printed at the top when an interactive sql session begins which prints the client-server latency. Latency is measured by the time it takes the "get cluster version" query to return. Addresses cockroachdb#49450 Release note (cli change): cli now prints the client-server latency on startup.
|
This change is |
There was a problem hiding this comment.
Looked into this into the context of the original issue.
I think in the commit message / PR description you can't yet write "Addresses" but instead "Informs"
(Also, nit: github has magic words to auto-close a linked issue; the words include "Fixes" or "Closes" but not "Addresses". You recognize a magic words when it gets underlined on the web display.)
Now at a higher level I think you're not being ambitious enough in this PR. The original issue makes several points which I think you can achieve here:
-
display the time to receive the 1st row of result separately from the time to receive the last row, and possibly separately to the time to finish the display. The client has all 3 timestamps and we could enhance our time display to inform the user.
-
regarding server-side execution latency. This information could be stored server-side, and retrieved by a statement. Look into the implementation of SHOW SYNTAX and SHOW TRANSACTION STATUS (the "observer statements"). You could add another one which observes the execution time of the last executed statement before it. Then you could modify the shell to issue that new observer statement immediately after a statement execution completes. Then we'd get exactly what Alex is requesting, and you wouldn't even need to report the client-server roundtrip latency at the beginning of the session.
| @@ -278,6 +281,7 @@ func (c *sqlConn) checkServerMetadata() error { | |||
| } | |||
| } | |||
| } | |||
| fmt.Printf("# Client-server latency: %s\n", latency) | |||
There was a problem hiding this comment.
Maybe add "roundtrip" here which is what you're measuring.
|
Seconding @knz here. I think displaying the client-server latency is of limited value compared to exposing the actual server-side latency, from parsing the query to returning the first row. If this is possible via "observer" statements that would be great. |
|
Thanks for the suggestions @knz. I especially like the second option, and I think it seems doable. Side note: My intention with this PR was to knock out the easy/quick patch and then iterate from there to tackle individual queries. That's why I wrote addresses in my PR instead of fix, though I agree writing informs would have made my intention clearer. |
|
I'm closing this for now; once the observer statement goes in, I'll change my approach and re-open a PR. |
Previously, we did not show client-server latency for interactive sql
connections. Now, a line is printed at the top when an interactive sql
session begins which prints the client-server latency. Latency is
measured by the time it takes the "get cluster version" query to return.
Addresses #49450
Release note (cli change): cli now prints the client-server latency on
startup.