You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "sky-dawn (via GitHub)" <gi...@apache.org> on 2023/05/04 08:41:19 UTC
[GitHub] [skywalking-java] sky-dawn opened a new pull request, #524: Fix SQLServer write data intercept failure (#10741)
sky-dawn opened a new pull request, #524:
URL: https://github.com/apache/skywalking-java/pull/524
<!--
β οΈ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
Fix SQLServer write data intercept failure
The issue link: [https://github.com/apache/skywalking/issues/10741](url)
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ====
### Add an agent plugin to support <framework name>
- [ ] Add a test case for the new plugin, refer to [the doc](https://github.com/apache/skywalking-java/blob/main/docs/en/setup/service-agent/java-agent/Plugin-test.md)
- [ ] Add a component id in [the component-libraries.yml](https://github.com/apache/skywalking/blob/master/oap-server/server-starter/src/main/resources/component-libraries.yml)
- [ ] Add a logo in [the UI repo](https://github.com/apache/skywalking-rocketbot-ui/tree/master/src/views/components/topology/assets)
==== π Remove this line WHEN AND ONLY WHEN you're adding a new plugin, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking-java/blob/main/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== π Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist π ==== -->
<!-- ==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ====
### <Feature description>
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
==== π Remove this line WHEN AND ONLY WHEN you're adding a new feature, follow the checklist π ==== -->
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
--
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
[GitHub] [skywalking-java] wu-sheng commented on pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#issuecomment-1535818840
```
[INFO] Starting audit...
Error: /home/runner/work/skywalking-java/skywalking-java/apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/PSSetterDefinitionOfJDBCInstrumentation.java:27:52: Using the '.*' form of import should be avoided - net.bytebuddy.matcher.ElementMatchers.*. [AvoidStarImport]
Audit done.
[INFO] There is 1 error reported by Checkstyle 9.3 with /home/runner/work/skywalking-java/skywalking-java/apm-checkstyle/checkStyle.xml ruleset.
Error: src/main/java/org/apache/skywalking/apm/plugin/jdbc/PSSetterDefinitionOfJDBCInstrumentation.java:[27,52] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - net.bytebuddy.matcher.ElementMatchers.*.
```
It seems you don't check the code style locally.
And you should update the changelog according to the PR template.
--
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
[GitHub] [skywalking-java] sky-dawn commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "sky-dawn (via GitHub)" <gi...@apache.org>.
sky-dawn commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1185650654
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
Yes, it's ClassCastException. For detail:
`java.lang.ClassCastException: com.microsoft.sqlserver.jdbc.Parameter cannot be cast to java.lang.Integer`
It happens in this piece of code:
```
final void setObject(Parameter param, Object obj, JavaType javaType, JDBCType jdbcType, Integer scale,
Integer precision, boolean forceEncrypt, int parameterIndex, String tvpName) throws SQLServerException
```
a non-public method
Normally, java.sql.PreparedStatement, The first param type is int of this interface's setXX method. But mssql's underlying implementation is non-standard.
And JDBCPreparedStatementSetterInterceptor intercepts methods like:
"setDouble", "setFloat", "setInt", "setLong", "setNString", "setObject", "setAsciiStream", "setBinaryStream", "setBlob", "setBytes" ...
This methods just set params for preparedstatement. SW just intercept public methodsοΌnot non-public methods.
--
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
[GitHub] [skywalking-java] sky-dawn commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "sky-dawn (via GitHub)" <gi...@apache.org>.
sky-dawn commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1184833773
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
Do you mean don't change the interceptor code?
In general, non-public method is the underlying implementation, I think it need not to be intercept.
So if I can change the PSSetterDefinitionOfJDBCInstrumentation code,like this:
```
@Override
public ElementMatcher<MethodDescription> getMethodsMatcher() {
ElementMatcher.Junction<MethodDescription> matcher = none();
// remove TRACE_SQL_PARAMETERS judgement for dynamic config
final Set<String> setters = ignorable ? PS_IGNORABLE_SETTERS : PS_SETTERS;
for (String setter : setters) {
matcher = matcher.or(named(setter));
}
return matcher;
}
```
change `matcher = matcher.or(named(setter))` to `matcher = matcher.or(named(setter).and(isPublic()))`
--
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
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1184729617
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
If you need to change the intercepting points, please change the codes of matcher in the instrument class.
--
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
[GitHub] [skywalking-java] sky-dawn commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "sky-dawn (via GitHub)" <gi...@apache.org>.
sky-dawn commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1184841714
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
or just change the mssql plugin's Instrumentation. I only find this problem in the mssql's interface.
--
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
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #524: Fix ClassCastException when mssql inserting data (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1185828549
##########
CHANGES.md:
##########
@@ -10,6 +10,7 @@ Release Notes.
* Support clickhouse-jdbc-plugin trace sql parameters
* Support monitor jetty server work thread pool metric
* Support Jersey REST framework
+* Fix ClassCastException when SQLServer inserting data (#10741)
Review Comment:
```suggestion
* Fix ClassCastException when SQLServer inserting data
```
--
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
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1185664016
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
You could try to set it for public only, I think. As we should only care about those methods.
--
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
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #524: Fix ClassCastException when mssql inserting data (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1185828549
##########
CHANGES.md:
##########
@@ -10,6 +10,7 @@ Release Notes.
* Support clickhouse-jdbc-plugin trace sql parameters
* Support monitor jetty server work thread pool metric
* Support Jersey REST framework
+* Fix ClassCastException when SQLServer inserting data (#10741)
Review Comment:
```suggestion
* Fix ClassCastException when SQLServer inserts data
```
--
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
[GitHub] [skywalking-java] wu-sheng merged pull request #524: Fix ClassCastException when mssql inserting data (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #524:
URL: https://github.com/apache/skywalking-java/pull/524
--
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
[GitHub] [skywalking-java] sky-dawn commented on pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "sky-dawn (via GitHub)" <gi...@apache.org>.
sky-dawn commented on PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#issuecomment-1535827329
> ```
> [INFO] Starting audit...
> Error: /home/runner/work/skywalking-java/skywalking-java/apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/PSSetterDefinitionOfJDBCInstrumentation.java:27:52: Using the '.*' form of import should be avoided - net.bytebuddy.matcher.ElementMatchers.*. [AvoidStarImport]
> Audit done.
> [INFO] There is 1 error reported by Checkstyle 9.3 with /home/runner/work/skywalking-java/skywalking-java/apm-checkstyle/checkStyle.xml ruleset.
> Error: src/main/java/org/apache/skywalking/apm/plugin/jdbc/PSSetterDefinitionOfJDBCInstrumentation.java:[27,52] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - net.bytebuddy.matcher.ElementMatchers.*.
> ```
>
> It seems you don't check the code style locally. And you should update the changelog according to the PR template.
Sorry! Let me check 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #524: Fix SQLServer write data intercept failure (#10741)
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #524:
URL: https://github.com/apache/skywalking-java/pull/524#discussion_r1184954837
##########
apm-sniffer/apm-sdk-plugin/jdbc-commons/src/main/java/org/apache/skywalking/apm/plugin/jdbc/JDBCPreparedStatementSetterInterceptor.java:
##########
@@ -24,13 +24,14 @@
import org.apache.skywalking.apm.plugin.jdbc.define.StatementEnhanceInfos;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
public class JDBCPreparedStatementSetterInterceptor implements InstanceMethodsAroundInterceptor {
@Override
public final void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments,
Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
final StatementEnhanceInfos statementEnhanceInfos = (StatementEnhanceInfos) objInst.getSkyWalkingDynamicField();
- if (statementEnhanceInfos != null) {
+ if (statementEnhanceInfos != null && Modifier.isPublic(method.getModifiers())) {
Review Comment:
According to the error, it seems mssql JDBC driver has some special public method, which isn't expected to trace.
As a result, it ends in ClassCastException.
Is this the case?
Changing mssql plugins only work for me, and adding `isPublic` to filter unnecessary intercepting points make sense too if there is no side-effect for other drivers.
--
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