-
Notifications
You must be signed in to change notification settings - Fork 90
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 all 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; | ||
| long startTimestamp = 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); | ||
| } | ||
| long endTimestamp = Clock.system().currentTimeNanos(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: if you actually only care about the duration (and not the timestamps), the difference between
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although we are ultimately interested in the duration on the UI, I think the timestamps are sufficient for us #7145 (comment) |
||
|
|
||
| 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(); | ||
|
|
||
| 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.
I'm a little sad when I see this; maybe Colin has more context, but we are still not able to use
google.protobuf.Timestampand/orgoogle.protobuf.Duration?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.
Colin suggested this in a previous comment: #7145 (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.
As per Colin's comment sticking with this
jstypesyntax, further discussion of protobuf types seems like a broader discussion beyond the scope of this ticketThere 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.
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.