Add CPU time to metrics for segment scanning. #1696
Conversation
drcrallen
force-pushed
the
cpuTimeReporting
branch
2 times, most recently
from
058811c to
717e697
Compare
| final Query<T> query, final Map<String, Object> responseContext | ||
| ) | ||
| { | ||
| final Sequence<T> baseSequence = delegate.run(query, responseContext); |
There was a problem hiding this comment.
if report is set to false, we can just return the baseSequence here.
There was a problem hiding this comment.
No, because it still accumulates.
|
👍 |
drcrallen
force-pushed
the
cpuTimeReporting
branch
from
717e697 to
f0dd101
Compare
|
👍 |
|
@drcrallen are we intentionally not making ServerManager like updates for the broker so that broker also report same metric? |
| ) | ||
| { | ||
| if (!THREAD_MX_BEAN.isThreadCpuTimeEnabled()) { | ||
| throw new ISE("Cpu time must enabled"); |
There was a problem hiding this comment.
documentation is not very explicit about when this will happen. Is it possible to not fail here and print some warn/error log saying cpu metrics can't be reported?
There was a problem hiding this comment.
nvmd, just noticed safeBuild below.
There was a problem hiding this comment.
Correct, safeBuild is the intended public constructor
drcrallen
force-pushed
the
cpuTimeReporting
branch
from
f0dd101 to
d06af0b
Compare
|
@himanshug Not in this PR. The appropriate runner could be wrapped at the broker level using the same approach but I purposefully left it out of this PR to limit the scope. |
drcrallen
force-pushed
the
cpuTimeReporting
branch
from
d06af0b to
fcf5cae
Compare
|
LGTM |
Add CPU time to metrics for segment scanning.
| toolChest | ||
| return CPUTimeMetricQueryRunner.safeBuild( | ||
| new FinalizeResultsQueryRunner<T>( | ||
| toolChest.mergeResults(factory.mergeRunners(exec, queryRunners)), |
There was a problem hiding this comment.
@drcrallen implementation of most QueryRunnerFactory.mergeRunners() methods delegates to ChainedExecutionQueryRunner, that delegates actual processing to a provided executor. Hence it seems to me that CPU Time wrapping should happen before mergeRunners(). What do you think?
There was a problem hiding this comment.
Ok I see, there is a distinct cpuTimeAccumulator variable used above to accumulate those metrics.
This is an alternative to #1298
This PR only reports CPU usage during the compute heavy portions of the query runner by wrapping the yielder calls (or at least is supposed to) and only reports metrics once per query including having a query ID.
The prior PR (#1298) reported cpu usage as part of the metrics emitting executor service by wrapping the runnables and callables, and didn't have a good way to get context for why the CPU was being used.
Overall I think this PR is a better way to go because it has better reporting over the context of the CPU usage.