Better harmonized dimensions for query metrics. #3245
Conversation
All query metrics now start with toolChest.makeMetricBuilder, and all of *those* now start with DruidMetrics.makePartialQueryTimeMetric. Also, "id" moved to common code, since all query metrics added it anyway. In particular this will add query-type specific dimensions like "threshold" and "numDimensions" to servlet-originated metrics like query/time.
|
👍 |
| @@ -236,6 +243,7 @@ public Object accumulate(Object accumulated, Object in) | |||
|
|
|||
| try { | |||
| final Query theQuery = query; | |||
| final QueryToolChest theToolChest = toolChest; | |||
There was a problem hiding this comment.
If I'm reading this correct, the only reason you need to have a 2nd variable name here is because the two code paths of A) query successfully deserializing and B) query throws exception while deserializing... are all muddled together in the exception handling.
Would it be worth it in this PR to try and clean that up a bit?
There was a problem hiding this comment.
This is here because the query and toolChest are passed to anonymous classes at some point, and so the references need to be final. But the original references aren't final due to how the exception handling is done.
I didn't really want to mess with any of that in this PR.
There was a problem hiding this comment.
Sure, I figured it would probably be outside the scope of this PR, but should be refactored before too many work-arounds are added.
|
Failed due to #2253 - https://travis-ci.org/druid-io/druid/jobs/144765914 - bouncing. |
|
👍 after travis |
All query metrics now start with toolChest.makeMetricBuilder, and all of
those now start with DruidMetrics.makePartialQueryTimeMetric. Also, "id"
moved to common code, since all query metrics added it anyway.
In particular this will add query-type specific dimensions like "threshold"
and "numDimensions" to servlet-originated metrics like query/time.