[GitHub] [skywalking] Patrick0308 commented on a change in pull request #5109: Fix issue 4965
Patrick0308 commented on a change in pull request #5109:
URL: https://github.com/apache/skywalking/pull/5109#discussion_r456210367
##
File path:
apm-sniffer/apm-sdk-plugin/mysql-common/src/main/java/org/apache/skywalking/apm/plugin/jdbc/mysql/PreparedStatementExecuteMethodsInterceptor.java
##
@@ -40,15 +40,14 @@
public final void beforeMethod(EnhancedInstance objInst, Method method,
Object[] allArguments,
Class[] argumentsTypes,
MethodInterceptResult result) {
StatementEnhanceInfos cacheObject = (StatementEnhanceInfos)
objInst.getSkyWalkingDynamicField();
-ConnectionInfo connectInfo = cacheObject.getConnectionInfo();
/**
* For avoid NPE. In this particular case, Execute sql inside the
{@link com.mysql.jdbc.ConnectionImpl} constructor,
* before the interceptor sets the connectionInfo.
- *
+ * Fixed https://github.com/apache/skywalking/issues/4965. When
invoking prepareCall, cacheObject is null. Because it will determine
procedures's parameter types by querying the table of proc in mysql.
* @see JDBCDriverInterceptor#afterMethod(EnhancedInstance, Method,
Object[], Class[], Object)
*/
-if (connectInfo != null) {
-
+if (cacheObject != null && cacheObject.getConnectionInfo() != null) {
Review comment:
Intercepter set `connectionInfo` after `Driver.connect`. So, in the
constructor of `ConnectionImpl`, executing built-in SQL will don't have
`connectionInfo`.
https://github.com/apache/skywalking/blob/e7e72d937b769f40e50ec644a210bc497f2204f9/apm-sniffer/apm-sdk-plugin/mysql-common/src/main/java/org/apache/skywalking/apm/plugin/jdbc/mysql/PreparedStatementExecuteMethodsInterceptor.java#L44
Both the condition `cacheObject.getConnectionInfo() != null` and the
condition `cacheObject != null` are for not log error message and skip the
jdbc's built-in SQL.
The NPE not affect recording user's SQL.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] Patrick0308 commented on a change in pull request #5109: Fix issue 4965
Patrick0308 commented on a change in pull request #5109:
URL: https://github.com/apache/skywalking/pull/5109#discussion_r456210367
##
File path:
apm-sniffer/apm-sdk-plugin/mysql-common/src/main/java/org/apache/skywalking/apm/plugin/jdbc/mysql/PreparedStatementExecuteMethodsInterceptor.java
##
@@ -40,15 +40,14 @@
public final void beforeMethod(EnhancedInstance objInst, Method method,
Object[] allArguments,
Class[] argumentsTypes,
MethodInterceptResult result) {
StatementEnhanceInfos cacheObject = (StatementEnhanceInfos)
objInst.getSkyWalkingDynamicField();
-ConnectionInfo connectInfo = cacheObject.getConnectionInfo();
/**
* For avoid NPE. In this particular case, Execute sql inside the
{@link com.mysql.jdbc.ConnectionImpl} constructor,
* before the interceptor sets the connectionInfo.
- *
+ * Fixed https://github.com/apache/skywalking/issues/4965. When
invoking prepareCall, cacheObject is null. Because it will determine
procedures's parameter types by querying the table of proc in mysql.
* @see JDBCDriverInterceptor#afterMethod(EnhancedInstance, Method,
Object[], Class[], Object)
*/
-if (connectInfo != null) {
-
+if (cacheObject != null && cacheObject.getConnectionInfo() != null) {
Review comment:
Intercepter set `connectionInfo` after `Driver.connect`. However, in the
constructor of `ConnectionImpl`, executing built-in SQL will don't have
`connectionInfo`.
https://github.com/apache/skywalking/blob/e7e72d937b769f40e50ec644a210bc497f2204f9/apm-sniffer/apm-sdk-plugin/mysql-common/src/main/java/org/apache/skywalking/apm/plugin/jdbc/mysql/PreparedStatementExecuteMethodsInterceptor.java#L44
Both the condition `cacheObject.getConnectionInfo() != null` and the
condition `cacheObject != null` are for not log error message and skip the
jdbc's built-in SQL.
And the NPE not affect recording user's SQL.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [skywalking] Patrick0308 commented on a change in pull request #5109: Fix issue 4965
Patrick0308 commented on a change in pull request #5109:
URL: https://github.com/apache/skywalking/pull/5109#discussion_r456179614
##
File path:
apm-sniffer/apm-sdk-plugin/mysql-common/src/main/java/org/apache/skywalking/apm/plugin/jdbc/mysql/PreparedStatementExecuteMethodsInterceptor.java
##
@@ -40,15 +40,14 @@
public final void beforeMethod(EnhancedInstance objInst, Method method,
Object[] allArguments,
Class[] argumentsTypes,
MethodInterceptResult result) {
StatementEnhanceInfos cacheObject = (StatementEnhanceInfos)
objInst.getSkyWalkingDynamicField();
-ConnectionInfo connectInfo = cacheObject.getConnectionInfo();
/**
* For avoid NPE. In this particular case, Execute sql inside the
{@link com.mysql.jdbc.ConnectionImpl} constructor,
* before the interceptor sets the connectionInfo.
- *
+ * Fixed https://github.com/apache/skywalking/issues/4965. When
invoking prepareCall, cacheObject is null. Because it will determine
procedures's parameter types by querying the table of proc in mysql.
Review comment:
When invoking prepareCall, it will determine procedures's parameter
types by execute sql.` StatementEnhanceInfos` is set after prepareCall, so
`cacheObject is` null in
`PreparedStatementExecuteMethodsInterceptor.beforeMethod`.
If we don't need to record the sql which determine procedures's parameter
types when invoking prepareCall, we can skip it.
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
