You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2021/09/30 07:28:18 UTC
[GitHub] [skywalking-java] wallezhang opened a new pull request #41: Feature add clickhouse jdbc plugin
wallezhang opened a new pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41
<!--
⚠️ 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]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
### Add an agent plugin to support <framework name>
- [x] 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 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 👆 ==== -->
- [x] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #7815.
- [x] 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] wallezhang commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725748496
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
> If you want to be strict, be strict at the match rule. Because `instanceof` has performance impact(even it is tiny in 1.8+).
Ok, I will move `instanceof` match to the instrumentation definition.
--
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] wallezhang commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725745470
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
> This seems to be always true, according to the method signature of the instrumented method
For current version, it is true. The judgement is make sure the application can run successfully after `clickhouse-jdbc-driver` has break changes or any incompatible changes in the future.
--
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] wallezhang commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r724652800
##########
File path: test/plugin/scenarios/clickhouse-0.3.x-scenario/support-version.list
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+0.3.0
+0.3.1-patch
Review comment:
I modified `0.3.1-patch` to `0.3.1`
--
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] wallezhang edited a comment on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang edited a comment on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-939256807
I created a simple SpringBoot application, and expose an endpoint named `/hello/clickhouse`. This endpoint will get a connection from the pool and execute a `SELECT` sql. These are the screenshots:
topology(ClickHouse logo is not included in the main branch, so topology graph shows the default logo)
database dashboard
apm dashboard
trace
--
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] wallezhang commented on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-930948729
Hi @wu-sheng ,
Because the java agent is separated to a independent repository. Should I open another PR in the main repository to add a component id to oap-server?
--
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 change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725746259
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
If you want to be strict, be strict at the match rule. Because `instanceof` has performance impact(even it is tiny in 1.8+).
--
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] wallezhang commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725750906
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
> @wallezhang pull the condition to the instrumentation point (instrument a more specific method with argument types) might be more helpful and performant.
Already done. 😄
--
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] wallezhang commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r724652420
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/pom.xml
##########
@@ -0,0 +1,62 @@
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ 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>apm-sdk-plugin</artifactId>
+ <groupId>org.apache.skywalking</groupId>
+ <version>8.8.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>apm-clickhouse-0.3.x-plugin</artifactId>
+ <packaging>jar</packaging>
+
+ <name>clickhouse-0.3.x-plugin</name>
+ <url>http://maven.apache.org</url>
Review comment:
Done. I have removed 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] wallezhang commented on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-939631014
> It seems you break something, maybe code style?
I'm checking. Sorry.
--
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] kezhenxu94 commented on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-931027237
> Hi @wu-sheng ,
> Because the java agent is separated to a independent repository. Should I open another PR in the main repository to add a component id to oap-server?
Yes, please do that
--
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] kezhenxu94 commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r719178737
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/pom.xml
##########
@@ -0,0 +1,62 @@
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one or more
+ ~ contributor license agreements. See the NOTICE file distributed with
+ ~ this work for additional information regarding copyright ownership.
+ ~ The ASF licenses this file to You under the Apache License, Version 2.0
+ ~ (the "License"); you may not use this file except in compliance with
+ ~ the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing, software
+ ~ distributed under the License is distributed on an "AS IS" BASIS,
+ ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ ~ See the License for the specific language governing permissions and
+ ~ limitations under the License.
+ ~
+ -->
+
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+ 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>apm-sdk-plugin</artifactId>
+ <groupId>org.apache.skywalking</groupId>
+ <version>8.8.0-SNAPSHOT</version>
+ </parent>
+ <modelVersion>4.0.0</modelVersion>
+
+ <artifactId>apm-clickhouse-0.3.x-plugin</artifactId>
+ <packaging>jar</packaging>
+
+ <name>clickhouse-0.3.x-plugin</name>
+ <url>http://maven.apache.org</url>
Review comment:
Remove this, or at least replace with http://skywalking.apache.org
##########
File path: test/plugin/scenarios/clickhouse-0.3.x-scenario/support-version.list
##########
@@ -0,0 +1,18 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+0.3.0
+0.3.1-patch
Review comment:
For patching version, if they are not related to our implementation, I think we can simply ignore 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] kezhenxu94 commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725748503
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
@wallezhang pull the condition to the instrumentation point (instrument a more specific method with argument types) might be more helpful and performant.
--
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] wallezhang commented on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-939256807
I created a simple SpringBoot application, and expose an endpoint named `/hello/clickhouse`. This endpoint will get a connection from the pool and execute a `SELECT` sql. These are the screenshots:
topology
database dashboard
apm dashboard
trace
--
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 #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41
--
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] wallezhang commented on pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wallezhang commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-939249892
> Could you providr screenshots of topology, trace, and database dashboards to demonstrate this works well with backend?
Of course. I will submit screenshots later.
--
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 #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#issuecomment-939630735
It seems you break something, maybe code style?
--
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] kezhenxu94 commented on a change in pull request #41: Feature add clickhouse jdbc plugin
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #41:
URL: https://github.com/apache/skywalking-java/pull/41#discussion_r725646353
##########
File path: apm-sniffer/apm-sdk-plugin/clickhouse-0.3.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jdbc/clickhouse/InitConnectionMethodInterceptor.java
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ */
+
+package org.apache.skywalking.apm.plugin.jdbc.clickhouse;
+
+import java.lang.reflect.Method;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+import org.apache.skywalking.apm.network.trace.component.ComponentsDefine;
+import org.apache.skywalking.apm.plugin.jdbc.trace.ConnectionInfo;
+import ru.yandex.clickhouse.settings.ClickHouseProperties;
+
+/**
+ * Enhance {@link ru.yandex.clickhouse.ClickHouseConnectionImpl#initConnection(ClickHouseProperties)} method.
+ * <p>
+ * ClickHouse JDBC Driver uses this method to simulate the action of connecting database in JDBC protocol.
+ * So this method is enhanced to prevent the generation of exit span of http type.
+ * </p>
+ * <p>
+ * This interceptor is used to replace {@link org.apache.skywalking.apm.plugin.jdbc.JDBCDriverInterceptor}.
+ * </p>
+ */
+public class InitConnectionMethodInterceptor implements InstanceMethodsAroundInterceptor {
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes,
+ MethodInterceptResult result) throws Throwable {
+ final Object properties = allArguments[0];
+ if (properties instanceof ClickHouseProperties) {
Review comment:
This seems to be always true, according to the method signature of the instrumented method
--
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