[GitHub] [skywalking-java] lujiajing1126 commented on a change in pull request #96: [Feature] Support collecting database connection pool metric,support druid/hikari #8450

[GitBox <gi...@apache.org>]

lujiajing1126 commented on a change in pull request #96:
URL: https://github.com/apache/skywalking-java/pull/96#discussion_r794312663



##########
File path: apm-sniffer/apm-sdk-plugin/dbcp-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/dbcp/v2/define/BasicDataSourceInstrumentation.java
##########
@@ -60,6 +63,22 @@ public String getMethodsInterceptor() {
                         return CONNECT_GET_INTERCEPTOR;
                     }
 
+                    @Override
+                    public boolean isOverrideArgs() {
+                        return false;
+                    }
+                },
+                new InstanceMethodsInterceptPoint() {
+                    @Override
+                    public ElementMatcher<MethodDescription> getMethodsMatcher() {
+                        return named("setUrl").and(takesArgument(0, String.class));

Review comment:
       I suggest not adding the hook for metrics at this point.
   
   1. the datasource may not be fully initialized then.
   2. Versions `1.0.x` and `>=1.1.0` may behavior quite differently.
   
   The `Alibaba/Druid` authors use the following method  (static method `DruidDataSourceStatManager.addDataSource`) to register `MBean` which means at this stage, the metrics can be exposed.
   
   https://github.com/alibaba/druid/blob/f937ecd43363b2df61afdc120858097cb2510580/src/main/java/com/alibaba/druid/pool/DruidDataSource.java#L2151
   
   Besides, we can make sure that each DataSource is only registered once which is guarded by the `mbeanRegistered` property.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org