Time all tasks #361
Time all tasks #361
Conversation
This is fairly high touch consisting of 3 things: * Moving to Prometheus's Summary for timers * Timing at .2, .5, .8, .9, .99, and 1.0 (1.0 actually gets max I believe) * Timing all abstract EthTasks * The bulk of the changes: plumbing the timing context everywhere we need it
| public void executeTaskTimed() { | ||
| final OperationTimer.TimingContext timingContext = taskTimer.startTimer(); | ||
| executeTask(); | ||
| taskTimeInSec = timingContext.stopTimer(); |
There was a problem hiding this comment.
We should probably use the try (final OperationTimer.TimingContext timingContext = taskTimer.startTimer()) or an explicit final block here to ensure the timer completes even if the task fails.
There was a problem hiding this comment.
We need the final block because close doesn't return a time and we want to keep that for logging at higher levels.
| count, | ||
| skip, | ||
| reverse, | ||
| labels -> () -> () -> 0.0D); |
There was a problem hiding this comment.
Should we make this a constant somewhere? It's a very cryptic line to have scattered around the code base so much.
There was a problem hiding this comment.
made one in NoOpMetricsSystem
| @@ -17,7 +17,8 @@ | |||
| RPC("rpc"), | |||
| JVM("jvm", false), | |||
| PROCESS("process", false), | |||
| BLOCKCHAIN("blockchain"); | |||
| BLOCKCHAIN("blockchain"), | |||
| TASKS("tasks"); | |||
There was a problem hiding this comment.
TASKS feels too generic as a category - is this more SYNCHRONIZER?
There was a problem hiding this comment.
Synchronizer is similarly opaque to me. How about EthScheduler?
| new Observation(RPC, "request", null, asList("method", "0.99")), | ||
| new Observation(RPC, "request", null, asList("method", "1.0")), | ||
| new Observation(RPC, "request_sum", null, singletonList("method")), | ||
| new Observation(RPC, "request_count", null, singletonList("method"))); |
There was a problem hiding this comment.
For histograms we deliberately converted the _sum and _count metrics to labels instead of completely separate metrics. It comes out much nicer in the JSON-RPC that way. The logic is in convertHistogramSampleNamesToLabels and is probably very similar to what summaries would require.
There was a problem hiding this comment.
Done. I left the histogram branch in place in case we ever come back to it.
* Change from TASKS to ETH_SCHEDULER * Wrap timer read in a finally block * different debug_metrics output for summary that is closer to what we had for histograms * constant for lambda magic - 'labels -> () -> () -> 0'
PR description
This is fairly high touch consisting of 3 things:
A sample of the metrics can be seen here: https://gist.github.com/shemnon/13d1f57858f8b8f8ec0edf8cb4e3d0ab
I'm going to investigate alternatives to dragging the metric all across all those constructors.