kezhenxu94 commented on a change in pull request #39: URL: https://github.com/apache/skywalking-java/pull/39#discussion_r716321760
########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/define/ObjectMapperInstrumentation.java ########## @@ -0,0 +1,53 @@ +/* + * 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.jackson.define; + +import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch; +import org.apache.skywalking.apm.agent.core.plugin.match.NameMatch; + +import java.util.HashMap; +import java.util.Map; + +/** + * Jackson has been known as "the Java JSON library" or "the best JSON parser for Java". Or simply as "JSON for Java". + * <p> + * Jackson provides a "one stop" solution for json serialization and deserialization solution basic requirements. + * <p> + * ObjectMapper: writeValue()\writeValueAsString()\writeValueAsBytes()\readValue() + */ + +public class ObjectMapperInstrumentation extends AbstractInstrumentation { + + @Override + protected ClassMatch enhanceClass() { + return NameMatch.byName("com.fasterxml.jackson.databind.ObjectMapper"); + } + + @Override + protected Map<String, String> enhanceMethods() { + return new HashMap<String, String>() { + { + put("writeValue", "org.apache.skywalking.apm.plugin.jackson.BasicMethodsInterceptor"); + put("writeValueAsString", "org.apache.skywalking.apm.plugin.jackson.WriteValueAsStringInterceptor"); + put("writeValueAsBytes", "org.apache.skywalking.apm.plugin.jackson.WriteValueAsBytesInterceptor"); + put("readValue", "org.apache.skywalking.apm.plugin.jackson.ReadValueInterceptor"); + } + }; Review comment: Consider using a builder to build the map instead of creating anonymous inner classes, which creates unnecessary class. ```suggestion return ImmutableMap.<String, String>builder() .put( "writeValue", "org.apache.skywalking.apm.plugin.jackson.BasicMethodsInterceptor" ) .put( "writeValueAsString", "org.apache.skywalking.apm.plugin.jackson.WriteValueAsStringInterceptor" ) .put( "writeValueAsBytes", "org.apache.skywalking.apm.plugin.jackson.WriteValueAsBytesInterceptor" ) .put( "readValue", "org.apache.skywalking.apm.plugin.jackson.ReadValueInterceptor" ) .build(); ``` ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/define/ObjectMapperInstrumentation.java ########## @@ -0,0 +1,53 @@ +/* + * 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.jackson.define; + +import org.apache.skywalking.apm.agent.core.plugin.match.ClassMatch; +import org.apache.skywalking.apm.agent.core.plugin.match.NameMatch; + +import java.util.HashMap; +import java.util.Map; + +/** + * Jackson has been known as "the Java JSON library" or "the best JSON parser for Java". Or simply as "JSON for Java". Review comment: Same here ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/pom.xml ########## @@ -0,0 +1,58 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + ~ Licensed to the Apache Software Foundation (ASF) under one or more + ~ contributor license agreements. See the NOTICE file distributed with + ~ this work for additional information regarding copyright ownership. + ~ The ASF licenses this file to You under the Apache License, Version 2.0 + ~ (the "License"); you may not use this file except in compliance with + ~ the License. You may obtain a copy of the License at + ~ + ~ http://www.apache.org/licenses/LICENSE-2.0 + ~ + ~ Unless required by applicable law or agreed to in writing, software + ~ distributed under the License is distributed on an "AS IS" BASIS, + ~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + ~ See the License for the specific language governing permissions and + ~ limitations under the License. + ~ + --> + +<project xmlns="http://maven.apache.org/POM/4.0.0"; xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.skywalking</groupId> + <artifactId>optional-plugins</artifactId> + <version>8.8.0-SNAPSHOT</version> + </parent> + + <artifactId>apm-jackson-2.x-plugin</artifactId> + <name>jackson-2.x-plugin</name> + <packaging>jar</packaging> + + <properties> + <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> + <jackson.version>2.9.9</jackson.version> + </properties> + + <dependencies> + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-core</artifactId> + <version>${jackson.version}</version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-annotations</artifactId> Review comment: Is this needed? Seems it's unused ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/define/AbstractInstrumentation.java ########## @@ -0,0 +1,81 @@ +/* + * 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.jackson.define; + +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static net.bytebuddy.matcher.ElementMatchers.named; + +/** + * Jackson has been known as "the Java JSON library" or "the best JSON parser for Java". Or simply as "JSON for Java". + * <p> + * Jackson provides a "one stop" solution for json serialization and deserialization solution basic requirements. + */ + +public abstract class AbstractInstrumentation extends ClassInstanceMethodsEnhancePluginDefine { + + @Override + public ConstructorInterceptPoint[] getConstructorsInterceptPoints() { + return new ConstructorInterceptPoint[0]; + } + + @Override + public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() { + + final List<InstanceMethodsInterceptPoint> points = new ArrayList<InstanceMethodsInterceptPoint>(this.enhanceMethods().size()); Review comment: ```suggestion final List<InstanceMethodsInterceptPoint> points = new ArrayList<>(this.enhanceMethods().size()); ``` ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/define/AbstractInstrumentation.java ########## @@ -0,0 +1,81 @@ +/* + * 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.jackson.define; + +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static net.bytebuddy.matcher.ElementMatchers.named; + +/** + * Jackson has been known as "the Java JSON library" or "the best JSON parser for Java". Or simply as "JSON for Java". Review comment: Hi @VictorZeng , can you remove the words `the best` and avoid using similar words in the future? I don't mean they are wrong or Jackson is not good enough, but in Apache projects, we should not endorse for any third-parties / vendors. ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/BasicMethodsInterceptor.java ########## @@ -0,0 +1,51 @@ +/* + * 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.jackson; + +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.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; + +import java.lang.reflect.Method; + +public class BasicMethodsInterceptor implements InstanceMethodsAroundInterceptor { Review comment: Import our code style settings and format this file, it has some lines that are too long ########## File path: apm-sniffer/optional-plugins/jackson-2.x-plugin/src/main/java/org/apache/skywalking/apm/plugin/jackson/define/AbstractInstrumentation.java ########## @@ -0,0 +1,81 @@ +/* + * 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.jackson.define; + +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.ConstructorInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.InstanceMethodsInterceptPoint; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.ClassInstanceMethodsEnhancePluginDefine; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; + +import static net.bytebuddy.matcher.ElementMatchers.named; + +/** + * Jackson has been known as "the Java JSON library" or "the best JSON parser for Java". Or simply as "JSON for Java". + * <p> + * Jackson provides a "one stop" solution for json serialization and deserialization solution basic requirements. + */ + +public abstract class AbstractInstrumentation extends ClassInstanceMethodsEnhancePluginDefine { + + @Override + public ConstructorInterceptPoint[] getConstructorsInterceptPoints() { + return new ConstructorInterceptPoint[0]; + } + + @Override + public InstanceMethodsInterceptPoint[] getInstanceMethodsInterceptPoints() { + + final List<InstanceMethodsInterceptPoint> points = new ArrayList<InstanceMethodsInterceptPoint>(this.enhanceMethods().size()); + + for (Map.Entry<String, String> entry : this.enhanceMethods().entrySet()) { + final InstanceMethodsInterceptPoint point = new InstanceMethodsInterceptPoint() { + @Override + public ElementMatcher<MethodDescription> getMethodsMatcher() { + return named(entry.getKey()); + } + + @Override + public String getMethodsInterceptor() { + return entry.getValue(); + } + + @Override + public boolean isOverrideArgs() { + return false; + } + }; + points.add(point); + } + + return points.toArray(new InstanceMethodsInterceptPoint[points.size()]); Review comment: This is recommended, I remember the Fastjson did the same thing, can you update that too? ```suggestion return points.toArray(new InstanceMethodsInterceptPoint[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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
