feat: Query profiling feature #1221

danieljbruce · 2024-01-12T15:01:57Z

This PR implements Query Profiling in Node Datastore.

Changes:

Two dependencies are imported that are used for decoding structs contained in query profiling explain metrics into a flatter format that matches other clients.

A getInfoFromStats function is added that translates the explain metrics from the server into a format that the user will see. A query mode enum is added and offered to the user. Interfaces are added for explain metrics, the plan and the execution stats. runAggregationQuery and runQuery functions are modified to send info back to the user that contains explain metrics. System tests and unit tests are added as well.

Not included:

Note that this PR does not include query profiling samples. Samples must be included in a separate PR because the samples use require('@google-cloud/datastore') which fetches the latest released version of the client library. Therefore, we must release the query profiling feature first before query profiling samples will work in this PR because we are following the convention other samples follow where the latest release is used for samples.

Copy paste over the files from the preview branch

Thread the query mode through the client library. Allow calls to get made using the query mode.

This code allows basic query profiling to work and creates a basic code snippet that can be used to make it work.

This will be useful for debugging grpc issues.

This mock server will be super useful for ensuring that the right data gets transmitted in private preview so that we can confirm the client library is working correctly.

We use the ts file instead.

See if Explain gets passed along to the mock server.

Protos.json is needed to pass the plan along.

Make sure changes don’t break the use of this callback by adding this test.

Define it with a more general data structure.

…into query-profiling-feature # Conflicts: # protos/google/datastore/v1/query_profile.proto # protos/protos.d.ts # protos/protos.js # protos/protos.json # src/v1/datastore_client.ts

The merge caused generated files to have a diff. The diff should not be in the PR.

only should not be used here

Data structures need to be added for the return values that contain stats.

Add stats to info if it is available. If there are no results then end the stream and send the info back. This way, stats will always be sent in info if they are available and the program won ’t break if there are no results.

Explain what happens when the result set is empty. Just send the stats back.

The mock server code was used for debugging and now we don’t need it since the service is working.

Calls to nightly don’t have a second database to work with. Regular calls work now so nightly calls are not necessary.

Bring the query profiling tests back.

This reverts commit 040d0a5.

Stats do not necessarily come from the server.

Each query profiling mode needs to be explored.

Stats returned needs to be parsed by a special library that will remove the complexities of the Struct object.

A library is needed for parsing the Struct values. Add the libraries and use them.

kolea2 · 2024-04-25T15:40:48Z

src/query.ts

+  resultsReturned?: number;
+  executionDuration?: google.protobuf.IDuration;
+  readOperations?: number;
+  debugStats?: JSONValue;


in Java we made this a Map. Is there a reason why we are using this specific type?

The client library doesn't typically return Map objects. It usually returns dictionaries when an object needs to be returned that maps keys to values. For example, an interface like { [key: string]: any };.

The issue here is that the values coming from the server are of type google.protobuf.IStruct and the serializer.toProto3JSON function is used to decode these types returning a JSONValue type which is stored in debugStats. The compiler only guarantees the value will be a JSONValue, but in practice a { [key: string]: any }; type should always be returned.

We can change the return type to { [key: string]: any }; and the compiler won't complain because we are using Object.assign so maybe we should do that. What do you think?

I think that works, would like a NodeJS opinion however :)

kolea2 · 2024-04-25T15:41:02Z

src/query.ts

+}
+export interface ExecutionStats {
+  resultsReturned?: number;
+  executionDuration?: google.protobuf.IDuration;


similar here, could this just be a numeric value? Are there other already modeled fields that are similar? What are their return types?

When a time is provided by the user then just a number representing milliseconds is provided and parsed here to match the proto. However, when commit returns a commitTime then it is passed back to the user as is. go/cfdb-ds-go-query-profiling returns a duration object like this according to the design doc so I think this should stay as is to match Golang and other return values.

sg, would appreciate @sofisl's thoughts on this as well :)

For both types, I think this is OK given that this is exactly what is returned from the service and what other languages are doing. However as I was mentioning to @danieljbruce, I think it's worth showing users how to transform these values in samples, which will be in a separate PR.

sofisl · 2024-04-25T18:35:13Z

package.json

@@ -48,6 +48,8 @@
    "extend": "^3.0.2",
    "google-gax": "^4.0.5",
    "is": "^3.3.0",
+    "proto3-json-serializer": "^2.0.1",


These are pretty big additions. We use protobufjs & proto3-json-serializer in google-gax, is it possible to just use it from there?

Yes. Great suggestion.

danieljbruce added 12 commits October 25, 2023 10:01

Add the right files from the preview branch

3f3e759

Copy paste over the files from the preview branch

Thread Query mode through the library

cdaa193

Thread the query mode through the client library. Allow calls to get made using the query mode.

Create a basic workflow for testing profiling grpc

8f10750

This code allows basic query profiling to work and creates a basic code snippet that can be used to make it work.

This test makes a call to the mock server

1900c9a

This will be useful for debugging grpc issues.

Now the test works with the mock server in repo

031543b

This mock server will be super useful for ensuring that the right data gets transmitted in private preview so that we can confirm the client library is working correctly.

Delete js file

83eec6f

We use the ts file instead.

Try explain

516fdbf

See if Explain gets passed along to the mock server.

Update protos.json

2454f7d

Protos.json is needed to pass the plan along.

Try an aggregate query

698e021

Add test to catch breaking change to return type

79053b8

Make sure changes don’t break the use of this callback by adding this test.

Try redefining RunQueryCallback

e520985

Define it with a more general data structure.

Merge branch 'main' of https://github.com/googleapis/nodejs-datastore …

9e6bafc

…into query-profiling-feature # Conflicts: # protos/google/datastore/v1/query_profile.proto # protos/protos.d.ts # protos/protos.js # protos/protos.json # src/v1/datastore_client.ts

product-auto-label bot added size: xl Pull request size is extra large. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Jan 12, 2024

Revert Gapic changes to match head branch

47c04a4

The merge caused generated files to have a diff. The diff should not be in the PR.

product-auto-label bot added size: m Pull request size is medium. and removed size: xl Pull request size is extra large. labels Jan 12, 2024

danieljbruce added 13 commits January 12, 2024 10:25

Remove only

96e76a7

only should not be used here

Add data structures for return values

1da210e

Data structures need to be added for the return values that contain stats.

Add plumbing to send stats info back

7b94504

Add stats to info if it is available. If there are no results then end the stream and send the info back. This way, stats will always be sent in info if they are available and the program won ’t break if there are no results.

Set test to only that we intend to explore

4e9ae33

Add a comment about stats

5016e87

Explain what happens when the result set is empty. Just send the stats back.

Delete the mock server code

73d2f22

The mock server code was used for debugging and now we don’t need it since the service is working.

Remove calls to nightly

040d0a5

Calls to nightly don’t have a second database to work with. Regular calls work now so nightly calls are not necessary.

Introduce profiling tests again

ccf2f70

Bring the query profiling tests back.

Revert "Remove calls to nightly"

ae79151

This reverts commit 040d0a5.

Stats are optional

b4a236a

Stats do not necessarily come from the server.

Write some tests to test each mode

d134e2b

Each query profiling mode needs to be explored.

Add code for parsing the stats returned

89269e9

Stats returned needs to be parsed by a special library that will remove the complexities of the Struct object.

Add dependencies necessary for parsing

641f197

A library is needed for parsing the Struct values. Add the libraries and use them.

danieljbruce added 3 commits April 18, 2024 14:42

Remove bytesReturned from the interface

6ce178a

Make types in test more specific

167c3ee

name as string

4aaa437

danieljbruce requested a review from bhshkh April 19, 2024 13:46

bhshkh approved these changes Apr 24, 2024

View reviewed changes

kolea2 reviewed Apr 25, 2024

View reviewed changes

danieljbruce requested a review from sofisl April 25, 2024 18:32

sofisl reviewed Apr 25, 2024

View reviewed changes

danieljbruce added 5 commits April 30, 2024 15:15

Rely on 2 dependencies from google-gax instead

5d7df3c

Change expectations in the test to reflect new val

dd14826

Ran linter

841b8a5

Remove extraneous import

f09937f

Modify stats return type

7d10d01

danieljbruce requested review from kolea2 and sofisl May 1, 2024 17:45

sofisl approved these changes May 8, 2024

View reviewed changes

kolea2 approved these changes May 9, 2024

View reviewed changes

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024

Merge branch 'main' into query-profiling-feature

b9be66b

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

danieljbruce added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 9, 2024

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 9, 2024

danieljbruce merged commit 414dec4 into googleapis:main May 9, 2024
15 checks passed

release-please bot mentioned this pull request May 8, 2024

chore(main): release 9.0.0 #1247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Query profiling feature #1221

feat: Query profiling feature #1221

danieljbruce commented Jan 12, 2024 •

edited

Loading

kolea2 Apr 25, 2024

danieljbruce Apr 30, 2024

kolea2 Apr 30, 2024

kolea2 Apr 25, 2024

danieljbruce Apr 30, 2024 •

edited

Loading

kolea2 May 7, 2024

sofisl May 8, 2024

sofisl Apr 25, 2024

danieljbruce Apr 30, 2024

feat: Query profiling feature #1221

feat: Query profiling feature #1221

Conversation

danieljbruce commented Jan 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljbruce Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljbruce commented Jan 12, 2024 •

edited

Loading

danieljbruce Apr 30, 2024 •

edited

Loading