feat: DH-19382: add server side timing to JsCommandResult #7145

jnumainville · 2025-09-02T20:43:16Z

Server side PR for DH-19382
Adds server side timing to JsCommandResult for query execution time
Tested in deephaven/web-client-ui#2526

cpwright · 2025-09-02T20:45:18Z

proto/proto-backplane-grpc/src/main/proto/deephaven_core/proto/console.proto

 message ExecuteCommandResponse {
    string error_message = 1;
    io.deephaven.proto.backplane.grpc.FieldsChangeUpdate changes = 2;
+    string start_timestamp = 3;


I would prefer to use actual nanos since the epoch than something that needs parsing/interpretation.

Note, I realize that JS sucks at numbers; but the proto is a generic thing. Anytime we add a field, we should also add a comment for the protodocs.

We already have wiring to handle int64 in a format we can get it to js - the following change will make it still be a string in the browser, but we can expose as a DateWrapper so that JS can truncate to number, format to string, or ask for it as a js Date:

Suggested change

string start_timestamp = 3;

int64 start_timestamp = 3 [jstype=JS_STRING];

In theory proto also has a well known date types, but i'd vote just stick to nanos like we do for logs etc
https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/timestamp.proto

Modified types to the jstype and added a comment

...n/javascript/proto/dhinternal/io/deephaven_core/proto/console_pb/ExecuteCommandResponse.java

niloc132 · 2025-09-03T18:48:39Z

server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java

+                        diff.setChanges(fieldChanges);
+                        diff.setStartTimestamp(startTimestamp);
+                        diff.setEndTimestamp(endTimestamp);
+
+                        return diff.build();


nothing at all wrong with this style, but can also be a builder style as before:

Suggested change

diff.setChanges(fieldChanges);

diff.setStartTimestamp(startTimestamp);

diff.setEndTimestamp(endTimestamp);

return diff.build();

return diff.setChanges(fieldChanges)

.setStartTimestamp(startTimestamp)

.setEndTimestamp(endTimestamp)

.build();

Modified back to builder style

niloc132 · 2025-09-03T18:49:30Z

web/client-api/src/main/java/io/deephaven/web/client/api/console/JsCommandResult.java

+     * @return long
+     */
+    @JsProperty
+    public LongWrapper getStartTimestamp() {


Probably DateWrapper instead?

Changed to DateWrapper

…_api

github-actions · 2025-10-06T16:25:57Z

No docs changes detected for `63c9150`

devinrsmith · 2025-10-06T17:42:21Z

proto/proto-backplane-grpc/src/main/proto/deephaven_core/proto/console.proto

+    // Time it took to run the command, in nanoseconds
+    int64 start_timestamp = 3 [jstype=JS_STRING];
+    int64 end_timestamp = 4 [jstype=JS_STRING];


This comment is incorrect; if you were providing a single duration field, the comment would make sense.

Improved comment

devinrsmith · 2025-10-06T17:47:21Z

server/src/main/java/io/deephaven/server/console/ConsoleServiceGrpcImpl.java

                                throw new UnsupportedOperationException(
                                        "Unrecognized systemic option: " + systemicOption);
                        }
+                        long endTimestamp = Clock.system().currentTimeNanos();


Note: if you actually only care about the duration (and not the timestamps), the difference between java.lang.System.nanoTime() would be more appropriate than the difference between Clock.system().currentTimeNanos().

Although we are ultimately interested in the duration on the UI, I think the timestamps are sufficient for us #7145 (comment)

devinrsmith · 2025-10-06T17:50:01Z

proto/proto-backplane-grpc/src/main/proto/deephaven_core/proto/console.proto

    io.deephaven.proto.backplane.grpc.FieldsChangeUpdate changes = 2;
+
+    // Time it took to run the command, in nanoseconds
+    int64 start_timestamp = 3 [jstype=JS_STRING];


I'm a little sad when I see this; maybe Colin has more context, but we are still not able to use google.protobuf.Timestamp and/or google.protobuf.Duration?

Colin suggested this in a previous comment: #7145 (comment)

As per Colin's comment sticking with this jstype syntax, further discussion of protobuf types seems like a broader discussion beyond the scope of this ticket

the jstype option and using one of the built in types are sort of unrelated - but yes, we can't (yet) use other semi "built-in" types.

devinrsmith · 2025-10-09T21:04:44Z

proto/proto-backplane-grpc/src/main/proto/deephaven_core/proto/console.proto

    string error_message = 1;
    io.deephaven.proto.backplane.grpc.FieldsChangeUpdate changes = 2;
+
+    // Start and end timestamps for command execution, in nanoseconds since the unix epoch.


This comment is documentation for the start_timestamp field, not a general code comment. As such, probably makes sense to document each field as appropriate (I don't actually know if there is a way to add a "general" comment that doesn't effect resulting protobuf documentation).

Did not realize that, thanks, made it two comments

…_api

jnumainville added 2 commits September 2, 2025 15:34

working commit

f4d391d

remove log

6aae92b

jnumainville requested a review from mofojed September 2, 2025 20:43

cpwright reviewed Sep 2, 2025

View reviewed changes

mofojed reviewed Sep 3, 2025

View reviewed changes

...n/javascript/proto/dhinternal/io/deephaven_core/proto/console_pb/ExecuteCommandResponse.java Show resolved Hide resolved

niloc132 reviewed Sep 3, 2025

View reviewed changes

mofojed mentioned this pull request Sep 10, 2025

feat: DH-19382: Add actions and timing info on console history hover deephaven/web-client-ui#2526

Merged

jnumainville added 2 commits October 6, 2025 11:23

address comments

c5ac277

Merge remote-tracking branch 'origin/main' into 19382_server_duration…

ccc9de8

…_api

jnumainville added the NoDocumentationNeeded label Oct 6, 2025

fixing go protobuf

e68dcc7

jnumainville added the NoReleaseNotesNeeded No release notes are needed. label Oct 6, 2025

fixing py protobuf

5ecd055

jnumainville marked this pull request as ready for review October 6, 2025 17:32

jnumainville requested review from chipkent, devinrsmith, jmao-denver, nbauernfeind and rcaudy as code owners October 6, 2025 17:32

jnumainville requested review from cpwright, mofojed and niloc132 October 6, 2025 17:32

devinrsmith reviewed Oct 6, 2025

View reviewed changes

fixing comment

65ffce7

jnumainville requested a review from devinrsmith October 7, 2025 20:56

devinrsmith reviewed Oct 9, 2025

View reviewed changes

fixed comments

4e9705e

jnumainville requested a review from devinrsmith October 9, 2025 21:12

jnumainville enabled auto-merge (squash) October 9, 2025 21:12

devinrsmith approved these changes Oct 9, 2025

View reviewed changes

Merge remote-tracking branch 'origin/main' into 19382_server_duration…

63c9150

…_api

mofojed approved these changes Oct 15, 2025

View reviewed changes

niloc132 approved these changes Oct 15, 2025

View reviewed changes

cpwright approved these changes Oct 17, 2025

View reviewed changes

jnumainville merged commit 45fe6f3 into deephaven:main Oct 17, 2025
24 checks passed

github-actions bot locked and limited conversation to collaborators Oct 17, 2025

	string start_timestamp = 3;
	int64 start_timestamp = 3 [jstype=JS_STRING];

feat: DH-19382: add server side timing to JsCommandResult #7145

feat: DH-19382: add server side timing to JsCommandResult #7145

Uh oh!

Conversation

jnumainville commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No docs changes detected for 63c9150

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Oct 6, 2025 •

edited

Loading

No docs changes detected for `63c9150`