Support thread pool metric collect by xu1009 · Pull Request #95 · apache/skywalking-java

xu1009 · 2022-01-20T15:18:07Z

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes [Feature] [Java] Support collect thread pool metrics skywalking#8433.
Update the CHANGES log.

wu-sheng · 2022-01-20T15:45:10Z

MeterService is already hosting the created metric. Please read how CounterConstructInterceptor works. And you should reduce unnecessary codes.

wu-sheng · 2022-01-20T15:53:13Z

BTW, I am asking @mrproliu to enhance the plugin test tool, which will be able to verify metrics, like it did for tracing data.

mrproliu · 2022-01-21T02:43:36Z

BTW, I am asking @mrproliu to enhance the plugin test tool, which will be able to verify metrics, like it did for tracing data.

I have already enhanced it, It could receive the meter data from meter report protocol. If you want to use it, here is the documentation and also have plugin test been used.

xu1009 · 2022-01-24T06:15:00Z

the way is to make new thread type add is simple, just get ThreadPoolExecutor instacne and register it, and then, the main metric can be send to the backend

wu-sheng · 2022-01-24T06:17:52Z

the way is to make new thread type add is simple, just get ThreadPoolExecutor instacne and register it, and then, the main metric can be send to the backend

You just need to create the metric, register it and hold it in the pool object. What we recommended is making your PR easier.

wu-sheng · 2022-01-24T06:20:31Z

@xu1009 Please take a deep like at our comments. Most things you did have been provided at the core level. Because most of this registration mechanism is a common requirement.

xu1009 · 2022-01-24T06:23:44Z

@xu1009 Please take a deep like at our comments. Most things you did have been provided at the core level. Because most of this registration mechanism is a common requirement.

in this way, every new plugin needs to create metirc, i want to extract these common things to core

wu-sheng · 2022-01-24T06:31:38Z

in this way, every new plugin needs to create metirc, i want to extract these common things to core

What is the issue of creating a metric?
You don't need to consider that. How to create a metric is a core level thing. The plugin should keep all logic simple.
If you want to discuss what core APIs lack, we could work on that separately.

wu-sheng · 2022-01-24T08:02:02Z

Please fix the CI and add meter plugin test according to the link above. #95 (comment)

xu1009 · 2022-01-24T09:06:44Z

ok, i add it lately

xu1009 · 2022-01-24T15:34:49Z

the test seems not work in new mac book with m1 cpu

wu-sheng · 2022-01-24T15:38:40Z

the test seems not work in new mac book with m1 cpu

Yes, we may not have enough images for m1 testing. As most of the team are still using the intel chip.

Could you find a Linux to run this? Or try to locate what is the block(s) in AARM?

wu-sheng · 2022-01-24T15:45:34Z

Please take a little look at #73, these 2 files should be updated, one for CI, the other for people reading.

xu1009 · 2022-01-24T15:45:48Z

the test seems not work in new mac book with m1 cpu

Yes, we may not have enough images for m1 testing. As most of the team are still using the intel chip.

Could you find a Linux to run this? Or try to locate what is the block(s) in AARM?

not have image, i will try it in other tomorrow, our company has used M1 widely ==

wu-sheng · 2022-01-24T15:49:22Z

not have image, i will try it in other tomorrow, our company has used M1 widely ==

I see, take a look at https://hub.docker.com/_/eclipse-temurin?tab=tags. AdoptOpenJDK has moved to Eclipse Temurin. I can see AARM images are available there.

FYI @dmsolr

wu-sheng · 2022-01-24T15:55:43Z

If you will submit any pull request about enhancing M1 testing, please submit through another pull request with detailed steps about running which case locally.
I think I am the only few ppl in the project committer team using the M1 series(M1 Max) chip. I have to verify this locally.

wu-sheng · 2022-01-27T15:37:41Z

Are you going to update this PR? No update for 3 days.

xu1009 · 2022-01-28T15:20:29Z

Are you going to update this PR? No update for 3 days.

sorry, very busy with my own job these days, i am lookig this

…thread-pool # Conflicts: # CHANGES.md

wu-sheng · 2022-01-29T00:16:29Z

+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>
+        <artifactId>optional-plugins</artifactId>


Questions,

Should we merge this plugin into the existing undertow tracing plugin?

Should we make it works in default? I can see this is an optional plugin, but a thread pool plugin should not impact the performance, right?

About (1), if you want this plugin to be disabled separately from the tracing plugin, I am fine to keep the codes in another plugin jar. But (2) should be considered.

@xu1009 Please follow this, then we could merge on both sides.

thread pool plugin may impact performance, because it has lock when java get the metric, so it is optional plugin, peoeple really need can add it

The fetching period is 20s, I think it's find to keep it in default.
This lock happens in every getting and returning threads from pool. It is not a big impact.
Agree?

yes, but the fetching period can be modified by user, if it is set 1s, and the service has high qps, it may impact

Manual setting equals optional plugin. So, don't worry.

so we make the plugin in default, an add config control it ?

Moving it to default is enough. Don't need to add a new config.

mrproliu

LGTM

…' into feature/support-java-thread-pool

kezhenxu94 · 2022-01-31T02:18:17Z

 8.10.0
 ------------------
-
+* Support Java thread pool metric collect.


This PR only provides thread pool metrics of Undertow, but the change log makes me (and probably other users) feel that we support JDK's thread pool. WDYT @wu-sheng ?

Updated, check the latest codes

I noted that the V9.0.0 just display tomcat thread pool metric. Now I have many customized thread pool which created by "org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor". How can I collect the metric of my thread pool and display it in UI？Is need to write another customized plugins or just need write some config?

Check #146 you may be interested.

I had checked #146 before i asked and i had read the code. The code does not contains any METRIC about "CorePoolSize","MaximumPoolSize".

I find a plugin more closer I wanted but hard coded[org.apache.skywalking.apm.plugin.undertow.worker.thread.pool.UndertowWorkerThreadPoolConstructorIntercept]. So I want to konw if i have to write some customized plugin which the count is same to the ThreadPool count？Or have some config more convenient？

Why only including two methods, there was a long discussion on that PR, you could check.

You definitely could write a new plugin to do whatever you want, but you should know, that the discussion context is recommended for you before you take action. There are risks to instrument all, which is why we don't do that.

Thanks, i will check later

@wu-sheng There are many thread pools in my project, such as ThreadPoolA and ThreadPoolB. If I want to monitor the metrics of these two thread pools separately, do I need to add a new plugin to the agent and two rules to the threadpool.ymal of OAP like:
- name: ThreadPoolA
exp: ThreadPoolA.avg(['metric_type', 'pool_name', 'instance', 'service'])
- name: ThreadPoolB
exp: ThreadPoolB.avg(['metric_type', 'pool_name', 'instance', 'service'])

Learn MAL, your questions are irrelevant

litexu added 2 commits January 20, 2022 23:12

init

d839777

change log

e0723fd

wu-sheng reviewed Jan 20, 2022

View reviewed changes

Comment thread ...-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/theadpool/ThreadPoolName.java Outdated

add licence

1c7b9a7

wu-sheng reviewed Jan 20, 2022

View reviewed changes

Comment thread ...nt-core/src/main/java/org/apache/skywalking/apm/agent/core/theadpool/ThreadPoolRegister.java Outdated

wu-sheng reviewed Jan 20, 2022

View reviewed changes

Comment thread ...ing/apm/plugin/undertow/worker/thread/pool/UndertowWorkerThreadPoolConstructorIntercept.java Outdated

wu-sheng added enhancement New feature or request plugin labels Jan 20, 2022

wu-sheng added this to the 8.9.0 milestone Jan 20, 2022

wu-sheng mentioned this pull request Jan 20, 2022

Support thread pool metric collect apache/skywalking#8461

Merged

2 tasks

mrproliu reviewed Jan 21, 2022

View reviewed changes

Comment thread ...ent-core/src/main/java/org/apache/skywalking/apm/agent/core/theadpool/ThreadPoolService.java Outdated

rm

8f76788

wu-sheng mentioned this pull request Jan 24, 2022

[Feature] Support collecting database connection pool metric,support dbcp2/druid/hikari #96

Merged

2 tasks

wu-sheng removed this from the 8.9.0 milestone Jan 28, 2022

litexu and others added 6 commits January 29, 2022 01:20

test support

a50a0dc

update doc

92aa411

Merge remote-tracking branch 'origin/main' into feature/support-java-…

f98cdcc

…thread-pool # Conflicts: # CHANGES.md

update to 8.10.0

77b21ae

add test

e2c9920

Update Supported-list.md

c894229

wu-sheng requested a review from mrproliu January 29, 2022 00:01

wu-sheng added this to the 8.10.0 milestone Jan 29, 2022

wu-sheng reviewed Jan 29, 2022

View reviewed changes

mrproliu previously approved these changes Jan 29, 2022

View reviewed changes

litexu added 2 commits January 29, 2022 13:59

mv to default

725b47c

Merge remote-tracking branch 'origin/feature/support-java-thread-pool…

056f2b7

…' into feature/support-java-thread-pool

xu1009 dismissed mrproliu’s stale review via 056f2b7 January 29, 2022 05:59

wu-sheng approved these changes Jan 29, 2022

View reviewed changes

wu-sheng merged commit 8e3213b into apache:main Jan 29, 2022

kezhenxu94 reviewed Jan 31, 2022

View reviewed changes

wu-sheng mentioned this pull request Aug 4, 2022

[Bug] ThreadPoolExecutor can not be enhanced apache/skywalking#9425

Closed

3 tasks

Conversation

xu1009 commented Jan 20, 2022 • edited by wu-sheng Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wu-sheng commented Jan 20, 2022

Uh oh!

wu-sheng commented Jan 20, 2022

Uh oh!

Uh oh!

mrproliu commented Jan 21, 2022

Uh oh!

xu1009 commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

xu1009 commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

xu1009 commented Jan 24, 2022

Uh oh!

xu1009 commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

xu1009 commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 27, 2022

Uh oh!

xu1009 commented Jan 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrproliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu1009 commented Jan 20, 2022 •

edited by wu-sheng

Loading

xu1009 commented Jan 24, 2022 •

edited

Loading