Add metric -- count of queries waiting for merge buffers #15025

kaisun2000 · 2023-09-22T05:49:06Z

Description

Currently there is no metric to indicate the contention level for the shared resource -- merge buffer pools. Thus, it is hard to know that the query performance suffers from the contention for shared merge buffer pools. We did meet such performance degradation issue in the production environment.

This PR exposed the count of waiting queries (threads) blocking at in the merge buffers pools.

Summary of the changes for a quick reference:

BlockingPool interface was updated with a new method getPendingQueries to return the count of pending queries.
DefaultBlockingPool class was updated with the implementation of gathering the number of pending queries with an AtomicLong counter.
MergeBufferPoolMonitor class was created with the shared merge buffer pool (annotated with @Merging) injected. User can add this monitor to the configuration file such as druid.monitoring.monitors=["org.apache.druid.java.util.metrics.MergeBufferPoolMonitor", ...]
Unit test is added. This unit test validated the MergeBufferPoolMonitor would emit correct count of pending queries with a query blocking at the DefaultBlockingPool by taking more than the number of available buffers in the DefaultBlockingPool

Release note

NEW: You can now add a metric mergeBuffer\pendingRequests to monitor the contention level of the merge buffer pool. This metric is exposed through the QueryCountStatsMonitor

Key changed/added classes in this PR

You can monitor the number of pending queries at the merge buffer pool with the metric mergeBuffer/pendingRequest
Add the MergeBufferPoolMonitor to the configuration files of Broker, Historical or Router as druid.monitoring.monitors=["org.apache.druid.java.util.metrics.MergeBufferPoolMonitor", ...]

This PR has:

… buffers.

…nitor by default

suneet-s

Thanks for your first contribution!

I took a quick pass of the code and have some suggestions - specifically in the MergeBufferPoolMonitor that should make it easier to deploy.

Since this monitor runs only once on every emission period, the metrics only provide a snapshot in time of how many queries were waiting on merge buffers at that instant. Have you considered reporting metrics like mean, P98 and max time spent waiting on merge buffers instead?

That should provide visibility into how long queries are taking to acquire merge buffers on all the different services.

Another metric that may be interesting is the number of queries that timed out while waiting to acquire merge buffers.

processing/src/main/java/org/apache/druid/collections/DefaultBlockingPool.java

processing/src/main/java/org/apache/druid/java/util/metrics/MergeBufferPoolMonitor.java

LakshSingla

Thanks for the first PR! Left a few review comments.
Also, we should add the monitor to docs/configuration/index.md along with the other monitors.

processing/src/main/java/org/apache/druid/java/util/metrics/MergeBufferPoolMonitor.java

LakshSingla · 2023-09-26T22:55:26Z

processing/src/main/java/org/apache/druid/java/util/metrics/MergeBufferPoolMonitor.java

+
+  @Inject
+  public MergeBufferPoolMonitor(
+      @Merging BlockingPool<ByteBuffer> mergeBufferPoolIn


The monitor itself shouldn't care about the actual blocking pool. It should have a "stats provider" (interface) which exposes the method getPendingQueries.

You can take a look at the example: QueryCountStatsProvider and QueryResource for the pattern. You will need to wire it up in Guice as well.

@LakshSingla is there a reason you want to do it that way? what structure do you have in mind exactly?

processing/src/test/java/org/apache/druid/java/util/metrics/MergeBufferPoolMonitorTest.java

processing/src/main/java/org/apache/druid/collections/BlockingPool.java

abhishekagarwal87 · 2023-09-27T10:10:07Z

We don't need to add another monitor. The new metrics can be emitted from the QueryCountStatsMonitor.

kaisun2000 · 2023-09-27T21:48:14Z

We don't need to add another monitor. The new metrics can be emitted from the QueryCountStatsMonitor.

@abhishekagarwal87, just want to pan out the overall structure of add this metric from the QueryCountStatsMonitor and confirm the design before I make the changes.

QueryCountStatsMonitor takes a QueryCountStatsProvider in its constructor to provide the QueryCountStats (aggregate stats instead of query specific stats). The QueryCountStatsProvider is implemented as QueryResource (which implements the logic for query REST endpoints).

So here, I can add a parameter in QueryResource constructor as @Merging BlockingPool<ByteBuffer> mergeBufferPoolIn. And let Guice to inject the shared merge buffer pool. This way, we can reuse the same monitor.

In the interface of QueryCountStatsProvider, we need to add a method to report the pending query count such as public long getPendingQueriesCount()

We will also need to keep the same changes in this PR to add a method in BlockingPool to report the pending queries in the pool, as public long getPendingQueries()

Let me know if I missed anything. Or next I will update the PR to reflect this changes discussed here.

abhishekagarwal87 · 2023-09-29T07:21:54Z

@kaisun2000 - How about injecting BlockingPool directly into the QueryCountStatsMonitor? Though seems like Laksh has objections to injecting pool into a monitor. I will let him chime in too.

kaisun2000 · 2023-10-02T22:48:47Z

@kaisun2000 - How about injecting BlockingPool directly into the QueryCountStatsMonitor? Though seems like Laksh has objections to injecting pool into a monitor. I will let him chime in too.

@abhishekagarwal87, thx for coordinating this. @LakshSingla , let me know your take here.

In this case, I will inject the BlockinPool to QueryCountStatsMonitor and not touching the QueryCountStatsProvider interface. Will send the revised PR next.

…rgebuffer-waiting-queries-count

LakshSingla · 2023-10-03T07:23:42Z

Though seems like Laksh has objections to injecting pool into a monitor. I will let him chime in too.

I had concerns with the monitor directly interacting with the merge buffer pool, instead of the stats provider that it was supposed to work with. However, I don't have a strong stance on the same, since its mostly internal implementation. The changes.

LakshSingla

Overall LGTM left a few comments. TY for taking this up!

server/src/test/java/org/apache/druid/server/metrics/QueryCountStatsMonitorTest.java

processing/src/main/java/org/apache/druid/collections/BlockingPool.java

processing/src/main/java/org/apache/druid/collections/DefaultBlockingPool.java

server/src/main/java/org/apache/druid/server/metrics/QueryCountStatsMonitor.java

LakshSingla

Code coverage issues, however, the coverage looks good to me. The insufficiency is due to the DummyBlockingPool not being tested anywhere, which is okay to me.

LGTM 🚀

LakshSingla · 2023-10-09T07:25:26Z

ty for the contribution, I am merging the PR. Can you please add a release note to the description @kaisun2000? (a few line user-facing summary of the change) It helps in prepping up the release notes and calling out user-facing changes.

kaisun2000 · 2023-10-09T17:20:19Z

@LakshSingla, just added the release notes section. Thx for help to get this PR reviewed and checked in.

abhishekagarwal87 · 2023-10-10T06:26:58Z

thank you for your contribution @kaisun2000

Add 'mergeBuffer/pendingRequests' metric that exposes the count of waiting queries (threads) blocking in the merge buffers pools.

…des (#16992) #15025 adds mergeBuffer/pendingRequests metric in QueryCountStatsMonitor. Since real-time nodes also use the same merge buffers for queries and have QueryCountStatsMonitor , the documentation is being updated to include this metric.

) * Add metric -- count of queries waiting for merge buffers (apache#15025) Add 'mergeBuffer/pendingRequests' metric that exposes the count of waiting queries (threads) blocking in the merge buffers pools. * Update addMetric to build --------- Co-authored-by: kaisun2000 <[email protected]>

) (#223) * Add metric -- count of queries waiting for merge buffers (apache#15025) Add 'mergeBuffer/pendingRequests' metric that exposes the count of waiting queries (threads) blocking in the merge buffers pools. * Update addMetric to build --------- Co-authored-by: kaisun2000 <[email protected]>

…des (apache#16992) apache#15025 adds mergeBuffer/pendingRequests metric in QueryCountStatsMonitor. Since real-time nodes also use the same merge buffers for queries and have QueryCountStatsMonitor , the documentation is being updated to include this metric.

Add metric for the number of pending queries waiting for shared merge…

64c95d4

… buffers.

github-actions bot added the Area - Metrics/Event Emitting label Sep 22, 2023

Kai Sun added 2 commits September 22, 2023 01:08

fix static check failure; remove the registering of MergeBufferPoolMo…

06bb042

…nitor by default

enhance the unit test to make it not flaky

a15cba3

suneet-s reviewed Sep 27, 2023

View reviewed changes

LakshSingla reviewed Sep 27, 2023

View reviewed changes

Kai Sun added 2 commits October 2, 2023 17:03

Merge remote-tracking branch 'upstream/master' into feature-expose-me…

dbc272a

…rgebuffer-waiting-queries-count

revised base on feedback.

1352bd4

github-actions bot added Area - Documentation and removed Area - Metrics/Event Emitting labels Oct 3, 2023

LakshSingla reviewed Oct 3, 2023

View reviewed changes

address Laksh's comments

32f3697

LakshSingla approved these changes Oct 9, 2023

View reviewed changes

LakshSingla merged commit e2cc1c4 into apache:master Oct 9, 2023
63 of 65 checks passed

LakshSingla added Area - Metrics/Event Emitting Release Notes labels Oct 9, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

hardikbajaj mentioned this pull request Sep 3, 2024

Update Documentation on meregeBuffer/pendingRequests for Real-time nodes #16992

Merged

10 tasks

hardikbajaj mentioned this pull request Sep 4, 2024

Backport "Add metric -- count of queries waiting for merge buffers" … confluentinc/druid#223

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add metric -- count of queries waiting for merge buffers #15025

Add metric -- count of queries waiting for merge buffers #15025

kaisun2000 commented Sep 22, 2023 •

edited

Loading

suneet-s left a comment •

edited

Loading

LakshSingla left a comment

LakshSingla Sep 26, 2023

abhishekagarwal87 Sep 29, 2023

abhishekagarwal87 commented Sep 27, 2023

kaisun2000 commented Sep 27, 2023 •

edited

Loading

abhishekagarwal87 commented Sep 29, 2023

kaisun2000 commented Oct 2, 2023 •

edited

Loading

LakshSingla commented Oct 3, 2023

LakshSingla left a comment

LakshSingla left a comment

LakshSingla commented Oct 9, 2023

kaisun2000 commented Oct 9, 2023

abhishekagarwal87 commented Oct 10, 2023

Add metric -- count of queries waiting for merge buffers #15025

Add metric -- count of queries waiting for merge buffers #15025

Conversation

kaisun2000 commented Sep 22, 2023 • edited Loading

Description

Release note

Key changed/added classes in this PR

suneet-s left a comment • edited Loading

Choose a reason for hiding this comment

LakshSingla left a comment

Choose a reason for hiding this comment

LakshSingla Sep 26, 2023

Choose a reason for hiding this comment

abhishekagarwal87 Sep 29, 2023

Choose a reason for hiding this comment

abhishekagarwal87 commented Sep 27, 2023

kaisun2000 commented Sep 27, 2023 • edited Loading

abhishekagarwal87 commented Sep 29, 2023

kaisun2000 commented Oct 2, 2023 • edited Loading

LakshSingla commented Oct 3, 2023

LakshSingla left a comment

Choose a reason for hiding this comment

LakshSingla left a comment

Choose a reason for hiding this comment

LakshSingla commented Oct 9, 2023

kaisun2000 commented Oct 9, 2023

abhishekagarwal87 commented Oct 10, 2023

kaisun2000 commented Sep 22, 2023 •

edited

Loading

suneet-s left a comment •

edited

Loading

kaisun2000 commented Sep 27, 2023 •

edited

Loading

kaisun2000 commented Oct 2, 2023 •

edited

Loading