Add CommonsObjectPool2Metrics #1712
Conversation
|
Can this code be reused to implement #1710 |
chang-chao
force-pushed
the
master
branch
2 times, most recently
from
1f6a295
to
5355458
Compare
because dbcp2 uses commons pool2, so I believe yes.all the common metrics of commons pool will be exposed, like For the metrics specific to dbcp, like |
| @Override | ||
| public void bindTo(MeterRegistry registry) { | ||
| for (String type : TYPES) { | ||
| registerMetricsEventually( |
There was a problem hiding this comment.
Is there a non-JMX way to register these?
There was a problem hiding this comment.
Other ways are not always possible as far as I know, because libraries using commons-pool2(like jedis or dbcp2) keeps the pool objects as internal properties, and we cannot get them.
and commons-pool exposes JMX, so using JMX seems to be a natural fit.
There was a problem hiding this comment.
I agree JMX as the default is a good idea. I was more curious if there was a way if we did have access to the pool directly. I used commons pool in some of my own code and didn't have JMX enabled at the time. It would be nice to offer a way to still gather those metrics if posssible.
There was a problem hiding this comment.
As we can see from the following code, JMX registration in commons-pool2 is enabled by default , unless you configure to disable it explicitly, which I don't think a serious application should do.
https://github.com/apache/commons-pool/blob/2d2049233940b10c7dd35df70a353d383f601053/src/main/java/org/apache/commons/pool2/impl/BaseObjectPoolConfig.java#L139
| "objects"); | ||
|
|
||
| registerTimeGaugeForObject( | ||
| registry, o, "MaxBorrowWaitTimeMillis", "maxBorrowWaitTime", tags, "The maximum time a thread has waited to borrow objects from the pool"); |
There was a problem hiding this comment.
Please use dot separated names like max.borrow.wait.time, which will be camel-cased by certain NamingConvention implementations. By keeping dot-separated names in the core code, we maximize the portability of the metric to many different monitoring systems.
There was a problem hiding this comment.
@jkschneider thanks for pointing it out, fixed in 32c5c0d
|
@chao-chang-paypay is this PR considered finished? Can I help you somehow? I'm actually looking forward to this update 👍 |
|
Thanks for the hard work on this @chao-chang-paypay. I can see that this was a non-trivial implementation, and you did a fantastic job on it! Polished a bit and merged with 2f7f51b. |
|
I already see these code in branch Master，but can not find tnem in the latest release version. What's the problem? and i can not wait anymore. |
|
@chang-chao @jkschneider I am using 1.6.0 version of |
|
@AnkitKaneri There's no out-of-the-box support for |
fixes #1711