-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add timing for query parsing to GraphQLController #214
Add timing for query parsing to GraphQLController #214
Conversation
This will let us see if there are random slowdowns in query parsing. If not, we'll instrument more!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; optional suggestions, most important one is to add operationName
to what's logged.
val before = System.currentTimeMillis() | ||
val res = f | ||
val after = System.currentTimeMillis() | ||
logger.info(s"Parsed query in ${after - before}ms") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message would be much more useful if you passed in and logged operation
here. Maybe some graphQL queries are slower than others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another note about this. This is going to probably cause a lot of logging. Consider logging at debug
level, then we turn it on while triaging? I'm not sure how you guys normally do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MetricsCollector isn't designed to be used in production (we override this with a DatadogMetricsCollector in our repo), so this is mostly just used for debugging in the example
project
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, but I could tag the metric with operation name. good point
QueryParser.parse(query) match { | ||
val parsedQuery = metricsCollector.timeQueryParsing { | ||
QueryParser.parse(query) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a start. Do you think the later assignments to incomingQuery
, filterFn
and the call to filterFn(incomingQuery)
are worth tracking, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this doesn't give us any actionable data, we can add some more tracking in. I'm pretty sure those are all very cheap operations, so I don't want to over-collect data. But it's easy enough to add it in later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
This will let us see if there are random slowdowns in query parsing. If not, we'll instrument more!
PTAL @yifan-coursera or @mkovacs-coursera