Add segment merge time as a metric #1770
Conversation
| @@ -538,6 +538,9 @@ public void doRun() | |||
| cleanShutdown = false; | |||
| abandonSegment(truncatedTime, sink); | |||
| } | |||
| } finally { | |||
| metrics.incrementPersistTimeMillis(mergeStopwatch.elapsed(TimeUnit.MILLISECONDS)); | |||
There was a problem hiding this comment.
this also includes segment publish time, it might be worthwhile to only count actual merge time if handoff can take a while?
There was a problem hiding this comment.
moved it to exclude segment publish time.
|
How viable is it to add CPU time in addition to wall-time? |
nishantmonu51
force-pushed
the
merge-time
branch
from
abba6c2
to
790f928
Compare
|
@drcrallen added cpuTime for both persist and merge operations. please have a look again. |
nishantmonu51
force-pushed
the
merge-time
branch
from
790f928
to
d426562
Compare
| emitter.emit(builder.build("ingest/merge/time", metrics.mergeTimeMillis() - previous.mergeTimeMillis())); | ||
| emitter.emit(builder.build("ingest/merge/cpu", metrics.mergeCpuTime() - previous.mergeCpuTime())); | ||
| emitter.emit(builder.build("ingest/persist/cpu", metrics.persistCpuTime() - previous.persistCpuTime())); | ||
|
|
There was a problem hiding this comment.
minor nit, unnecessary empty lines
There was a problem hiding this comment.
removed empty lines.
nishantmonu51
force-pushed
the
merge-time
branch
2 times, most recently
from
e2f8717
to
795d09f
Compare
|
|
||
| emitter.emit(builder.build("ingest/merge/time", metrics.mergeTimeMillis() - previous.mergeTimeMillis())); | ||
| emitter.emit(builder.build("ingest/merge/cpu", metrics.mergeCpuTime() - previous.mergeCpuTime())); | ||
| emitter.emit(builder.build("ingest/persist/cpu", metrics.persistCpuTime() - previous.persistCpuTime())); |
There was a problem hiding this comment.
this needs documentation update.
There was a problem hiding this comment.
also, it appears we will be publishing these metrics whether or not cpu time is supported , so should document that if user sees a value of 0 then that means its just not supported on the jvm
Add merge and persist cpu time Fix typo review comment move cpu time measuring to VMUtils review comments.
nishantmonu51
force-pushed
the
merge-time
branch
from
795d09f
to
7cecc55
Compare
|
LGTM |
|
👍 @xvrl are your comments addressed? |
Add segment merge time as a metric
Backport #1770 - Add segment merge time as a metric
Adds these metrics -