-
Notifications
You must be signed in to change notification settings - Fork 91
feat: DH-19382: add server side timing to JsCommandResult #7145
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
Changes from 2 commits
f4d391d
6aae92b
c5ac277
ccc9de8
e68dcc7
5ecd055
65ffce7
4e9705e
63c9150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |||||||||||||||||||
| import com.google.common.base.Throwables; | ||||||||||||||||||||
| import com.google.rpc.Code; | ||||||||||||||||||||
| import io.deephaven.base.LockFreeArrayQueue; | ||||||||||||||||||||
| import io.deephaven.base.clock.Clock; | ||||||||||||||||||||
| import io.deephaven.configuration.Configuration; | ||||||||||||||||||||
| import io.deephaven.engine.context.ExecutionContext; | ||||||||||||||||||||
| import io.deephaven.engine.context.QueryScope; | ||||||||||||||||||||
|
|
@@ -197,6 +198,7 @@ public void executeCommand( | |||||||||||||||||||
|
|
||||||||||||||||||||
| // If not set, we'll use defaults, otherwise we will explicitly set the systemicness. | ||||||||||||||||||||
| final ScriptSession.Changes changes; | ||||||||||||||||||||
| String startTimestamp = Long.toString(Clock.system().currentTimeNanos()); | ||||||||||||||||||||
| switch (systemicOption) { | ||||||||||||||||||||
| case NOT_SET_SYSTEMIC: | ||||||||||||||||||||
| changes = scriptSession.evaluateScript(request.getCode()); | ||||||||||||||||||||
|
|
@@ -213,6 +215,7 @@ public void executeCommand( | |||||||||||||||||||
| throw new UnsupportedOperationException( | ||||||||||||||||||||
| "Unrecognized systemic option: " + systemicOption); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| String endTimestamp = Long.toString(Clock.system().currentTimeNanos()); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| final ExecuteCommandResponse.Builder diff = ExecuteCommandResponse.newBuilder(); | ||||||||||||||||||||
| final FieldsChangeUpdate.Builder fieldChanges = FieldsChangeUpdate.newBuilder(); | ||||||||||||||||||||
|
|
@@ -226,7 +229,11 @@ public void executeCommand( | |||||||||||||||||||
| diff.setErrorMessage(Throwables.getStackTraceAsString(changes.error)); | ||||||||||||||||||||
| log.error().append("Error running script: ").append(changes.error).endl(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return diff.setChanges(fieldChanges).build(); | ||||||||||||||||||||
| diff.setChanges(fieldChanges); | ||||||||||||||||||||
| diff.setStartTimestamp(startTimestamp); | ||||||||||||||||||||
| diff.setEndTimestamp(endTimestamp); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| return diff.build(); | ||||||||||||||||||||
|
||||||||||||||||||||
| diff.setChanges(fieldChanges); | |
| diff.setStartTimestamp(startTimestamp); | |
| diff.setEndTimestamp(endTimestamp); | |
| return diff.build(); | |
| return diff.setChanges(fieldChanges) | |
| .setStartTimestamp(startTimestamp) | |
| .setEndTimestamp(endTimestamp) | |
| .build(); |
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.
Modified back to builder style
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
|
|
||
| import com.vertispan.tsdefs.annotations.TsInterface; | ||
| import com.vertispan.tsdefs.annotations.TsName; | ||
| import io.deephaven.web.client.api.LongWrapper; | ||
| import jsinterop.annotations.JsProperty; | ||
|
|
||
| /** | ||
|
|
@@ -15,10 +16,14 @@ | |
| public class JsCommandResult { | ||
| private final JsVariableChanges changes; | ||
| private final String error; | ||
| private final String startTimestamp; | ||
| private final String endTimestamp; | ||
|
|
||
| public JsCommandResult(JsVariableChanges changes, String error) { | ||
| public JsCommandResult(JsVariableChanges changes, String error, String startTimestamp, String endTimestamp) { | ||
| this.changes = changes; | ||
| this.error = error; | ||
| this.startTimestamp = startTimestamp; | ||
| this.endTimestamp = endTimestamp; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -41,6 +46,26 @@ public String getError() { | |
| return error; | ||
| } | ||
|
|
||
| /** | ||
| * The timestamp when the command started running. | ||
| * | ||
| * @return long | ||
| */ | ||
| @JsProperty | ||
| public LongWrapper getStartTimestamp() { | ||
|
||
| return LongWrapper.of(Long.parseLong(startTimestamp)); | ||
| } | ||
|
|
||
| /** | ||
| * The timestamp when the command finished running. | ||
| * | ||
| * @return long | ||
| */ | ||
| @JsProperty | ||
| public LongWrapper getEndTimestamp() { | ||
| return LongWrapper.of(Long.parseLong(endTimestamp)); | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| if (error != null && !error.isEmpty()) { | ||
|
|
||
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 would prefer to use actual nanos since the epoch than something that needs parsing/interpretation.
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.
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.
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.
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:
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
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.
Modified types to the
jstypeand added a comment