-
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,14 +39,27 @@ class MetricsCollectionMiddleware( | |
|
||
trait GraphQLMetricsCollector { | ||
def markFieldError(fieldName: String): Unit | ||
def timeQueryParsing[A](f: => A): A | ||
} | ||
|
||
class LoggingMetricsCollector extends GraphQLMetricsCollector with StrictLogging { | ||
def markFieldError(fieldName: String): Unit = { | ||
logger.info(s"Error when loading field $fieldName") | ||
} | ||
|
||
def timeQueryParsing[A](f: => A): A = { | ||
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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, but I could tag the metric with operation name. good point |
||
res | ||
} | ||
} | ||
|
||
class NoopMetricsCollector extends GraphQLMetricsCollector with StrictLogging { | ||
def markFieldError(fieldName: String): Unit = () | ||
|
||
def timeQueryParsing[A](f: => A): A = { | ||
f | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
version in ThisBuild := "0.8.4" | ||
version in ThisBuild := "0.8.5" |
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 tofilterFn(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