Added merge/buffer metric to print buffer pool size every minute #5541

Sarvesh1990 · 2018-03-27T15:00:00Z

No description provided.

gianm

Thanks for the contribution @Sarvesh1990!

In addition to the comments on the diff, I've got one more comment: please add a line to the metrics.md documentation page explaining the new metric and what it means.

gianm · 2018-03-27T16:32:25Z

processing/src/main/java/io/druid/query/MetricsEmittingExecutorService.java

@@ -66,6 +71,7 @@ public void emitMetrics(ServiceEmitter emitter, ServiceMetricEvent.Builder metri
  {
    if (delegate instanceof PrioritizedExecutorService) {
      emitter.emit(metricBuilder.build("segment/scan/pending", ((PrioritizedExecutorService) delegate).getQueueSize()));
+      emitter.emit(metricBuilder.build("merge/buffers/size", ((DefaultBlockingPool) mergeBufferPool).getPoolSize()));


Suggest calling this query/merge/buffersUsed. Presumably, the other metric (merge buffer wait time) would be something like query/merge/wait.

gianm · 2018-03-27T16:33:38Z

processing/src/main/java/io/druid/query/MetricsEmittingExecutorService.java

@@ -66,6 +71,7 @@ public void emitMetrics(ServiceEmitter emitter, ServiceMetricEvent.Builder metri
  {
    if (delegate instanceof PrioritizedExecutorService) {
      emitter.emit(metricBuilder.build("segment/scan/pending", ((PrioritizedExecutorService) delegate).getQueueSize()));
+      emitter.emit(metricBuilder.build("merge/buffers/size", ((DefaultBlockingPool) mergeBufferPool).getPoolSize()));


getPoolSize() is not the right method to use here. It's the number of buffers in the pool that have not been handed out, but we want this metric to be the number that are handed out. Try adding a new method to the BlockingPool interface like getUsedBufferCount() and then implementing that in DefaultBlockingPool.

Done.

Regarding metrics.md, I am not sure what to write in the Dimensions part. Also Normal value should ideally be the number of buffers allocated but that value will vary depending on cores. So did not write anything in that.

Let me know if this works fine.

jihoonson

@Sarvesh1990 thanks for your PR! It looks good to me and I left some trivial comments.

jihoonson · 2018-03-28T21:06:55Z

common/src/main/java/io/druid/collections/BlockingPool.java

@@ -25,6 +25,8 @@
 {
  int maxSize();

+  int getUsedBufferCount();


Please add a simple javadoc.

jihoonson · 2018-03-28T21:09:11Z

server/src/main/java/io/druid/guice/DruidProcessingModule.java

@@ -97,7 +97,7 @@ public ExecutorService getProcessingExecutorService(
        PrioritizedExecutorService.create(
            lifecycle,
            config
-        ),
+        ), getMergeBufferPool(config),


This should be like

PrioritizedExecutorService.create( lifecycle, config ), getMergeBufferPool(config), executorServiceMonitor

jihoonson · 2018-03-28T21:12:15Z

processing/src/main/java/io/druid/query/MetricsEmittingExecutorService.java

@@ -66,6 +71,7 @@ public void emitMetrics(ServiceEmitter emitter, ServiceMetricEvent.Builder metri
  {
    if (delegate instanceof PrioritizedExecutorService) {
      emitter.emit(metricBuilder.build("segment/scan/pending", ((PrioritizedExecutorService) delegate).getQueueSize()));
+      emitter.emit(metricBuilder.build("query/merge/buffersUsed", ((DefaultBlockingPool) mergeBufferPool).getUsedBufferCount()));


Looks that the casting is not needed.

Done. Let me know if its fine.

Sarvesh1990 · 2018-04-02T07:12:09Z

Hey @gianm @jihoonson - Does the fix looks okay to merge?

gianm

@Sarvesh1990 - just read over this again. There is an issue with the implementation in getProcessingExecutorService, but also, I suggest we side-step that anyway. See the comment for more details.

gianm · 2018-04-02T16:59:21Z

server/src/main/java/io/druid/guice/DruidProcessingModule.java

@@ -98,6 +98,7 @@ public ExecutorService getProcessingExecutorService(
            lifecycle,
            config
        ),
+        getMergeBufferPool(config),


This shouldn't be getMergeBufferPool(config) - it's going to create a new merge buffer pool rather than use the existing one. (Check the implementation of getMergeBufferPool.) So the metric would always be zero since it wouldn't be checking the "real" pool. Instead, the patch should have a @Merging BlockingPool<ByteBuffer> mergePool as an argument to getProcessingExecutorService. Then the right one would get injected.

However! I think we should go a different path entirely. The Processing ExecutorService is not really the right place to add this metric. The metric is unrelated to executor processing. I think it'd be better to have the Merging BlockingPool do this itself, in much the same way that the Processing ExecutorService does. By that I mean: define a MetricsEmittingBlockingPool and have that emit the buffersUsed metric. Then, that should be returned by the existing getMergeBufferPool method.

Hey @gianm.

Thanks for your comments. I also had doubt while using this function but then I guessed that @LazySingleton annotation will take care of that.

No worries. I will fix this thing and let you know in sometime.

Sounds great, I'll keep an eye on this pr.

gianm · 2018-04-12T03:11:12Z

Hi @Sarvesh1990, are you still interested in working on this feature?

b-slim · 2018-05-03T17:39:06Z

@Sarvesh1990 can you please take this to the finish line? thanks

Sarvesh1990 · 2018-05-05T12:55:17Z

Hey. I am really sorry for delaying this. Actually I had to go out of staion because of some important work. I will finish this feature by next week for sure. Also I will start contributing more seriously from next week.

… used

Sarvesh1990 · 2018-05-25T14:15:59Z

Hey @gianm @b-slim

Please find the changes as @gianm wanted in his previous comment. Have moved the responsibility to print merge pool to the pool emitter. Have also renamed ExecutorServiceMonitor to ProcessingMonitor so that we can use the same monitor to print all processing related data.

Let me know if it looks okay. Sorry for delaying this. Will be regular from now.

Sarvesh1990 · 2018-05-29T08:27:45Z

Hey @gianm @b-slim

Does this PR looks okay? Can I start working on the other one?

Sarvesh1990 · 2018-06-12T04:35:29Z

Hey @gianm @b-slim

Can you please review this PR if it looks okay?

kaijianding · 2018-08-12T13:45:32Z

docs/content/operations/metrics.md

@@ -37,6 +37,7 @@ Available Metrics
 |`query/success/count`|number of queries successfully processed|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/failed/count`|number of failed queries|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/interrupted/count`|number of queries interrupted due to cancellation or timeout|This metric is only available if the QueryCountStatsMonitor module is included.||
+|`query/merge/buffersUsed`|number of merge buffers allocated to broker while performing groupBy merge queries|||


I guess the DruidProcessingModule.getMergeBufferPool() is called GroupByStrategyV2, and GroupByStrategyV2 is called in historical and realtime when group by query, so this metric also should be documented in historical and realtime section?

@kaijianding is right - it should be documented in those other sections as well.

gianm

Thanks @Sarvesh1990 for the contribution! This looks generally good but please take a look at the couple of comments.

gianm · 2018-11-28T10:32:09Z

docs/content/operations/metrics.md

@@ -37,6 +37,7 @@ Available Metrics
 |`query/success/count`|number of queries successfully processed|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/failed/count`|number of failed queries|This metric is only available if the QueryCountStatsMonitor module is included.||
 |`query/interrupted/count`|number of queries interrupted due to cancellation or timeout|This metric is only available if the QueryCountStatsMonitor module is included.||
+|`query/merge/buffersUsed`|number of merge buffers allocated to broker while performing groupBy merge queries|||


@kaijianding is right - it should be documented in those other sections as well.

gianm · 2018-11-28T10:33:38Z

processing/src/main/java/io/druid/query/MetricsEmittingMergingBlockingPool.java

+  public MetricsEmittingMergingBlockingPool(
+          Supplier<T> generator,
+          int limit,
+          ProcessingMonitor processingMonitor)


Code style is a bit off here (the ending paren should be on its own line; also, indentation). If you are using IntelliJ, you can apply Druid's code style from the codestyle config in the repo. Also, our checkstyle should catch some things like this.

gianm · 2018-12-04T21:33:17Z

Hey @Sarvesh1990 - hope the review was helpful, let us know if you have any questions about further implementation!

gianm · 2018-12-20T23:14:53Z

Hey @Sarvesh1990, do you think you'll have an opportunity to pick this up in the future?

gianm · 2019-01-08T21:42:51Z

Hi @Sarvesh1990, I'm going to close this, but thanks for the interest in contributing, and please reopen it if you have the time to work on it some more.

Sarvesh1990 mentioned this pull request Mar 27, 2018

Metrics for merge buffer usage #4345

Open

gianm reviewed Mar 27, 2018

View reviewed changes

jihoonson reviewed Mar 28, 2018

View reviewed changes

gianm added Area - Metrics/Event Emitting Feature labels Apr 2, 2018

gianm reviewed Apr 2, 2018

View reviewed changes

Sarvesh Kumar added 3 commits May 25, 2018 17:39

Added merge/buffer metric to print buffer pool size every minute

b62d88b

Added documentation, changed metric from buffers remaining to buffers…

baad03b

… used

Formatting change, javadoc

cba920f

Sarvesh1990 force-pushed the feature/merge_buffer branch from f8179c1 to ddff801 Compare May 25, 2018 14:01

Moved metric for merge pool from exectuor service to independent class

689792a

Sarvesh1990 force-pushed the feature/merge_buffer branch from ddff801 to 689792a Compare May 25, 2018 14:11

kaijianding reviewed Aug 12, 2018

View reviewed changes

gianm self-assigned this Sep 28, 2018

gianm reviewed Nov 28, 2018

View reviewed changes

gianm closed this Jan 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added merge/buffer metric to print buffer pool size every minute #5541

Added merge/buffer metric to print buffer pool size every minute #5541

Sarvesh1990 commented Mar 27, 2018

gianm left a comment

gianm Mar 27, 2018

gianm Mar 27, 2018

Sarvesh1990 Mar 28, 2018 •

edited

Loading

jihoonson left a comment

jihoonson Mar 28, 2018

jihoonson Mar 28, 2018

jihoonson Mar 28, 2018

Sarvesh1990 Mar 29, 2018

Sarvesh1990 commented Apr 2, 2018

gianm left a comment

gianm Apr 2, 2018

Sarvesh1990 Apr 3, 2018

gianm Apr 3, 2018

gianm commented Apr 12, 2018

b-slim commented May 3, 2018

Sarvesh1990 commented May 5, 2018

Sarvesh1990 commented May 25, 2018

Sarvesh1990 commented May 29, 2018

Sarvesh1990 commented Jun 12, 2018

kaijianding Aug 12, 2018

gianm Nov 28, 2018

gianm left a comment

gianm Nov 28, 2018

gianm Nov 28, 2018

gianm commented Dec 4, 2018

gianm commented Dec 20, 2018

gianm commented Jan 8, 2019

Added merge/buffer metric to print buffer pool size every minute #5541

Added merge/buffer metric to print buffer pool size every minute #5541

Conversation

Sarvesh1990 commented Mar 27, 2018

gianm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sarvesh1990 Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sarvesh1990 commented Apr 2, 2018

gianm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Apr 12, 2018

b-slim commented May 3, 2018

Sarvesh1990 commented May 5, 2018

Sarvesh1990 commented May 25, 2018

Sarvesh1990 commented May 29, 2018

Sarvesh1990 commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gianm commented Dec 4, 2018

gianm commented Dec 20, 2018

gianm commented Jan 8, 2019

Sarvesh1990 Mar 28, 2018 •

edited

Loading