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 2020/03/04 05:53:42 UTC
[GitHub] [skywalking] huangyoje opened a new pull request #4441: Add finagle
plugin (#4433)
huangyoje opened a new pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441
Please answer these questions before submitting pull request
- Why submit this pull request?
- [ ] Bug fix
- [x] New feature provided
- [ ] Improve performance
- Related issues
[https://github.com/apache/skywalking/issues/4433](https://github.com/apache/skywalking/issues/4433)
___
### Bug fix
- Bug description.
- How to fix?
___
### New feature or improvement
- Add finagle plugin, version from 6.5.0 to 20.1.0
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388193632
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
These methods are overridden by methods in subsequent integrations. Recommend to modify these methods to abstract methods, so that the logic may be clearer. construct like this `abstract void onConstruct(EnhancedInstance objInst, Object[] allArguments, AbstractSpan span)`. Or they can be split into two classes like `InstanceConstructorInterceptor InstanceMethodsAroundInterceptor` ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388352300
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
Let's leave this here. My review has been challenging deeper things than this code style discussion. Please follow those first, then we could do the review again.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388350352
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextHolderFactory.java
##########
@@ -0,0 +1,308 @@
+/*
+ * 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.finagle;
+
+import com.twitter.finagle.context.Context;
+import com.twitter.finagle.context.Contexts;
+import com.twitter.finagle.context.LocalContext;
+import com.twitter.finagle.context.MarshalledContext;
+import com.twitter.io.Buf;
+import com.twitter.util.Local;
+import scala.Option;
+import scala.Predef;
+import scala.Some;
+import scala.Some$;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.JavaConverters$;
+
+import javax.annotation.Nullable;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+
+public class ContextHolderFactory {
Review comment:
Read my comments about witness class, and remove this.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-594581223
> 2. add ICON to SkyWalking-UI repo
Hi, I think i need some help. After checking the skywalking-ui directory, I could't find where the icon should be added? Also are there any requirements for icons, such as size.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389328863
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/scala/org/apache/skywalking/apm/plugin/finagle/SWContextCarrier.scala
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.finagle
+
+import com.twitter.finagle.context.Contexts
+import com.twitter.io.Buf
+import com.twitter.util.{Return, Try}
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier
+
+class SWContextCarrier(val carrier: ContextCarrier) {
Review comment:
`SWContextCarrier ` implement `MarshalledContext.Key`, which is a scala inner class, I don't find a right way to do this in java.
```java
class Impl1 extends MarshalledContext.Key<String> {
public Impl1() {
Contexts.broadcast().super("impl1");
}
}
```
javac throws a compile error for above codes. However, when i fix the compile error, there will be a runtime error. Java and scala's inner classes are slightly different[https://stackoverflow.com/a/39692316/7645128](https://stackoverflow.com/a/39692316/7645128), may be the error here comes from difference of javac and scalac.
I am still looking for a right way to do this in java,
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr edited a comment on issue #4441: Add finagle
plugin (#4433)
Posted by GitBox <gi...@apache.org>.
dmsolr edited a comment on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-594352263
> Please follow these four first
>
> 1. Make codes well formatted, including comments of the codes
> 2. Add unit tests to your plugin codes.
> 3. Make CI and plugin integration test work
> 4. supported doc update.
Additional:
1. add it to component-libraries.yaml (have 2 files, missing one)
2. add ICON to SkyWalking-UI repo
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596161248
> As #4458 merged, the expected YAML file has fields been renamed. Take a look the PR and updated plugin test doc.
OK.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388193632
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
These methods are overridden by methods in subsequent integrations. Recommend to modify these methods to abstract methods, so that the logic may be clearer. Or they can be split into two classes?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-595251908
> Open the actions tab, https://github.com/apache/skywalking/pull/4441/checks. Open the PluginsTest tab on the left side, then you could check every sub group execution time. Or check the list above
I see. thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596154880
As #4458 merged, the expected YAML file has fields been renamed. Take a look the PR and updated plugin test doc.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597456616
@wu-sheng OK.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389245031
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextHolder.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.finagle;
+
+import javax.annotation.Nullable;
+
+/**
+ * @see com.twitter.finagle.context.Context
Review comment:
I have removed this.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389249047
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+/**
+ * The implementation detail of this plugin depend on a private class of Finagle Framework, we can not ensure that
+ * class will still exists in future versions. The mechanism of witness class can ensure this plugin can work in
+ * existing versions, it can not ensure this plugin will still work in future versions. So the purpose of the class
+ * is to check whether this plugin is compatible with future versions, if it does't, the plugin just do nothing,
+ * avoiding unexpected runtime exceptions.
+ */
+public class CompatibilityChecker {
+
+ static ILog LOGGER = LogManager.getLogger(CompatibilityChecker.class);
+
+ private static boolean COMPATIBILITY = false;
+
+ static {
+ try {
+ if (FinagleCtxs.RPC != null
+ && FinagleCtxs.SW_SPAN != null
+ && checkContextHolder()) {
+ COMPATIBILITY = true;
Review comment:
Sorry, I misunderstanding your point, I will remove this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596212276
Thanks for the update. If all these provided in next week, we could add this as a part of 7.0.0 release.
> Looks like it caused by scala codes. First, I will try to replace these codes with java, if not, I will try to find the compatible version for related components.
`appveyor.yml` is controlling the CI process in the `appveyor`. We used to use appveyor for Windows compiling, but this has been rechecked in GitHub action too, here is the control file, https://github.com/apache/skywalking/blob/master/.github/workflows/ci-it.yaml#L53-L72.
I don't know what is the difference.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388281947
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
Actually, in java, once you have set `AbstractInterceptor` as abstract, all these empty methods could be deleted directly :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389246243
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextHolderFactory.java
##########
@@ -0,0 +1,308 @@
+/*
+ * 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.finagle;
+
+import com.twitter.finagle.context.Context;
+import com.twitter.finagle.context.Contexts;
+import com.twitter.finagle.context.LocalContext;
+import com.twitter.finagle.context.MarshalledContext;
+import com.twitter.io.Buf;
+import com.twitter.util.Local;
+import scala.Option;
+import scala.Predef;
+import scala.Some;
+import scala.Some$;
+import scala.Tuple2;
+import scala.collection.JavaConverters;
+import scala.collection.JavaConverters$;
+
+import javax.annotation.Nullable;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+
+public class ContextHolderFactory {
Review comment:
Comments has been added, please check `ContextHolder, ContextHolderFactory`, the class is necessary for the current implementation.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388200652
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientDestTracingFilterInterceptor.java
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.finagle;
+
+import com.twitter.finagle.Address;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.net.InetSocketAddress;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextCarrierHelper.setPeerHost;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+public class ClientDestTracingFilterInterceptor extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ enhancedInstance.setSkyWalkingDynamicField(getRemote(objects));
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, MethodInterceptResult methodInterceptResult) throws Throwable {
+ String peer = (String) enhancedInstance.getSkyWalkingDynamicField();
+ AbstractSpan span = getSpan();
+ if (span != null) {
+ span.setPeer(peer);
+ }
+ SWContextCarrier swContextCarrier = getContextCarrier();
+ if (swContextCarrier != null) {
+ setPeerHost(swContextCarrier.carrier(), peer);
+ }
+ }
+
+ @Override
+ public Object afterMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Object o) throws Throwable {
+ return o;
+ }
+
+ @Override
+ public void handleMethodExceptionImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Throwable throwable) {
+
Review comment:
Exception information to be added like `ContextManager.activeSpan().errorOccurred().log(t)`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] dmsolr commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
dmsolr commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-594352263
> Please follow these four first
>
> 1. Make codes well formatted, including comments of the codes
> 2. Add unit tests to your plugin codes.
> 3. Make CI and plugin integration test work
> 4. supported doc update.
Additional:
1. add it to component-libraries.yaml
2. add ICON to SkyWalking-UI repo
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-594344519
> Please follow these four first
>
> 1. Make codes well formatted, including comments of the codes
> 2. Add unit tests to your plugin codes.
> 3. Make CI and plugin integration test work
> 4. supported doc update.
OK! I will follow these steps to improve.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389341840
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
Review comment:
Take a look at #4462
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596075917
Have you followed no threadlocal and using the witness class? That should be followed.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597457632
@huangyoje Ping you at the PR.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-595244531
> After check the git history of the file, I find the last merged plugin is avro, does the fast group is Spring31x_ES5_Gateway_Avro_Struts25? Please tolerate me with this kind of questions, I did not use github actions before.
Open the actions tab, https://github.com/apache/skywalking/pull/4441/checks. Open the PluginsTest tab on the left side, then you could check every sub group execution time. Or check the list above
That is easy.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597535162
> Merging. Thank you for your contributions.
πΊπΊ
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389246738
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
First, That thing liked I said, having IP issue. And same as I said, you shouldn't implement in this way.
I think I have said clear. Any question?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389329984
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
Review comment:
> Is the enhancedInstance used in only one thread only?
The `enhancedInstance ` is used in mulit threads, I will remove usage of it.
> I am confused here, if the dynamic field is safe enough, why you need the ContextHolder?
`ContextHolder` is used to put `finagleSpan`,`SWContextCarrier` to Context, thus these two can be accessed by `ClientDestTracingFilterInterceptor`, even if `ClientDestTracingFilter` is executed in another thread.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388349955
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextHolder.java
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.finagle;
+
+import javax.annotation.Nullable;
+
+/**
+ * @see com.twitter.finagle.context.Context
Review comment:
This is not allowed, if it is copied from somewhere else, you are facing source code level LICENSE update. Also, as I commented above, I prefer to remove this.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389370593
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextCarrierHelper.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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.trace.ExitSpan;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.RPC;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getOperationName;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getPeerHost;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * We need set peer host to {@link ContextCarrier} in {@link ClientDestTracingFilterInterceptor}, but there is no
+ * public method to do this, so we use this helper to achieve it.
+ */
+class ContextCarrierHelper {
+
+ static void tryInjectContext() {
+ String operationName = getOperationName();
+ if (operationName == null) {
+ return;
+ }
+ String peer = getPeerHost();
+ if (peer == null) {
+ return;
+ }
Review comment:
This format seems wrong.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389328512
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
Review comment:
Why do you plan to propagate the op name in the wire? Entry span should focus on the server side logic only. The only thing will propagate in your RPC header, is the official ContextCarrier.
Like my other comments say, I have concern about you are hijacking the ContextCarrier. If we change it some day, even it is rarely to see, it is hard to remember the Finagle plugin has this operation.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389263366
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
You also hijacking the op name in the ContextCarrier. Those two are uncommon case for me. Are these really necessary?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389325508
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
+ getLocalContextHolder().let(FinagleCtxs.RPC, rpc);
+ } else {
+ span.setOperationName(rpc);
+ }
+ SWContextCarrier swContextCarrier = getContextCarrier();
+ if (swContextCarrier != null) {
+ swContextCarrier.setOperationName(rpc);
+ }
+ }
+ }
+
+ @Override
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ @Override
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ ContextManager.activeSpan().errorOccurred().log(t);
Review comment:
I will remove this. The `AnnotationInterceptor` only needs to implement `InstanceConstructorInterceptor`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388347770
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
Version compatibility feature is provided by a thing called `witness` class. If you provide that in the instrumentation definition, your plugin is activated only those class exists.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388244559
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
I will make these methods abstract.
Because `ClientDestTracingFilterInterceptor` need to implement both `onConstructImpl ` and `beforeMethodImpl ` methods and java did not support mutiple inheritance, so they can not be split into two 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng merged pull request #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389251592
##########
File path: .github/workflows/plugins-test.yaml
##########
@@ -271,6 +271,12 @@ jobs:
run: bash test/plugin/run.sh undertow-scenario
- name: Run jedis 2.4.0-2.9.0 (18)
run: bash test/plugin/run.sh jedis-scenario
+ - name: Run finagle 6.25.0-6.43.0
+ run: bash test/plugin/run.sh finagle-6.25.x-scenario
+ - name: Run finagle 6.44.0-7.1.0
+ run: bash test/plugin/run.sh finagle-6.44.x-scenario
+ - name: Run finagle 17.10.0-20.1.0
+ run: bash test/plugin/run.sh finagle-17.10.x-scenario
Review comment:
I just look your test cases, there are 3 folders but the codes are sharing or same. You should merge them into one case with version from 6.25-20.1. Because the only reason we separate the case, because the library APIs changed from version to version. But for finagle, I think it isn't. Am I right?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389263432
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
+ getLocalContextHolder().let(FinagleCtxs.RPC, rpc);
+ } else {
+ span.setOperationName(rpc);
+ }
+ SWContextCarrier swContextCarrier = getContextCarrier();
+ if (swContextCarrier != null) {
+ swContextCarrier.setOperationName(rpc);
+ }
+ }
+ }
+
+ @Override
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ @Override
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ @Override
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ ContextManager.activeSpan().errorOccurred().log(t);
Review comment:
Once this interceptor is not in the user thread, there is no active span.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388343122
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
These methods we discussed above are`onConstructImpl`,`beforeMethodImpl `, they are not inherited from interface. The purpose of `AbstractInterceptor ` is a adapter for `InstanceConstructorInterceptor , InstanceMethodsAroundInterceptor `.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389256712
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
Why don't you create the exit span in the `ClientDestTracingFilterInterceptor` directly? ContextCarrier require the peer information, otherwise the topology will break.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596073964
The comments have been added. The implementation of this plugin is described in the `AbstractAbstractceptor` class, and other classes have added their own comments. I hope these comments can be described clearly, if there is any ambiguity, please point out, i will modify again.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389333225
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
Review comment:
> Why do you plan to propagate the op name in the wire?
For finagle version 17.12.0 and above, we can not get op name in the server side directly. If we add `finagle-{protocol}(eg: finagle-thrift)` into this plugin, then we could deserialize the bytes of the request paramenter and get op name, but this will introduce more complexity becase we have to be compatible with various protocols.
> Like my other comments say, I have concern about you are hijacking the ContextCarrier. If we change it some day, even it is rarely to see, it is hard to remember the Finagle plugin has this operation.
So far, I can't think of a better way to do this. When we create span and contextCarrier, we don't know the remote address and op name, but it is difficult to propagate trace information across finagle's inner threads and then create span untill we know remote address and op name.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-595299435
> The comments of your codes are required. All the review are based on my guess, please don't let me do that.
> Comments are important for other contributors and reviewers.
You are right, sorry for that. I will add comments this weekend.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596164519
> I noticed, this PR fails every time on AppVeyor. Strange.
I will check this issue.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388348981
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
The ThreadLocal has low performance in the agent, and cost more CPU resources, comparing to other ways. Your codes use this in high frequency which is a potential performance issue. We only accept one way to use thread local, is there is no way to communicate between the interceptor. From your class name, this is not for that case.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389263814
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/scala/org/apache/skywalking/apm/plugin/finagle/SWContextCarrier.scala
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.finagle
+
+import com.twitter.finagle.context.Contexts
+import com.twitter.io.Buf
+import com.twitter.util.{Return, Try}
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier
+
+class SWContextCarrier(val carrier: ContextCarrier) {
Review comment:
Why this one in Scala? Could you explain the reason? I can't see that through codes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389247045
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
The future versions are never being sure. So it is meaningless. That is why we have explicit version test CI process, and indicate them in support list doc. Only real test could verify 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389263554
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
Review comment:
What does this `in case` mean? Why there is unexpected execution sequence?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389245836
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
> Version compatibility feature is provided by a thing called `witness` class. If you provide that in the instrumentation definition, your plugin is activated only those class exists.
The implementation detail of this plugin depend on a private class of Finagle Framework, we can not ensure that class will still exists in future versions. The mechanism of witness class can ensure this plugin can work in existing versions, it can not ensure this plugin will still work in future versions. So the purpose of the class is to check whether this plugin is compatible with future versions, if it does't, the plugin just do nothing, avoiding unexpected runtime exceptions.
This has been added in comments.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597455359
@huangyoje I think this is my fault. I was not expecting the tracing context could be finished in the main thread when we do lazy injection. I will submit a fix PR soon.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388193632
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
These methods are overridden by methods in subsequent integrations. Recommend to modify these methods to abstract methods, so that the logic may be clearer. construct like this `protected void onConstruct(EnhancedInstance objInst, Object[] allArguments, AbstractSpan span)`. Or they can be split into two classes like `InstanceConstructorInterceptor InstanceMethodsAroundInterceptor` ?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388193632
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AbstractInterceptor.java
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceConstructorInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+abstract class AbstractInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor {
+ @Override
+ public void onConstruct(EnhancedInstance objInst, Object[] allArguments) {
+ if (CompatibilityChecker.isCompatible()) {
+ onConstructImpl(objInst, allArguments);
+ }
+ }
+
+ @Override
+ public void beforeMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ beforeMethodImpl(objInst, method, allArguments, argumentsTypes, result);
+ }
+ }
+
+ @Override
+ public Object afterMethod(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ if (CompatibilityChecker.isCompatible()) {
+ return afterMethodImpl(objInst, method, allArguments, argumentsTypes, ret);
+ }
+ return ret;
+ }
+
+ @Override
+ public void handleMethodException(EnhancedInstance objInst, Method method, Object[] allArguments, Class<?>[] argumentsTypes, Throwable t) {
+ if (CompatibilityChecker.isCompatible()) {
+ handleMethodExceptionImpl(objInst, method, allArguments, argumentsTypes, t);
+ }
+ }
+
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ protected void beforeMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, MethodInterceptResult result) throws Throwable {
+
+ }
+
+ protected Object afterMethodImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Object ret) throws Throwable {
+ return ret;
+ }
+
+ protected void handleMethodExceptionImpl(EnhancedInstance objInst, Method method, Object[] allArguments,
+ Class<?>[] argumentsTypes, Throwable t) {
+
+ }
Review comment:
These methods are overridden by methods in subsequent integrations. Recommend to modify these methods to abstract methods, so that the logic may be clearer. construct like this `protected void onConstruct(EnhancedInstance objInst, Object[] allArguments, AbstractSpan span)` . they can be split into two classes?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389371660
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ContextCarrierHelper.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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.trace.ExitSpan;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.RPC;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getOperationName;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getPeerHost;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * We need set peer host to {@link ContextCarrier} in {@link ClientDestTracingFilterInterceptor}, but there is no
+ * public method to do this, so we use this helper to achieve it.
+ */
+class ContextCarrierHelper {
+
+ static void tryInjectContext() {
+ String operationName = getOperationName();
+ if (operationName == null) {
+ return;
+ }
+ String peer = getPeerHost();
+ if (peer == null) {
+ return;
+ }
Review comment:
Fixed.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597446020
Merging. Thank you for your contributions.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389260826
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
The peer infomation is added to ContextCarrier in `ClientDestTracingFilterInterceptor` by `ContextCarrierHelper`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389327492
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/AnnotationInterceptor.java
##########
@@ -0,0 +1,87 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+/**
+ * Finagle use Annotation to represent data that tracing system interested, usually these annotations are created by
+ * filters after ClientTracingFilter in the rpc call stack. We can intercept annotations that we interested.
+ */
+public class AnnotationInterceptor {
+
+ abstract static class Abstract extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ onConstruct(enhancedInstance, objects, getSpan());
+ }
+
+ protected abstract void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span);
+ }
+
+ /**
+ * When we create exitspan in ClientTracingFilter, we can't know the operation name, however the Rpc annotation
+ * contains the operation name we need, so we intercept the constructor of this Annotation and set operation name
+ * to exitspan.
+ */
+ public static class Rpc extends Abstract {
+
+ @Override
+ protected void onConstruct(EnhancedInstance enhancedInstance, Object[] objects, AbstractSpan span) {
+ if (objects != null && objects.length == 1) {
+ String rpc = (String) objects[0];
+ if (span == null) {
+ // in case the exitspan is created later
Review comment:
Iniitally, here is used to set op to server entryspan(sorry for the wrong comments), and finally, I passed op from client to server, so it is no longer needed here. The codes and comments has been modified as below:
```java
if (span != null) {
/*
* The Rpc Annotation is created both in client side and server side, in server side, this
* annotation is created only in finagle versions below 17.12.0.
*
* If the span is not null, which means we are in the client side, we just set op to the exitspan.
*
* If the span is null, which means we are in the server side with finagle version below 17.12.0.
* In server side, we don't need this annotation, because we can get op from Contexts.broadcast
* which comes from client.
*/
span.setOperationName(rpc);
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389245836
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
> Version compatibility feature is provided by a thing called `witness` class. If you provide that in the instrumentation definition, your plugin is activated only those class exists.
The implementation detail of this plugin depend on a private class of Finagle Framework, we can not ensure that class will still exists in future versions. The mechanism of witness class can ensure this plugin can work in existing versions, it can not ensure this plugin will still work in future versions. So the purpose of the class is to check whether this plugin is compatible with future versions, if it does't, the plugin just do nothing, avoiding unexpected runtime exceptions.
This has been added in comments.
`witness` has been used in this plugin to ignore versions below 6.25.0.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389247303
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+/**
+ * The implementation detail of this plugin depend on a private class of Finagle Framework, we can not ensure that
+ * class will still exists in future versions. The mechanism of witness class can ensure this plugin can work in
+ * existing versions, it can not ensure this plugin will still work in future versions. So the purpose of the class
+ * is to check whether this plugin is compatible with future versions, if it does't, the plugin just do nothing,
+ * avoiding unexpected runtime exceptions.
+ */
+public class CompatibilityChecker {
+
+ static ILog LOGGER = LogManager.getLogger(CompatibilityChecker.class);
+
+ private static boolean COMPATIBILITY = false;
+
+ static {
+ try {
+ if (FinagleCtxs.RPC != null
+ && FinagleCtxs.SW_SPAN != null
+ && checkContextHolder()) {
+ COMPATIBILITY = true;
Review comment:
You are using a Thread local to check compatibility, why you think it makes sense?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596163647
I noticed, this PR fails every time on AppVeyor. Strange.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389255176
##########
File path: .github/workflows/plugins-test.yaml
##########
@@ -271,6 +271,12 @@ jobs:
run: bash test/plugin/run.sh undertow-scenario
- name: Run jedis 2.4.0-2.9.0 (18)
run: bash test/plugin/run.sh jedis-scenario
+ - name: Run finagle 6.25.0-6.43.0
+ run: bash test/plugin/run.sh finagle-6.25.x-scenario
+ - name: Run finagle 6.44.0-7.1.0
+ run: bash test/plugin/run.sh finagle-6.44.x-scenario
+ - name: Run finagle 17.10.0-20.1.0
+ run: bash test/plugin/run.sh finagle-17.10.x-scenario
Review comment:
OK. Let's leave the cases like this for now. Please continue on other changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596210746
> Most concerns have been eased. Just `CodecUtils` and `ContextHolderFactory` may leak comments.
>
> To be honest, I can't get too much details of this plugin, as I am not familiar with the Finagle :). All these requirements are making another contributor or further bug fix one could understand your logic. Feel free to add a `.md` in your plugin rood folder to do better explanation.
I will take your suggesiton, and to summarize what i will do next:
1. Fix `ContextHolderFactory`
For the current implentation of `ContextHolderFactory`, if server calls another rpc, for example A ->B(serverB) ->C(serverC), there will have context pollution in serverB. I will test this and fix.
2. Fix the failure of appveyor
Looks likedto it caused by scala codes. First, I will try to replace these codes with java, if not, I will try to find the compatible version for related components.
3. Improve comments.
4. Add a `implementation.md` in root folder
Hope to finish above in next week!
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389264350
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
Review comment:
I am confused here, if the `dynamic field` is safe enough, why you need the `ContextHolder`?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389260458
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
Because `ClientDestTracingFilter ` may be executed in another thread, not the thread that initiated the rpc request. It is hard to know when and where the thread change occurs.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389245836
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
> Version compatibility feature is provided by a thing called `witness` class. If you provide that in the instrumentation definition, your plugin is activated only those class exists.
The implementation detail of this plugin depend on a private class of Finagle Framework, we can not ensure that class will still exists in future versions. The mechanism of witness class can ensure this plugin can work in existing versions, it can not ensure this plugin will still work in future versions. So the purpose of the class is to check whether this plugin is compatible with future versions, if it does't, the plugin just do nothing, avoiding unexpected runtime exceptions.
This has been added in comments.
`witness` has been used in this plugin to ignore version below 6.25.0.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388246962
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientDestTracingFilterInterceptor.java
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.finagle;
+
+import com.twitter.finagle.Address;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+import java.net.InetSocketAddress;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextCarrierHelper.setPeerHost;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getContextCarrier;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.getSpan;
+
+public class ClientDestTracingFilterInterceptor extends AbstractInterceptor {
+
+ @Override
+ public void onConstructImpl(EnhancedInstance enhancedInstance, Object[] objects) {
+ enhancedInstance.setSkyWalkingDynamicField(getRemote(objects));
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, MethodInterceptResult methodInterceptResult) throws Throwable {
+ String peer = (String) enhancedInstance.getSkyWalkingDynamicField();
+ AbstractSpan span = getSpan();
+ if (span != null) {
+ span.setPeer(peer);
+ }
+ SWContextCarrier swContextCarrier = getContextCarrier();
+ if (swContextCarrier != null) {
+ setPeerHost(swContextCarrier.carrier(), peer);
+ }
+ }
+
+ @Override
+ public Object afterMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Object o) throws Throwable {
+ return o;
+ }
+
+ @Override
+ public void handleMethodExceptionImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Throwable throwable) {
+
Review comment:
Sure, I will add 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389329984
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
Review comment:
> Is the enhancedInstance used in only one thread only?
The `enhancedInstance ` is used in mulit threads, I will remove usage of it.
> I am confused here, if the dynamic field is safe enough, why you need the ContextHolder?
`ContextHolder` is used to put `finagleSpan`,`SWContextCarrier` to Context, thus these two can be accessed by `ClientDestTracingFilterInterceptor`, even if `ClientDestTracingFilter` is executed in another thread.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] aderm commented on a change in pull request #4441: Add
finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
aderm commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388206943
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
+ }
+
+ @Override
+ public Object afterMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Object ret) throws Throwable {
+ CacheObjects cacheObjects = (CacheObjects) enhancedInstance.getSkyWalkingDynamicField();
+
+ final AbstractSpan finagleSpan = cacheObjects.localContextHolder.remove(SW_SPAN);
+ cacheObjects.marshlledContextHolder.remove(SWContextCarrier$.MODULE$);
+
+ finagleSpan.prepareForAsync();
+ ContextManager.stopSpan(finagleSpan);
+
+ ((Future<?>) ret).addEventListener(new FutureEventListener<Object>() {
+ @Override
+ public void onSuccess(Object value) {
+ finagleSpan.asyncFinish();
+ }
+
+ @Override
+ public void onFailure(Throwable cause) {
+ finagleSpan.errorOccurred();
+// finagleSpan.log(cause);
Review comment:
why comment this?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388250217
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
+ }
+
+ @Override
+ public Object afterMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Object ret) throws Throwable {
+ CacheObjects cacheObjects = (CacheObjects) enhancedInstance.getSkyWalkingDynamicField();
+
+ final AbstractSpan finagleSpan = cacheObjects.localContextHolder.remove(SW_SPAN);
+ cacheObjects.marshlledContextHolder.remove(SWContextCarrier$.MODULE$);
+
+ finagleSpan.prepareForAsync();
+ ContextManager.stopSpan(finagleSpan);
+
+ ((Future<?>) ret).addEventListener(new FutureEventListener<Object>() {
+ @Override
+ public void onSuccess(Object value) {
+ finagleSpan.asyncFinish();
+ }
+
+ @Override
+ public void onFailure(Throwable cause) {
+ finagleSpan.errorOccurred();
+// finagleSpan.log(cause);
Review comment:
```java
public ExitSpan log(Throwable t) {
if (stackDepth == 1) {
super.log(t);
}
return this;
}
```
For ExitSpan, the log will only works when stackDepth=1. I don't understand why, Does it ok to invoke log method here?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389264270
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
Review comment:
Is the `enhancedInstance` used in only one thread only? If there is a race condition, you can't use the dynamic field.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597397603
@aderm @arugal @dmsolr @kezhenxu94 Any of you could recheck?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin (#4433)
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#discussion_r388345062
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,97 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
+ }
+
+ @Override
+ public Object afterMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes, Object ret) throws Throwable {
+ CacheObjects cacheObjects = (CacheObjects) enhancedInstance.getSkyWalkingDynamicField();
+
+ final AbstractSpan finagleSpan = cacheObjects.localContextHolder.remove(SW_SPAN);
+ cacheObjects.marshlledContextHolder.remove(SWContextCarrier$.MODULE$);
+
+ finagleSpan.prepareForAsync();
+ ContextManager.stopSpan(finagleSpan);
+
+ ((Future<?>) ret).addEventListener(new FutureEventListener<Object>() {
+ @Override
+ public void onSuccess(Object value) {
+ finagleSpan.asyncFinish();
+ }
+
+ @Override
+ public void onFailure(Throwable cause) {
+ finagleSpan.errorOccurred();
+// finagleSpan.log(cause);
Review comment:
> For ExitSpan, the log will only works when stackDepth=1. I don't understand why, Does it ok to invoke log method here?
Why your plugin works as depth > 1? And yes, this is designed in that way, meaning, if there is a client-side RPC works, the nested one wouldn't be captured.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
(#4433)
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-595240890
> 2. You need to add your test into the CI pass by changing this file, https://github.com/apache/skywalking/blob/master/.github/workflows/plugins-test.yaml . Please use the fast group.
After check the git history of the file, I find the last merged plugin is avro, does the fast group is Spring31x_ES5_Gateway_Avro_Struts25? Please tolerate me with this kind of questions, I did not use github actions before.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389247585
##########
File path: .github/workflows/plugins-test.yaml
##########
@@ -271,6 +271,12 @@ jobs:
run: bash test/plugin/run.sh undertow-scenario
- name: Run jedis 2.4.0-2.9.0 (18)
run: bash test/plugin/run.sh jedis-scenario
+ - name: Run finagle 6.25.0-6.43.0
+ run: bash test/plugin/run.sh finagle-6.25.x-scenario
+ - name: Run finagle 6.44.0-7.1.0
+ run: bash test/plugin/run.sh finagle-6.44.x-scenario
+ - name: Run finagle 17.10.0-20.1.0
+ run: bash test/plugin/run.sh finagle-17.10.x-scenario
Review comment:
I am feeling you are not using the witness mechanism properly.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-597453559
I'm doing additional tests for scenario: A -> ServerB -> ServerC and meet above exception. I haven't had time to find out why, it better to revert this pr first.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596076469
> Have you followed no threadlocal and using the witness class? That should be followed.
https://github.com/apache/skywalking/pull/4441#discussion_r389245836
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on issue #4441: Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on issue #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-596097518
> I think you update the query-protocol submodule by a mistake.
This has been corrected.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389329322
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/scala/org/apache/skywalking/apm/plugin/finagle/SWContextCarrier.scala
##########
@@ -0,0 +1,50 @@
+/*
+ * 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.finagle
+
+import com.twitter.finagle.context.Contexts
+import com.twitter.io.Buf
+import com.twitter.util.{Return, Try}
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier
+
+class SWContextCarrier(val carrier: ContextCarrier) {
Review comment:
OK, if this is required in the original codes, we could keep it. This is not the issue.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io commented on issue #4441: Add finagle
plugin (#4433)
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4441: Add finagle plugin (#4433)
URL: https://github.com/apache/skywalking/pull/4441#issuecomment-595307764
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=h1) Report
> Merging [#4441](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/0df2d0a5e9210e0c2a6c4f7e5fe350d3aa21cab0?src=pr&el=desc) will **increase** coverage by `0.39%`.
> The diff coverage is `62.11%`.
[](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4441 +/- ##
==========================================
+ Coverage 25.14% 25.53% +0.39%
==========================================
Files 1223 1235 +12
Lines 28293 28615 +322
Branches 3890 3926 +36
==========================================
+ Hits 7115 7308 +193
- Misses 20521 20631 +110
- Partials 657 676 +19
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [.../define/ClientDestTracingFilterInstrumetation.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9kZWZpbmUvQ2xpZW50RGVzdFRyYWNpbmdGaWx0ZXJJbnN0cnVtZXRhdGlvbi5qYXZh) | `0% <0%> (ΓΈ)` | |
| [.../apm/network/trace/component/ComponentsDefine.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXByb3RvY29sL2FwbS1uZXR3b3JrL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9uZXR3b3JrL3RyYWNlL2NvbXBvbmVudC9Db21wb25lbnRzRGVmaW5lLmphdmE=) | `0% <0%> (ΓΈ)` | :arrow_up: |
| [...plugin/finagle/ClientTracingFilterInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9DbGllbnRUcmFjaW5nRmlsdGVySW50ZXJjZXB0b3IuamF2YQ==) | `100% <100%> (ΓΈ)` | |
| [...lking/apm/plugin/finagle/ContextHolderFactory.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9Db250ZXh0SG9sZGVyRmFjdG9yeS5qYXZh) | `34.82% <34.82%> (ΓΈ)` | |
| [...alking/apm/plugin/finagle/AbstractInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9BYnN0cmFjdEludGVyY2VwdG9yLmphdmE=) | `43.75% <43.75%> (ΓΈ)` | |
| [...in/finagle/ClientDestTracingFilterInterceptor.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9DbGllbnREZXN0VHJhY2luZ0ZpbHRlckludGVyY2VwdG9yLmphdmE=) | `52.38% <52.38%> (ΓΈ)` | |
| [...ywalking/apm/plugin/finagle/SWContextCarrier.scala](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9hcG0vcGx1Z2luL2ZpbmFnbGUvU1dDb250ZXh0Q2Fycmllci5zY2FsYQ==) | `66.66% <66.66%> (ΓΈ)` | |
| [...lking/apm/plugin/finagle/ContextCarrierHelper.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9Db250ZXh0Q2FycmllckhlbHBlci5qYXZh) | `72.72% <72.72%> (ΓΈ)` | |
| [...lking/apm/plugin/finagle/CompatibilityChecker.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9Db21wYXRpYmlsaXR5Q2hlY2tlci5qYXZh) | `73.33% <73.33%> (ΓΈ)` | |
| [...che/skywalking/apm/plugin/finagle/FinagleCtxs.java](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLXNkay1wbHVnaW4vZmluYWdsZS02LjI1LngtcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL2FwbS9wbHVnaW4vZmluYWdsZS9GaW5hZ2xlQ3R4cy5qYXZh) | `76.47% <76.47%> (ΓΈ)` | |
| ... and [16 more](https://codecov.io/gh/apache/skywalking/pull/4441/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=footer). Last update [0df2d0a...4ed9a08](https://codecov.io/gh/apache/skywalking/pull/4441?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389246574
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/CompatibilityChecker.java
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.finagle;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+
+public class CompatibilityChecker {
Review comment:
> The ThreadLocal has low performance in the agent, and cost more CPU resources, comparing to other ways. Your codes use this in high frequency which is a potential performance issue. We only accept one way to use thread local, is there is no way to communicate between the interceptor. From your class name, this is not for that case.
`ContextHolderFactory`? There are no `ThreadLocal` used directly, however, it use `com.twitter.util.Local` which backed by `ThreadLocal`, but this is necessary, please check the comments.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389254457
##########
File path: .github/workflows/plugins-test.yaml
##########
@@ -271,6 +271,12 @@ jobs:
run: bash test/plugin/run.sh undertow-scenario
- name: Run jedis 2.4.0-2.9.0 (18)
run: bash test/plugin/run.sh jedis-scenario
+ - name: Run finagle 6.25.0-6.43.0
+ run: bash test/plugin/run.sh finagle-6.25.x-scenario
+ - name: Run finagle 6.44.0-7.1.0
+ run: bash test/plugin/run.sh finagle-6.44.x-scenario
+ - name: Run finagle 17.10.0-20.1.0
+ run: bash test/plugin/run.sh finagle-17.10.x-scenario
Review comment:
> Are these versions using different coding API for framework end users? Even from 6.33 to 6.34? Could you explain?
Yes. To use finagle, at least needs two jars, `finagle-core` and `finagle-{protocol}(eg: finagle-thrift, finagle-http)`, I choose `finagle-thrift` in tests. The plugin implementation is only based on `finagle-core`, but the difference of coding API comes from `finagle-thrift`.
Specifically, to use `finagle-thrift`, `scrooge-maven-plugin` is also needed to generate code from thrift file, and the versions of `finagle-thrift` and `scrooge-maven-plugin` should be compatible. In these tests, the generatd code of `finagle-6.25.x-scenario` does not compatible with finagle-thrift 6.44. and the generated code of `finagle-6.44.x-scenario` indeed compatible with finagle-thrift 6.43, but does not compatible with all versions from 6.25 to 6.43. I put these versions that both tests compatible into `finagle-6.25.x-scenario`. And from version 17.10, these three jars(`finagle-core`,`finagle-thrift`,`scrooge-maven-plugin`) just has the same version.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] huangyoje commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
huangyoje commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389333363
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
So far it is necessary.
https://github.com/apache/skywalking/pull/4441#discussion_r389333225
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389247511
##########
File path: .github/workflows/plugins-test.yaml
##########
@@ -271,6 +271,12 @@ jobs:
run: bash test/plugin/run.sh undertow-scenario
- name: Run jedis 2.4.0-2.9.0 (18)
run: bash test/plugin/run.sh jedis-scenario
+ - name: Run finagle 6.25.0-6.43.0
+ run: bash test/plugin/run.sh finagle-6.25.x-scenario
+ - name: Run finagle 6.44.0-7.1.0
+ run: bash test/plugin/run.sh finagle-6.44.x-scenario
+ - name: Run finagle 17.10.0-20.1.0
+ run: bash test/plugin/run.sh finagle-17.10.x-scenario
Review comment:
Are these versions using different coding API for framework end users? Even from 6.33 to 6.34? Could you explain?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389264528
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
+
+ finagleSpan.setComponent(FINAGLE);
+ SpanLayer.asRPCFramework(finagleSpan);
+
+ ContextHolder marshlledContextHolder = getMarshalledContextHolder();
+ marshlledContextHolder.let(SWContextCarrier$.MODULE$, SWContextCarrier.of(contextCarrier));
+
+ ContextHolder localContextHolder = getLocalContextHolder();
+ localContextHolder.let(SW_SPAN, finagleSpan);
+
+ enhancedInstance.setSkyWalkingDynamicField(new CacheObjects(marshlledContextHolder, localContextHolder));
Review comment:
`dynamic field` could set any customized class, from my reading, it should be used to replace the `ContextHolder`.
Anyway, I think I need your further explanation about this.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4441:
Add finagle plugin
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4441: Add finagle plugin
URL: https://github.com/apache/skywalking/pull/4441#discussion_r389263325
##########
File path: apm-sniffer/apm-sdk-plugin/finagle-6.25.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/finagle/ClientTracingFilterInterceptor.java
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.finagle;
+
+import com.twitter.util.Future;
+import com.twitter.util.FutureEventListener;
+import org.apache.skywalking.apm.agent.core.context.ContextCarrier;
+import org.apache.skywalking.apm.agent.core.context.ContextManager;
+import org.apache.skywalking.apm.agent.core.context.trace.AbstractSpan;
+import org.apache.skywalking.apm.agent.core.context.trace.SpanLayer;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.EnhancedInstance;
+import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult;
+
+import java.lang.reflect.Method;
+
+import static org.apache.skywalking.apm.network.trace.component.ComponentsDefine.FINAGLE;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getLocalContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.ContextHolderFactory.getMarshalledContextHolder;
+import static org.apache.skywalking.apm.plugin.finagle.FinagleCtxs.SW_SPAN;
+
+public class ClientTracingFilterInterceptor extends AbstractInterceptor {
+
+ private static class CacheObjects {
+ private ContextHolder marshlledContextHolder;
+ private ContextHolder localContextHolder;
+
+ private CacheObjects(ContextHolder marshlledContextHolder, ContextHolder localContextHolder) {
+ this.marshlledContextHolder = marshlledContextHolder;
+ this.localContextHolder = localContextHolder;
+ }
+ }
+
+ @Override
+ protected void onConstructImpl(EnhancedInstance objInst, Object[] allArguments) {
+
+ }
+
+ @Override
+ public void beforeMethodImpl(EnhancedInstance enhancedInstance, Method method, Object[] objects, Class<?>[] classes,
+ MethodInterceptResult methodInterceptResult) throws Throwable {
+ ContextCarrier contextCarrier = new ContextCarrier();
+ /*
+ * At this time, we can't know the operation name and peer address, so we just use placeholders here, the
+ * operation name will be filled by {@link AnnotationInterceptor$Rpc} and the peer address will be filled by
+ * {@link ClientDestTracingFilterInterceptor} later.
+ */
+ AbstractSpan finagleSpan = ContextManager.createExitSpan("pending", contextCarrier, "");
Review comment:
OK. From your codes, you are not just setting the peer later, you are hijacking the ContextCarrier.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services