feat(firestore): Query profiling #10164
Conversation
firestore/query.go
Outdated
| // RunOption lets the user provide options while running a query | ||
| type RunOption interface { | ||
| apply(*runQuerySettings) error |
There was a problem hiding this comment.
Same comment as above for this option as well.
| @@ -475,6 +644,14 @@ func (q Query) fromProto(pbQuery *pb.RunQueryRequest) (Query, error) { | |||
| q.limit = limit | |||
| } | |||
|
|
|||
| var err error | |||
| q.runQuerySettings, err = newRunQuerySettings(nil) | |||
There was a problem hiding this comment.
Is there a case where this line will produce an error? Is there something in the proto that you wanted to extract here?
There was a problem hiding this comment.
It won't produce error now but I don't want to ignore the error since it may return some error in the future.
Things are getting extracted from proto in the next few lines.
| @@ -1226,14 +1459,17 @@ type QuerySnapshot struct { | |||
|
|
|||
| type btreeDocumentIterator btree.Iterator | |||
|
|
|||
| func (it *btreeDocumentIterator) next() (*DocumentSnapshot, error) { | |||
| func (it *btreeDocumentIterator) next(_ ...RunOption) (*DocumentSnapshot, error) { | |||
There was a problem hiding this comment.
I'm kinda curious, is there a reason why a RunOption would need to be passed in here despite the function not looking like it needs it? Is it to futureproof this call?
There was a problem hiding this comment.
type docIterator interface {
next(opts ...RunOption) (*DocumentSnapshot, error)
getExplainMetrics() *ExplainMetrics
stop()
}
docIterator interface has been modified. RunOption is being passed here to ensure that the btreeDocumentIterator implements the docIterator interface.
Yes, it is not getting used currently but might get used later
firestore/query.go
Outdated
| // This can be used in combination with Deserialize to marshal Query objects. | ||
| // This could be useful, for instance, if executing a query formed in one | ||
| // process in another. | ||
| func (q Query) Serialize() ([]byte, error) { |
There was a problem hiding this comment.
Is this new Serialize function required for query profiling?
There was a problem hiding this comment.
This was earlier at line 342. This is not new function. The diff is not getting rendered correctly.
| // result, err := query.GetAllWithOptions(ExplainOptions{Analyze: true}) | ||
| // | ||
| // It is not necessary to call Stop on the iterator after calling GetAllWithOptions. | ||
| func (it *DocumentIterator) GetAllWithOptions(opts ...RunOption) (*GetAllWithOptionsResult, error) { |
There was a problem hiding this comment.
Is this new function required? It seems a little dangerous to give multiple places to set options like this. I could see that making maintenance/testing difficult in the future
There was a problem hiding this comment.
Please see: go/cfdb-fs-go-query-profiling and go/cfdb-ds-go-query-profiling
| @@ -1083,8 +1304,16 @@ func newQueryDocumentIterator(ctx context.Context, q *Query, tid []byte, rs *rea | |||
| } | |||
| } | |||
|
|
|||
| func (it *queryDocumentIterator) next() (_ *DocumentSnapshot, err error) { | |||
| // opts override the options stored in it.q.runQuerySettings | |||
| func (it *queryDocumentIterator) next(opts ...RunOption) (_ *DocumentSnapshot, err error) { | |||
There was a problem hiding this comment.
Does this mean we support changing options mid-query?
There was a problem hiding this comment.
This is an unexported method. User cannot call this directly
But yes, user can definitely do this:
:
// Analyze true
docIter := client.Collection("cities").
WithRunOptions(firestore.ExplainOptions{Analyze: true}).
Documents(ctx)
// Read single document
docSnapshot, _ := docIter.Next()
// Read rest of the documents with Analyze false
res, _ := docIter.GetAllWithOptions(firestore.ExplainOptions{Analyze: true})
Design: go/cfdb-fs-go-query-profiling and go/cfdb-ds-go-query-profiling
Similar PR for Datastore #9200