graphql: metrics for validation and parsing phases #3760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kamilkisiela
force-pushed
the
kamil-graphql-metrics
branch
from
92d5c46 to
841e8e0
Compare
|
Seems there are build issues. |
|
I'm trying to add This way it's all in one place. |
kamilkisiela
force-pushed
the
kamil-graphql-metrics
branch
from
841e8e0 to
b2cd397
Compare
kamilkisiela
changed the title
|
Tests are not passing. |
leoyvens
requested changes
Jul 21, 2022
graph/src/data/query/result.rs
Outdated
| @@ -45,12 +46,14 @@ pub type Data = Object; | |||
| /// A collection of query results that is serialized as a single result. | |||
| pub struct QueryResults { | |||
| results: Vec<Arc<QueryResult>>, | |||
| pub validation_time: Option<Duration>, | |||
There was a problem hiding this comment.
Not great that this field had to be added. Could perhaps the GraphQlRunner have access to the GraphQLServiceMetrics and register the metric immediately after taking the measurement?
There was a problem hiding this comment.
I created GraphQLMetrics that is passed to GraphQlRunner and is responsible for all metrics related to GraphQL:
query_execution_timequery_parsing_timequery_validation_timequery_result_sizequery_result_max
kamilkisiela
force-pushed
the
kamil-graphql-metrics
branch
3 times, most recently
from
81dc87a to
e525140
Compare
leoyvens
requested changes
Jul 22, 2022
graph/src/components/graphql.rs
Outdated
Comment on lines
50
to
53
| pub trait GraphQLMetrics: Send + Sync + 'static { | ||
| fn observe_query_execution(&self, duration: Duration, results: &QueryResults) -> (); | ||
| fn observe_query_parsing(&self, duration: Duration, results: &QueryResults) -> (); | ||
| fn observe_query_validation(&self, duration: Duration, id: &DeploymentHash) -> (); |
There was a problem hiding this comment.
The -> () can be omitted.
Suggested change
| pub trait GraphQLMetrics: Send + Sync + 'static { | |
| fn observe_query_execution(&self, duration: Duration, results: &QueryResults) -> (); | |
| fn observe_query_parsing(&self, duration: Duration, results: &QueryResults) -> (); | |
| fn observe_query_validation(&self, duration: Duration, id: &DeploymentHash) -> (); | |
| pub trait GraphQLMetrics: Send + Sync + 'static { | |
| fn observe_query_execution(&self, duration: Duration, results: &QueryResults); | |
| fn observe_query_parsing(&self, duration: Duration, results: &QueryResults); | |
| fn observe_query_validation(&self, duration: Duration, id: &DeploymentHash); |
server/index-node/src/service.rs
Outdated
| @@ -120,7 +120,7 @@ where | |||
| let validated = ValidatedRequest::new(body, &req_parts.headers)?; | |||
| let query = validated.query; | |||
|
|
|||
| let query = match PreparedQuery::new(&self.logger, schema, None, query, None, 100) { | |||
| let query = match PreparedQuery::new(&self.logger, schema, None, query, None, 100, None) { | |||
There was a problem hiding this comment.
I'd prefer to use a fake metrics collector like there is for tests, than have this be an option.
|
@kamilkisiela this just needs to be rebased. |
kamilkisiela
force-pushed
the
kamil-graphql-metrics
branch
from
cf91af7 to
a90028a
Compare
|
@leoyvens rebased |
leoyvens
approved these changes
Jul 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New Prometheus metrics
query_parsing_timequery_validation_timeCloses #3752