[Feature] Support collecting database connection pool metric,support dbcp2/druid/hikari by sunOnly · Pull Request #96 · apache/skywalking-java

sunOnly · 2022-01-24T12:38:47Z

If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
Update the CHANGES log.

…druid/hikari #8450

wu-sheng · 2022-01-24T12:45:07Z

Please follow the discussion starting from #95 (comment).

Don't build the register mechanism. This is not a good practice.

Also, follow this, #95 (comment), meter plugin test is required like the trace plugin test.

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

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

…druid/hikari/dbcp2 #8450

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

Merge branch 'main' of github.com:sunOnly/skywalking-java into main

wu-sheng · 2022-01-28T03:57:21Z

Please fix CI and follow the comments. Then, you need to add plugin tests.

lujiajing1126 · 2022-01-28T09:10:39Z

Additionally for HikariCP, they have some extra metrics at runtime,

https://github.com/brettwooldridge/HikariCP/blob/dev/src/main/java/com/zaxxer/hikari/metrics/IMetricsTracker.java

Are you going to consider these?

wu-sheng · 2022-01-29T07:46:23Z

The other undertow thread pool PR has been merged. You could take a reference.

wu-sheng · 2022-02-07T06:32:24Z

@sunOnly Are you going to continue on this?

sunOnly · 2022-02-07T06:59:59Z

@sunOnly Are you going to continue on this?
Yes

wu-sheng · 2022-02-07T07:03:09Z

Thanks for the feedback.

lujiajing1126 · 2022-02-09T02:52:25Z

+                new InstanceMethodsInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getMethodsMatcher() {
+                        return named("setUrl");


Just a final comment:

As I've stated before, we have to be careful with the interceptor points for Alibaba/Druid and HikariCP. But I'am not quite familiar with this dbcp2 DataSource.

So, could you please justify the choice of this interceptor point?

@sunOnly First of all, good to see the tests passed. Let's follow this one. #setUrl is not a good point to begin the monitoring, because in the runtime, a datasource could be not-created at this moment, even never(code bug).

I was looking through the codes, it is likely createDataSource is a better method to begin all metrics registration and reporting. Meanwhile, I understand you intercept #setUrl is to get the URL parameter, I noticed there is getUrl method in the class, but with a synchronized, which I have concerns about deadlock. So, it is better you keep this setUrl method intercepting, but keep the value of URL into the EnhancedInstance#setSkyWalkingDynamicField. Then in new createDataSource interceptor, you could use #getSkyWalkingDynamicField to read this.

This would be more robust.

@sunOnly First of all, good to see the tests passed. Let's follow this one. #setUrl is not a good point to begin the monitoring, because in the runtime, a datasource could be not-created at this moment, even never(code bug).

I was looking through the codes, it is likely createDataSource is a better method to begin all metrics registration and reporting. Meanwhile, I understand you intercept #setUrl is to get the URL parameter, I noticed there is getUrl method in the class, but with a synchronized, which I have concerns about deadlock. So, it is better you keep this setUrl method intercepting, but keep the value of URL into the EnhancedInstance#setSkyWalkingDynamicField. Then in new createDataSource interceptor, you could use #getSkyWalkingDynamicField to read this.

This would be more robust.
get

I was looking through the codes, it is likely createDataSource is a better method to begin all metrics registration and reporting. Meanwhile, I understand you intercept #setUrl is to get the URL parameter, I noticed there is getUrl method in the class, but with a synchronized, which I have concerns about deadlock. So, it is better you keep this setUrl method intercepting, but keep the value of URL into the EnhancedInstance#setSkyWalkingDynamicField. Then in new createDataSource interceptor, you could use #getSkyWalkingDynamicField to read this.

What about using jmxRegister() called within createDataSource() (if dataSource is not created) as the interception point?

Since every call to getConnection() will invoke createDataSource(), the performance may be impacted. It also exists from 2.0.1 to 2.9.0.

Since every call to getConnection() will invoke createDataSource()

Really? This doesn't seem reasonable.

What about using jmxRegister() called within createDataSource() (if dataSource is not created) as the interception point?

@sunOnly Could you try this?

Since every call to getConnection() will invoke createDataSource()

Really? This doesn't seem reasonable.

https://github.com/apache/commons-dbcp/blob/466091e51d875d2a4499adbbb63058e32ad5a7c7/src/main/java/org/apache/commons/dbcp2/BasicDataSource.java#L732

Seems so.

wu-sheng · 2022-02-09T05:34:01Z

It seems one changed case still fails.

wu-sheng · 2022-02-11T00:30:24Z

Another question, I noticed in the commit log, there are 2 ID, one is @sunOnly (GitHub), the other is xiaojianjun. Are these belong to the same person?

sunOnly · 2022-02-11T01:51:50Z

Another question, I noticed in the commit log, there are 2 ID, one is @sunOnly (GitHub), the other is xiaojianjun. Are these belong to the same person?

It's the same person

wu-sheng · 2022-02-11T02:24:48Z

Please follow the comment above, there is one more todo.

mrproliu · 2022-02-11T02:34:41Z

                        <sourceDirectory>${project.build.sourceDirectory}</sourceDirectory>
                        <sourceDirectory>${project.build.testSourceDirectory}</sourceDirectory>
+<sourceDirectory>scenarios/dbcp-2.x-scenario</sourceDirectory>
+<sourceDirectory>scenarios/druid-1.x-scenario</sourceDirectory>


Please remove these declare, this should be generated by CI.

wu-sheng · 2022-02-11T03:55:50Z

@lujiajing1126 Please recheck.

wu-sheng

Once tests pass, I am good.

lujiajing1126 · 2022-02-11T04:19:04Z

+    public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+        ConnectionInfo connectionInfo = URLParser.parser((String) allArguments[0]);
+        String tagValue = connectionInfo.getDatabaseName() + "_" + connectionInfo.getDatabasePeer();
+        if (tagValue != null) {


As I understood, it could not be null?

As I understood, it could not be null?

The external call may be set to null

As I understood, it could not be null?
change to
if (allArguments[0] != null) {
ConnectionInfo connectionInfo = URLParser.parser((String) allArguments[0]);
String tagValue = connectionInfo.getDatabaseName() + "_" + connectionInfo.getDatabasePeer();
objInst.setSkyWalkingDynamicField(tagValue);
}

I think he means _ is in the string combining. So, it can't be null.

I think he means _ is in the string combining. So, it can't be null.

Exactly.

lujiajing1126 · 2022-02-11T04:23:29Z

+                new InstanceMethodsInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getMethodsMatcher() {
+                        return named("setUrl");


I was looking through the codes, it is likely createDataSource is a better method to begin all metrics registration and reporting. Meanwhile, I understand you intercept #setUrl is to get the URL parameter, I noticed there is getUrl method in the class, but with a synchronized, which I have concerns about deadlock. So, it is better you keep this setUrl method intercepting, but keep the value of URL into the EnhancedInstance#setSkyWalkingDynamicField. Then in new createDataSource interceptor, you could use #getSkyWalkingDynamicField to read this.

What about using jmxRegister() called within createDataSource() (if dataSource is not created) as the interception point?

Since every call to getConnection() will invoke createDataSource(), the performance may be impacted. It also exists from 2.0.1 to 2.9.0.

lujiajing1126 · 2022-02-11T05:50:50Z

I'm good with the last version. Let's wait for the CI.

lujiajing1126

LGTM. Thanks for your patience!

…he#96)

xiaojianjun added 2 commits January 24, 2022 20:35

[Feature] Support collecting database connection pool metric,support …

03d2bfc

…druid/hikari #8450

[Feature] Support collecting database connection pool metric,support …

5e89096

…druid/hikari #8450

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

wu-sheng mentioned this pull request Jan 24, 2022

[Feature] Support datasource metrics analysis apache/skywalking#8474

Merged

2 tasks

sunOnly and others added 5 commits January 28, 2022 10:21

Merge branch 'apache:main' into main

e15380f

[Feature] Support collecting database connection pool metric,support …

5f6aabe

…druid/hikari/dbcp2 #8450

Merge branch 'main' of github.com:sunOnly/skywalking-java into main

63e68b5

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

Merge branch 'apache:main' into main

8ba0cac

:wq

13b1aa9

Merge branch 'main' of github.com:sunOnly/skywalking-java into main

wu-sheng reviewed Jan 28, 2022

View reviewed changes

Comment thread ...ain/java/org/apache/skywalking/apm/plugin/dbcp/v2/define/BasicDataSourceInstrumentation.java Outdated

wu-sheng reviewed Jan 28, 2022

View reviewed changes

Comment thread ...in/java/org/apache/skywalking/apm/plugin/druid/v1/define/DruidDataSourceInstrumentation.java Outdated

wu-sheng reviewed Jan 28, 2022

View reviewed changes

Comment thread ...rc/main/java/org/apache/skywalking/apm/plugin/hikaricp/define/HikariPoolInstrumentation.java Outdated

wu-sheng reviewed Jan 28, 2022

View reviewed changes

Comment thread ...rc/main/java/org/apache/skywalking/apm/plugin/hikaricp/define/HikariPoolInstrumentation.java Outdated

modify format#8450

6ca6867

lujiajing1126 requested changes Jan 28, 2022

View reviewed changes

Comment thread ...in/java/org/apache/skywalking/apm/plugin/druid/v1/define/DruidDataSourceInstrumentation.java Outdated

lujiajing1126 reviewed Jan 28, 2022

View reviewed changes

Comment thread ...rc/main/java/org/apache/skywalking/apm/plugin/hikaricp/define/HikariPoolInstrumentation.java

xiaojianjun and others added 2 commits January 28, 2022 18:09

druid metric monitor #8450

5b2f044

Merge branch 'apache:main' into main

1a4da64

kezhenxu94 reviewed Jan 31, 2022

View reviewed changes

Comment thread ...n/java/org/apache/skywalking/apm/plugin/hikaricp/HikariDataSourceConstructorInterceptor.java Outdated

add support config params collect apache#96

6ab8cf7

lujiajing1126 reviewed Feb 9, 2022

View reviewed changes

xiaojianjun added 2 commits February 9, 2022 11:48

remove static field and log apache#96

c6adcf8

format apache#96

15aa2ab

xiaojianjun and others added 7 commits February 9, 2022 16:05

optimization apache#96

c08204b

remove space key apache#96

fd9b150

Merge branch 'main' into main

0e142fc

add tomcat metric apache#96

1ec6f11

Merge branch 'main' of github.com:sunOnly/skywalking-java into main

bbcfb63

validate value change apache#96

b6ee8a0

remove datasource metric apache#96

820cd0f

wu-sheng added feature and removed enhancement New feature or request labels Feb 11, 2022

Merge branch 'main' into main

aa9f446

mrproliu reviewed Feb 11, 2022

View reviewed changes

xiaojianjun added 2 commits February 11, 2022 11:06

optimize interception point apache#96

8ae7517

Merge branch 'main' of github.com:sunOnly/skywalking-java into main

25c9f46

sunOnly requested a review from lujiajing1126 February 11, 2022 03:29

wu-sheng previously approved these changes Feb 11, 2022

View reviewed changes

lujiajing1126 requested changes Feb 11, 2022

View reviewed changes

Optimize interception point apache#96

bee196f

sunOnly dismissed wu-sheng’s stale review via bee196f February 11, 2022 05:46

lujiajing1126 approved these changes Feb 11, 2022

View reviewed changes

wu-sheng merged commit 00cf97e into apache:main Feb 11, 2022

GuoHaoZai pushed a commit to GuoHaoZai/skywalking-java that referenced this pull request Apr 24, 2025

Adding exclude namespace policy in the eBPF Access Log Protocol (apac…

b5f6ebe

…he#96)

Conversation

sunOnly commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 24, 2022

Uh oh!

wu-sheng commented Jan 27, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wu-sheng commented Jan 28, 2022

Uh oh!

Uh oh!

lujiajing1126 commented Jan 28, 2022

Uh oh!

Uh oh!

wu-sheng commented Jan 29, 2022

Uh oh!

Uh oh!

wu-sheng commented Feb 7, 2022

Uh oh!

sunOnly commented Feb 7, 2022

Uh oh!

wu-sheng commented Feb 7, 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!

wu-sheng commented Feb 9, 2022

Uh oh!

wu-sheng commented Feb 11, 2022

Uh oh!

sunOnly commented Feb 11, 2022

Uh oh!

wu-sheng commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-sheng commented Feb 11, 2022

Uh oh!

wu-sheng 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!

wu-sheng Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lujiajing1126 commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lujiajing1126 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

wu-sheng Feb 11, 2022 •

edited

Loading

lujiajing1126 commented Feb 11, 2022 •

edited

Loading