You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "kylixs (via GitHub)" <gi...@apache.org> on 2023/04/29 16:46:47 UTC
[GitHub] [skywalking-java] kylixs opened a new pull request, #521: Improve bytebuddy class enhance
kylixs opened a new pull request, #521:
URL: https://github.com/apache/skywalking-java/pull/521
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
### Improve bytebuddy class enhance
- [ x ] If this is non-trivial feature, paste the links/URLs to the design doc.
https://github.com/kylixs/kylixs.github.io/blob/master/skywalking/byteduddy-retransfrom-faliure-exploration.md
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-java/blob/main/CHANGES.md).
* Add bytebuddy patch to support retransform class multiple times compatible with other javaagent without class cache.
* Support for hotswap classes that are enhanced by skywalking plugins, like Spring MVC Controller, etc.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1182250166
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/SWAsmVisitorWrapper.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 net.bytebuddy.agent.builder;
+
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.field.FieldList;
+import net.bytebuddy.description.method.MethodList;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.jar.asm.ClassVisitor;
+import net.bytebuddy.jar.asm.FieldVisitor;
+import net.bytebuddy.jar.asm.Opcodes;
+import net.bytebuddy.pool.TypePool;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Remove duplicated fields of instrumentedType
+ */
+public class SWAsmVisitorWrapper implements AsmVisitorWrapper {
Review Comment:
I'll check it agian.
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 net.bytebuddy.implementation;
+
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation.SpecialMethodInvocation;
+import net.bytebuddy.implementation.auxiliary.AuxiliaryType;
+import net.bytebuddy.utility.RandomString;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Generate fixed auxiliary type name of delegate method
+ */
+public class SWAuxiliaryTypeNamingStrategy implements AuxiliaryType.NamingStrategy {
+ private static final String DEFAULT_PREFIX = "sw_auxiliary";
+ private static ILog LOGGER = LogManager.getLogger(SWAuxiliaryTypeNamingStrategy.class);
+ private String suffix;
+
+ public SWAuxiliaryTypeNamingStrategy() {
+ this(DEFAULT_PREFIX);
+ }
+
+ public SWAuxiliaryTypeNamingStrategy(String suffix) {
+ this.suffix = suffix;
+ }
+
+ @Override
+ public String name(TypeDescription instrumentedType, AuxiliaryType auxiliaryType) {
+ String description = findDescription(auxiliaryType);
+ if (description != null) {
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(description.hashCode());
+ }
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(auxiliaryType.hashCode());
+ }
+
+ private String findDescription(AuxiliaryType auxiliaryType) {
+ try {
+ Class<? extends AuxiliaryType> auxiliaryTypeClass = auxiliaryType.getClass();
+ String auxiliaryTypeClassName = auxiliaryTypeClass.getName();
+ if (auxiliaryTypeClassName.endsWith("Morph$Binder$RedirectionProxy")
+ || auxiliaryTypeClassName.endsWith("MethodCallProxy")) {
+ // get MethodDescription from field 'specialMethodInvocation.methodDescription'
+ Field specialMethodInvocationField = auxiliaryTypeClass.getDeclaredField("specialMethodInvocation");
+ specialMethodInvocationField.setAccessible(true);
+ SpecialMethodInvocation specialMethodInvocation = (SpecialMethodInvocation) specialMethodInvocationField.get(auxiliaryType);
Review Comment:
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1567717884
Generally, the ides works for me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531034463
Hi @kylixs Thanks for providing your way of debugging and resolving the issue. I believe locking the field and method name for auxiliary class and method is a way to resolve the transfer issue.
But meanwhile, I am not very certain, whether this is always perfect, as SkyWalking agent kernel has been used in many other cases, such as other APMs' agent are actually a fork of upstream.
Is this way going to bring a new conflict between skywalking agent and skywalking agent fork(re-distribution)? Our answer doesn't have to be that, we support this kind of hybrid deployment, but I want to include this as a consideration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1182242937
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/NativeMethodStrategySupport.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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 net.bytebuddy.agent.builder;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Inject custom NativeMethodStrategy to AgentBuilder
+ */
+public class NativeMethodStrategySupport {
+ private static final String PREFIX = "sw_origin$";
+ private static ILog LOGGER = LogManager.getLogger(NativeMethodStrategySupport.class);
+
+ public static void inject(AgentBuilder agentBuilder) {
+ Class<? extends AgentBuilder> clazz = agentBuilder.getClass();
+ if (clazz != AgentBuilder.Default.class) {
+ throw new IllegalStateException("Only accept original AgentBuilder instance but not a wrapper instance: " + clazz.getName());
+ }
+ try {
+ Field nativeMethodStrategyField = clazz.getDeclaredField("nativeMethodStrategy");
+ nativeMethodStrategyField.setAccessible(true);
+ nativeMethodStrategyField.set(agentBuilder, new SWNativeMethodStrategy(PREFIX));
Review Comment:
OK, That's what I am thinking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531388620
> I can't provide this information as I don't have their owner permission, sorry. I just want to mention this as a case. Such as if they have a modified plugin/interceptor but targeting the same class and same method.
Without enough information, it's hard to evaluate the results. Some possibilities:
* Custom plugins: For example, two plugins enhance the same method of the same class. Not tested yet, don't know if this is currently supported. If we want to support it, we should define it as a feature and add test cases.
* Custom class transformer: Use a different ByteBuddy/AgentBuilder instance so it is not affected by name locking.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1586109838
> I remembered that @raphw mentioned, the `advice` annotation rather than delegator could support multiple interceptors. But I didn't try.
We can intercept the target class method using Advice, and then filter the matching plugins in the Advice static proxy method and invoke the intercepting classes of plugins.
It seems that this method can adapt to the original plugin, but need to change the plugin mechanism, I try it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1182177929
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/ClassEnhancePluginDefine.java:
##########
@@ -65,8 +65,8 @@ public abstract class ClassEnhancePluginDefine extends AbstractClassEnhancePlugi
*/
@Override
protected DynamicType.Builder<?> enhanceInstance(TypeDescription typeDescription,
- DynamicType.Builder<?> newClassBuilder, ClassLoader classLoader,
- EnhanceContext context) throws PluginException {
+ DynamicType.Builder<?> newClassBuilder, ClassLoader classLoader,
+ EnhanceContext context) throws PluginException {
Review Comment:
These seem some local code style differences. Could you revert these changes?
##########
apm-sniffer/apm-agent-core/src/main/java/org/apache/skywalking/apm/agent/core/plugin/interceptor/enhance/ClassEnhancePluginDefine.java:
##########
@@ -113,15 +114,15 @@ protected DynamicType.Builder<?> enhanceInstance(TypeDescription typeDescription
for (ConstructorInterceptPoint constructorInterceptPoint : constructorInterceptPoints) {
if (isBootstrapInstrumentation()) {
newClassBuilder = newClassBuilder.constructor(constructorInterceptPoint.getConstructorMatcher())
- .intercept(SuperMethodCall.INSTANCE.andThen(MethodDelegation.withDefaultConfiguration()
- .to(BootstrapInstrumentBoost
- .forInternalDelegateClass(constructorInterceptPoint
- .getConstructorInterceptor()))));
+ .intercept(SuperMethodCall.INSTANCE.andThen(MethodDelegation.withDefaultConfiguration()
+ .to(BootstrapInstrumentBoost
+ .forInternalDelegateClass(constructorInterceptPoint
+ .getConstructorInterceptor()))));
Review Comment:
Please revert all unnecessary changes.
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/NativeMethodStrategySupport.java:
##########
@@ -0,0 +1,46 @@
+/*
+ * 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 net.bytebuddy.agent.builder;
+
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Inject custom NativeMethodStrategy to AgentBuilder
+ */
+public class NativeMethodStrategySupport {
+ private static final String PREFIX = "sw_origin$";
+ private static ILog LOGGER = LogManager.getLogger(NativeMethodStrategySupport.class);
+
+ public static void inject(AgentBuilder agentBuilder) {
+ Class<? extends AgentBuilder> clazz = agentBuilder.getClass();
+ if (clazz != AgentBuilder.Default.class) {
+ throw new IllegalStateException("Only accept original AgentBuilder instance but not a wrapper instance: " + clazz.getName());
+ }
+ try {
+ Field nativeMethodStrategyField = clazz.getDeclaredField("nativeMethodStrategy");
+ nativeMethodStrategyField.setAccessible(true);
+ nativeMethodStrategyField.set(agentBuilder, new SWNativeMethodStrategy(PREFIX));
Review Comment:
These seem a hijacking and relying on upstream(bytebuddy) doesn't change the internal implementation.
Considering bytebuddy is always very friendly to our community, you are better to submit a proposal/request to bytebuddy, and discuss the official API to do so.
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 net.bytebuddy.implementation;
+
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation.SpecialMethodInvocation;
+import net.bytebuddy.implementation.auxiliary.AuxiliaryType;
+import net.bytebuddy.utility.RandomString;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Generate fixed auxiliary type name of delegate method
+ */
+public class SWAuxiliaryTypeNamingStrategy implements AuxiliaryType.NamingStrategy {
+ private static final String DEFAULT_PREFIX = "sw_auxiliary";
+ private static ILog LOGGER = LogManager.getLogger(SWAuxiliaryTypeNamingStrategy.class);
+ private String suffix;
+
+ public SWAuxiliaryTypeNamingStrategy() {
+ this(DEFAULT_PREFIX);
+ }
+
+ public SWAuxiliaryTypeNamingStrategy(String suffix) {
+ this.suffix = suffix;
+ }
+
+ @Override
+ public String name(TypeDescription instrumentedType, AuxiliaryType auxiliaryType) {
+ String description = findDescription(auxiliaryType);
+ if (description != null) {
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(description.hashCode());
+ }
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(auxiliaryType.hashCode());
+ }
+
+ private String findDescription(AuxiliaryType auxiliaryType) {
+ try {
+ Class<? extends AuxiliaryType> auxiliaryTypeClass = auxiliaryType.getClass();
+ String auxiliaryTypeClassName = auxiliaryTypeClass.getName();
+ if (auxiliaryTypeClassName.endsWith("Morph$Binder$RedirectionProxy")
+ || auxiliaryTypeClassName.endsWith("MethodCallProxy")) {
+ // get MethodDescription from field 'specialMethodInvocation.methodDescription'
+ Field specialMethodInvocationField = auxiliaryTypeClass.getDeclaredField("specialMethodInvocation");
+ specialMethodInvocationField.setAccessible(true);
+ SpecialMethodInvocation specialMethodInvocation = (SpecialMethodInvocation) specialMethodInvocationField.get(auxiliaryType);
Review Comment:
This is also another kind hijacking, please considering working with the upstream.
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -161,9 +175,10 @@ public DynamicType.Builder<?> transform(final DynamicType.Builder<?> builder,
final JavaModule javaModule,
final ProtectionDomain protectionDomain) {
LoadedLibraryCollector.registerURLClassLoader(classLoader);
+ DelegateNamingResolver.reset();
Review Comment:
I am a little confused about this `reset`. `#transform` should be called for every type, as a static method, we reset every time?
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 net.bytebuddy.implementation;
+
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation.SpecialMethodInvocation;
+import net.bytebuddy.implementation.auxiliary.AuxiliaryType;
+import net.bytebuddy.utility.RandomString;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Generate fixed auxiliary type name of delegate method
+ */
+public class SWAuxiliaryTypeNamingStrategy implements AuxiliaryType.NamingStrategy {
+ private static final String DEFAULT_PREFIX = "sw_auxiliary";
+ private static ILog LOGGER = LogManager.getLogger(SWAuxiliaryTypeNamingStrategy.class);
+ private String suffix;
+
+ public SWAuxiliaryTypeNamingStrategy() {
+ this(DEFAULT_PREFIX);
+ }
+
+ public SWAuxiliaryTypeNamingStrategy(String suffix) {
+ this.suffix = suffix;
+ }
+
+ @Override
+ public String name(TypeDescription instrumentedType, AuxiliaryType auxiliaryType) {
+ String description = findDescription(auxiliaryType);
+ if (description != null) {
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(description.hashCode());
+ }
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(auxiliaryType.hashCode());
+ }
+
+ private String findDescription(AuxiliaryType auxiliaryType) {
+ try {
+ Class<? extends AuxiliaryType> auxiliaryTypeClass = auxiliaryType.getClass();
+ String auxiliaryTypeClassName = auxiliaryTypeClass.getName();
+ if (auxiliaryTypeClassName.endsWith("Morph$Binder$RedirectionProxy")
+ || auxiliaryTypeClassName.endsWith("MethodCallProxy")) {
+ // get MethodDescription from field 'specialMethodInvocation.methodDescription'
+ Field specialMethodInvocationField = auxiliaryTypeClass.getDeclaredField("specialMethodInvocation");
+ specialMethodInvocationField.setAccessible(true);
+ SpecialMethodInvocation specialMethodInvocation = (SpecialMethodInvocation) specialMethodInvocationField.get(auxiliaryType);
+ MethodDescription methodDescription = specialMethodInvocation.getMethodDescription();
+ return methodDescription.toString();
+
+ } else if (auxiliaryTypeClassName.endsWith("Pipe$Binder$RedirectionProxy")) {
+ // get MethodDescription from field 'sourceMethod'
+ Field sourceMethodField = auxiliaryTypeClass.getDeclaredField("sourceMethod");
Review Comment:
Same hijacking concern.
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWAuxiliaryTypeNamingStrategy.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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 net.bytebuddy.implementation;
+
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation.SpecialMethodInvocation;
+import net.bytebuddy.implementation.auxiliary.AuxiliaryType;
+import net.bytebuddy.utility.RandomString;
+import org.apache.skywalking.apm.agent.core.logging.api.ILog;
+import org.apache.skywalking.apm.agent.core.logging.api.LogManager;
+
+import java.lang.reflect.Field;
+
+/**
+ * Generate fixed auxiliary type name of delegate method
+ */
+public class SWAuxiliaryTypeNamingStrategy implements AuxiliaryType.NamingStrategy {
+ private static final String DEFAULT_PREFIX = "sw_auxiliary";
+ private static ILog LOGGER = LogManager.getLogger(SWAuxiliaryTypeNamingStrategy.class);
+ private String suffix;
+
+ public SWAuxiliaryTypeNamingStrategy() {
+ this(DEFAULT_PREFIX);
+ }
+
+ public SWAuxiliaryTypeNamingStrategy(String suffix) {
+ this.suffix = suffix;
+ }
+
+ @Override
+ public String name(TypeDescription instrumentedType, AuxiliaryType auxiliaryType) {
+ String description = findDescription(auxiliaryType);
+ if (description != null) {
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(description.hashCode());
+ }
+ return instrumentedType.getName() + "$" + suffix + "$" + RandomString.hashOf(auxiliaryType.hashCode());
+ }
+
+ private String findDescription(AuxiliaryType auxiliaryType) {
+ try {
+ Class<? extends AuxiliaryType> auxiliaryTypeClass = auxiliaryType.getClass();
+ String auxiliaryTypeClassName = auxiliaryTypeClass.getName();
+ if (auxiliaryTypeClassName.endsWith("Morph$Binder$RedirectionProxy")
+ || auxiliaryTypeClassName.endsWith("MethodCallProxy")) {
+ // get MethodDescription from field 'specialMethodInvocation.methodDescription'
+ Field specialMethodInvocationField = auxiliaryTypeClass.getDeclaredField("specialMethodInvocation");
+ specialMethodInvocationField.setAccessible(true);
+ SpecialMethodInvocation specialMethodInvocation = (SpecialMethodInvocation) specialMethodInvocationField.get(auxiliaryType);
+ MethodDescription methodDescription = specialMethodInvocation.getMethodDescription();
+ return methodDescription.toString();
+
+ } else if (auxiliaryTypeClassName.endsWith("Pipe$Binder$RedirectionProxy")) {
+ // get MethodDescription from field 'sourceMethod'
+ Field sourceMethodField = auxiliaryTypeClass.getDeclaredField("sourceMethod");
+ sourceMethodField.setAccessible(true);
+ MethodDescription sourceMethod = (MethodDescription) sourceMethodField.get(auxiliaryType);
+ return sourceMethod.toString();
+
+ } else if (auxiliaryTypeClassName.endsWith("FieldProxy$Binder$AccessorProxy")) {
+ // get fieldDescription
+ Field fieldDescriptionField = auxiliaryTypeClass.getDeclaredField("fieldDescription");
Review Comment:
Same hijacking concern.
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/agent/builder/SWAsmVisitorWrapper.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 net.bytebuddy.agent.builder;
+
+import net.bytebuddy.asm.AsmVisitorWrapper;
+import net.bytebuddy.description.field.FieldDescription;
+import net.bytebuddy.description.field.FieldList;
+import net.bytebuddy.description.method.MethodList;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.implementation.Implementation;
+import net.bytebuddy.jar.asm.ClassVisitor;
+import net.bytebuddy.jar.asm.FieldVisitor;
+import net.bytebuddy.jar.asm.Opcodes;
+import net.bytebuddy.pool.TypePool;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Remove duplicated fields of instrumentedType
+ */
+public class SWAsmVisitorWrapper implements AsmVisitorWrapper {
Review Comment:
I am not fully following this. Are you trying to cache all original methods before the instrumentation?
The name `RemoveDuplicatedFieldsClassVisitor` is not very clear to me.
Please reword the comments for `SWAsmVisitorWrapper`
##########
apm-sniffer/bytebuddy-patch/src/main/java/net/bytebuddy/implementation/SWImplementationContextFactory.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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 net.bytebuddy.implementation;
+
+import net.bytebuddy.ClassFileVersion;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.dynamic.scaffold.TypeInitializer;
+import net.bytebuddy.implementation.auxiliary.AuxiliaryType;
+import net.bytebuddy.utility.RandomString;
+
+/**
+ * Design to generate fixed name for CacheValueField (cache delegating Method)
Review Comment:
This comment doesn't fit the codes. Please reword this, you are building a ContextFactory with target type name based suffix on the top of `Implementation.Context.Default`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1182239454
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -161,9 +175,10 @@ public DynamicType.Builder<?> transform(final DynamicType.Builder<?> builder,
final JavaModule javaModule,
final ProtectionDomain protectionDomain) {
LoadedLibraryCollector.registerURLClassLoader(classLoader);
+ DelegateNamingResolver.reset();
Review Comment:
There seems to be a problem here, I'm not sure if there will be nested retransform multiple classes, which usually occurs the first time a class is loaded. If so, it would be unreasonable to reset all of them, just reset the counter of the class currently being transformed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531420725
> Custom plugins: For example, two plugins enhance the same method of the same class. Not tested yet, don't know if this is currently supported. If we want to support it, we should define it as a feature and add test cases.
This is the case I want to discuss it. AFAIK, we have problems 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531190316
> The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.
I don't quite understand this. Could you explain it in detail?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1533332570
> How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?
Unfortunately, there is a problem with intercepting constructors: Once a constructor is intercepted on one agent, other methods of the class that are intercepted on another agent will fail.
The second agent could not find the constructor's dependent auxiliary class (created in the first agent) when parsing the target class. The auxiliary class actually exists in the classloader, but couldn't get the bytecode, even using `ClassFileLocator. ForInstrumentation`:
```agentBuilder.with(ClassFileLocator.ForInstrumentation.fromInstalledAgent(...) )```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1533982970
OK, I will submit the problem and demo code to ByteBuddy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531276945
ByteBuddy uses random names, which is essential in dynamic proxy/class creation, but in APM scenarios, dynamic names are bad.
We use `sw_xxx$` as a specific flag for auxiliary class names, field names, and method names that do not conflict with other vendor names.
Vendors can extend with the Skywalking agent interceptor mechanism, or register their own class transformer, which is fine. The locking name only affects the Skywalking agent itself, and vendors using their own ByteBuddy instances are unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1603943407
move to: https://github.com/apache/skywalking-java/pull/561
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1567717237
> What do you mean OK? We only have one, right? You changed it to multiple? Or one per plugin or something?
Currently, it is supported to install an interceptor on the same method in two AgentBuilders respectively, resulting in a chain of proxy methods.
I think there are a few scenarios where conflicts need to be resolved:
1. The interception method of user-defined plug-ins may overlap with the existing plug-ins of Skywalking agent.
2. Two Skywalking agent plug-ins may handle the same method.
Interception methods can overlap in many ways, such as two plug-ins matching different annotations but having both annotations in the same class or method.
I think it is better to solve the above problems in one agent, because putting it into two agents will bring maintenance workload and confusion. I think users would prefer to maintain one custom plug-in rather than two agents.
One idea is to identify plug-ins that overlap and use different AgentBuilders to handle them. I'll try to see if this works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1528896832
I will be back on this at May 3rd.
Thanks for your detailed explanation.
I will take a close look and provide feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531137660
>
It's a bit complicated, and we haven't tested enough scenarios to know if there are other compatibility issues. I came up with a few ideas:
* Provide a fallback transform mode. If there is a compatibility problem, users can fall back to the previous mode. We can keep improving it.
* Create a retransform javaagent for automated testing every plugin for compatibility, and may need to modify testing tools to support it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1182241765
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -161,9 +175,10 @@ public DynamicType.Builder<?> transform(final DynamicType.Builder<?> builder,
final JavaModule javaModule,
final ProtectionDomain protectionDomain) {
LoadedLibraryCollector.registerURLClassLoader(classLoader);
+ DelegateNamingResolver.reset();
Review Comment:
If byte-buddy could expose the whole method signature rather than the name, I believe this would not be an issue.
How about asking for upstream 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1602971570
> I agree with your idea, but I think <1> and <2> are the same things, and have been reached.
I'm not sure to support that without hijacking, maybe make it a little more elegant.
I don't think we will avoid all hijackings, but we could limit the scope, currently, they are too wide. And we could add tests to verify them when we need to bump up the version of Byte-buddy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531445019
> > Custom plugins: For example, two plugins enhance the same method of the same class. Not tested yet, don't know if this is currently supported. If we want to support it, we should define it as a feature and add test cases.
>
> This is the case I want to discuss it. AFAIK, we have problems about this.
Let me have a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531231277
> > The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.
>
>
>
> I don't quite understand this. Could you explain it in detail?
The name of fields and methods are predictable and will be same.
There are other solutions of other vendors using our agent to build their own distributions, which may be APM agents or even others.
Once the names are locked, would they conflict with each others?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1603944089
move to: https://github.com/apache/skywalking-java/pull/561
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531739061
> How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?
If both agents lock names and the special identifier in the name is the same, there will be a conflict. We can solve this problem by randomly generating an identity when the agent starts.
Others need to be tested in practice. I think there is no big problem to do so, feedback later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531747296
> If both agents lock names and the special identifier in the name is the same, there will be a conflict. We can solve this problem by randomly generating an identity when the agent starts.
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1566186815
> > How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?
>
>
>
> Intercept same classes with two AgentBuilders are ok. Do you have any ideas on how to add specific test cases?
What do you mean OK? We only have one, right? You changed it to multiple? Or one per plugin or something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1533932998
Got it. Please ask for help from byte-buddy. I think we at least need two issues on upstream for making this ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1586097262
Took some time to try two solutions:
1. Each plugin uses an isolated `AgentBuilder`, and it turns out that each AgentBuilder instance do a lot of repetitive processing, which makes Java application start too slowly.
2. Each plugin uses an isolated `Transformer` and found that ByteBuddy is not supported.
I made some rough changes to the ByteBuddy AgentBuilder code, and it is now running, loading two plugins at the same time to enhance the classes of Spring Controller, and they both work.
I will submit it to the ByteBuddy community and need more discussion and design.
ByteBuddy code: https://github.com/kylixs/byte-buddy/tree/separate-transformer
Custom spring mvc plugin: https://github.com/kylixs/spring-mvc-plugin-demo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] raphw commented on pull request #521: Improve bytebuddy class enhance
Posted by "raphw (via GitHub)" <gi...@apache.org>.
raphw commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1588140998
Sorry, but this does not go into a direction that I see being merged into Byte Buddy. But you are very welcome to implement this on top of existing APIs and to offer it as an extension.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1586118138
It is good to tesr various possibilities. Eventually, we should keep changes as less as possible, to keep stability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1602889663
@wu-sheng I agree with your idea, but I think <1> and <2> are the same things, and have been reached.
> <3> Support multiple interceptors for one single method.
What has been tried:
1. Each plugin create a separate `Transformer`, requiring `AgentBuilder `to support isolated transformers.
2. With `Advice` transformer, the enhanced code is bloated and may be repeatedly enhanced, which is a bit troublesome to control.
Next step:
Make `InstMethodsInter` and so on support for calling multiple `XxxAroundInterceptors `of different plugins.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on a diff in pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on code in PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#discussion_r1208585380
##########
apm-sniffer/apm-agent/src/main/java/org/apache/skywalking/apm/agent/SkyWalkingAgent.java:
##########
@@ -161,9 +175,10 @@ public DynamicType.Builder<?> transform(final DynamicType.Builder<?> builder,
final JavaModule javaModule,
final ProtectionDomain protectionDomain) {
LoadedLibraryCollector.registerURLClassLoader(classLoader);
+ DelegateNamingResolver.reset();
Review Comment:
What do you mean? Tell me more about it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1602787096
@kylixs I am back on this as the 9.5.0 had been released. AFAIK, there are several things scoped in this PR, which makes it very hard to review and move forward from tech and stable perspectives.
So, I want to separate the issue into the following parts
1. Support predictable field names for interceptors
2. Support retransfer with other agents. I think it is easy if we landed <1>
3. Support multiple interceptors for one single method. This may be the hard part, but to be honest, this is not a very common use case inside SkyWalking.
So, how about we are going to do <1> only first? I think from your existing changes, it should be easy to support that without hijacking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1603865654
@kylixs Please open a new and clean changes PR. Let's focus on one at a time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531301699
If they haven't modified the Skywalking agent's plugin interceptor flow, they should be compatible. If they change the interceptor mechanism or the ByteBuddy option, they need to be compatible with the new mode. More information? Are there any known redistribution projects of Skywalking agent?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531144883
> Provide a fallback transform mode. If there is a compatibility problem, users can fall back to the previous mode. We can keep improving it.
Falling back usually is too complex to maintain.
___
About the re-transfer, once we have a clear path about how skywalking agent works in that agent, I think we are fine.
The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531319287
I can't provide this information as I don't have their owner permission, sorry.
I just want to mention this as a case. Such as if they have a modified plugin/interceptor but targeting the same class and same method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531697520
> > Custom plugins: For example, two plugins enhance the same method of the same class. Not tested yet, don't know if this is currently supported. If we want to support it, we should define it as a feature and add test cases.
>
> This is the case I want to discuss it. AFAIK, we have problems about this.
Some discoveries:
* A ByteBuddy/AgentBuilder instance installs multiple interceptors on the same method of the same class, with only the last one taking effect
* Multiple ByteBuddy/AgentBuilder instances that install an interceptor on the same method of the same class form an interceptor call chain
* Locking names does not change the behavior above
However, we have only one instance of ByteBuddy, and it cannot directly generate an interceptor chain. If we want to solve the problem of multiple interceptions of the same method, we may need to modify the interceptor mechanism to maintain a chain of interceptors within the Skywalking agent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531178316
> About the re-transfer, once we have a clear path about how skywalking agent works in that agent, I think we are fine.
> The question I left is only about whether we are going to conflict with our own fork, due to names are always same in two agents.
The key issue is the class and method that the plug-in intercepts. As you can see from the documentation of the process, the processing path is fairly clear, but it may not include all of the paths. Currently, all skywalking plug-ins are supported, but we don't know if any of the custom plug-ins have compatibility issues. At present, we have a better understanding of ByteBuddy processing process and debugging tools, which can solve the problem quickly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531289029
I know they can, I mean many of them are not doing this.
I think you still are not following what is redistribution of open source. Thinking about Ubuntu relationship with upstream Linux. Take SkyWalking as upstream Linux, vendor agents are a kind of Ubuntu.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1531712209
> Multiple ByteBuddy/AgentBuilder instances that install an interceptor on the same method of the same class form an interceptor call chain
How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?
> Locking names does not change the behavior above
Generally, we are fine to the new solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1602966318
> What has been tried:
Each plugin create a separate Transformer, requiring AgentBuilder to support isolated transformers.
With Advice transformer, the enhanced code is bloated and may be repeatedly enhanced, which is a bit troublesome to control.
Next step:
Make InstMethodsInter and so on to support for calling multiple XxxAroundInterceptors of different plugins.
That is fine.It can wait.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs closed pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs closed pull request #521: Improve bytebuddy class enhance
URL: https://github.com/apache/skywalking-java/pull/521
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] wu-sheng commented on pull request #521: Improve bytebuddy class enhance
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1586098517
I remembered that @raphw mentioned, the `advice` annotation rather than delegator could support multiple interceptors. But I didn't try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking-java] kylixs commented on pull request #521: Improve bytebuddy class enhance
Posted by "kylixs (via GitHub)" <gi...@apache.org>.
kylixs commented on PR #521:
URL: https://github.com/apache/skywalking-java/pull/521#issuecomment-1566185051
> How about two skywalking agents for one target? I don't expect we would do two interceptors for one target inside skywalking. The other one is fork distribution. Will that work?
Intercept same classes with two AgentBuilders are ok. Do you have any ideas on how to add specific test cases?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org