Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(datastore): Add query profiling #9200

Merged
merged 23 commits into from May 7, 2024
Merged

feat(datastore): Add query profiling #9200

merged 23 commits into from May 7, 2024

Conversation

bhshkh
Copy link
Contributor

@bhshkh bhshkh commented Dec 27, 2023 •
edited

Introducing ExplainOptions:

  • When ExplainOptions is not used: Datastore plans and executes the query. Only returns the query results.
  • When ExplainOptions.Analyze is false: Datastore plans but does not execute the query. Only returns planner stage information. Can be used to check that a query has the necessary indexes and understand which indexes are used.
  • When ExplainOptions.Analyze is true: Datastore plans and executes the query. Returns query results, planner stage information and execution statistics.

samples at GoogleCloudPlatform/golang-samples#3742

@bhshkh bhshkh requested review from a team as code owners December 27, 2023 04:08
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Dec 27, 2023
@bhshkh bhshkh changed the title Add query profiling feat(datastore): Add query profiling Dec 27, 2023
@product-auto-label product-auto-label bot added the api: datastore Issues related to the Datastore API. label Dec 27, 2023
kolea2
kolea2 reviewed Jan 4, 2024
View reviewed changes
datastore/query.go Outdated Show resolved Hide resolved
danieljbruce
danieljbruce reviewed Jan 11, 2024
View reviewed changes
Copy link

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I think @kolea2 mentioned that samples will be required as well so samples could be added in this PR or in a separate PR.

datastore/integration_test.go Outdated Show resolved Hide resolved
@bhshkh
Copy link
Contributor Author

bhshkh commented Jan 17, 2024

Samples at GoogleCloudPlatform/golang-samples#3742

@bhshkh bhshkh mentioned this pull request Jan 17, 2024
Merged
9 tasks
danieljbruce
danieljbruce approved these changes Jan 17, 2024
View reviewed changes
datastore/query.go Outdated Show resolved Hide resolved
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Jan 26, 2024
@bhshkh bhshkh requested a review from a team as a code owner February 13, 2024 20:31
danieljbruce
danieljbruce reviewed Feb 14, 2024
View reviewed changes
Copy link

@danieljbruce danieljbruce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I can do another review once the owl-bot PR comes in so that all the CI checks pass.

@product-auto-label product-auto-label bot removed the stale: old Pull request is old and needs attention. label Feb 25, 2024
@product-auto-label product-auto-label bot added the stale: extraold Pull request is critically old and needs prioritization. label Feb 25, 2024
kolea2
kolea2 reviewed Mar 20, 2024
View reviewed changes
datastore/query.go Outdated Show resolved Hide resolved
@bhshkh bhshkh requested a review from kolea2 March 27, 2024 04:02
@bhshkh bhshkh added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2024
@bhshkh
Copy link
Contributor Author

bhshkh commented Mar 29, 2024

Do not merge until release freeze ends in mid April

@bhshkh bhshkh removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 17, 2024
gkevinzheng
gkevinzheng reviewed Apr 22, 2024
View reviewed changes
datastore/query.go
Comment on lines +641 to +647
type ExplainOptions struct {
// When false (the default), the query will be planned, returning only
// metrics from the planning stages.
// When true, the query will be planned and executed, returning the full
// query results along with both planning and execution stage metrics.
Analyze bool
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this not structured as a private struct explainOptions with a function ExplainOptions that returns an instance of explainOptions? With this structure the user would have to pass in stuff like ExplainOptions{Analyze: true} instead of ExplainOptions(true), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, ExplainOptions contains only Analyze field. In the future, if more fields are added to ExplainOptions struct, user will no longer be able to use ExplainOptions(true).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see, I was confused because this is a proto field, not an option in the Golang sense of the word.

datastore/query.go Outdated Show resolved Hide resolved
@bhshkh bhshkh enabled auto-merge (squash) May 7, 2024 12:25
@bhshkh bhshkh merged commit b0235bb into googleapis:main May 7, 2024
8 checks passed
@release-please release-please bot mentioned this pull request May 7, 2024
Merged
@bhshkh bhshkh deleted the feature/ds-query-profiling branch May 7, 2024 17:04
@bhshkh bhshkh mentioned this pull request May 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieljbruce danieljbruce danieljbruce approved these changes

@gkevinzheng gkevinzheng gkevinzheng approved these changes

@kolea2 kolea2 Awaiting requested review from kolea2

Assignees
No one assigned
Labels
api: datastore Issues related to the Datastore API. size: l Pull request size is large. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bhshkh @danieljbruce @kolea2 @gkevinzheng