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.

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

Users/davetheunissen/query metrics option #38

Closed
wants to merge 3 commits into from
Closed

Users/davetheunissen/query metrics option #38

wants to merge 3 commits into from

Conversation

davetheunissen
Copy link

@davetheunissen davetheunissen commented Feb 1, 2019

This PR relates to issues #21 and #33

  • Added the PopulateQueryMetrics property to the CosmosQueryRequestOptions class and maped to underlying FeedOptions.

  • Updated CosmosQueryResponse Resources to be public so that the meta for the response is available to the calling client

  • Added a unit test to ensure that QueryMetrics are populated when the PopulateQueryMetrics feed option is set.

/// <summary>
/// Contains the metadata from the response headers from the Azure Cosmos DB call
/// </summary>
public IEnumerable<T> Resources;
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 being made public? The CosmosQueryResponse already implements IEnumerable.

Copy link
Author

@davetheunissen davetheunissen Feb 5, 2019

Choose a reason for hiding this comment

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

As far as I can tell, there is no way to cast the Resources object to CosmosQueryResponse<T> because internally it is of Type FeedResponse<T>.

The only alternate I can see is to create a public property in CosmosQueryResponse that returns FeedOptions<T> from Resources. Not sure that this is great option though?

If you have an example of how this might work, I'm happy to learn and ammend the PR etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kirankumarkolli any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't the CosmosResultSetIterator work to iterate with any type, like it works for CreateItemQuery?

Copy link
Author

@davetheunissen davetheunissen Feb 11, 2019

Choose a reason for hiding this comment

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

If I create a CosmosResultSetIterator<FeedReponse<T>> FetchNextSetAsync throws an exception because the document coming back from CosmosDB doesn't match that type.

CosmosResultSetIterator<T> returns an enumerable collection of CosmosQueryResponse. CosmosQueryResponse<T> returns an enumerator of Resources<FeedResponse<T>>. The FeedResponse includes the meta properties about the request so we don't need to enumerate Resources, we just need access to the FeedResponse<T> properties.

image

Given that FeedResponse<T> is an internal class, there isn't a way to create a public accessor property without changing the internal scope.

If there is an issue with making Resources public then I'll need to try something else to get at the FeedResponse properties.

Appreciate any help or thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a (new) public IQueryMetrics interface that IFeedResponse<T> would inherit from, and that's the interface we would use to expose query metrics from CosmosQueryResponse<T> through the Resources property? (although I don't think "Resources" would be a good name for that purpose)

Copy link
Author

@davetheunissen davetheunissen Feb 19, 2019

Choose a reason for hiding this comment

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

The IFeedResponse<T> interface already contains all the properties that we'd like to expose... incl. query metrics... however that's also scoped as internal. I'll have another crack at this on the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants