muse-dev[bot] commented on a change in pull request #5644: URL: https://github.com/apache/skywalking/pull/5644#discussion_r502782024
########## File path: apm-sniffer/apm-sdk-plugin/thrift-plugin/src/main/java/org/apache/skywalking/apm/plugin/thrift/wrapper/ServerInProtocolWrapper.java ########## @@ -0,0 +1,110 @@ +/* + * 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.thrift.wrapper; + +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import org.apache.skywalking.apm.agent.core.context.CarrierItem; +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.tag.StringTag; +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.logging.api.ILog; +import org.apache.skywalking.apm.agent.core.logging.api.LogManager; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.thrift.TException; +import org.apache.thrift.protocol.TField; +import org.apache.thrift.protocol.TMap; +import org.apache.thrift.protocol.TMessage; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.protocol.TType; + +/** + * Wrapping server input protocol for reading and parsing the trace header from the original protocol. It is a special + * field which will be skipped without one deal with. + */ +public class ServerInProtocolWrapper extends AbstractProtocolWrapper { + private static final ILog LOGGER = LogManager.getLogger(ServerInProtocolWrapper.class); + private static final StringTag TAG_ARGS = new StringTag("args"); + private AbstractContext context; + + public ServerInProtocolWrapper(final TProtocol protocol) { + super(protocol); + } + + public void initial(AbstractContext context) { + this.context = context; + } + + @Override + public TField readFieldBegin() throws TException { + final TField field = super.readFieldBegin(); + if (field.id == SW_MAGIC_FIELD_ID && field.type == TType.MAP) { + try { + TMap tMap = super.readMapBegin(); + Map<String, String> header = new HashMap(tMap.size); + + for (int i = 0; i < tMap.size; i++) { + header.put(readString(), readString()); + } + + super.readMessageEnd(); + super.readFieldEnd(); + + AbstractSpan span = ContextManager.createEntrySpan( + context.getOperatorName(), createContextCarrier(header)); + span.start(context.startTime); + span.tag(TAG_ARGS, context.getArguments()); + span.setComponent(ComponentsDefine.THRIFT_SERVER); + SpanLayer.asRPCFramework(span); + } catch (Throwable throwable) { + LOGGER.error("Failed to create EntrySpan.", throwable); + } finally { + context = null; + return readFieldBegin(); Review comment: *Finally:* If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead. ########## File path: apm-sniffer/apm-sdk-plugin/thrift-plugin/src/main/java/org/apache/skywalking/apm/plugin/thrift/wrapper/ServerInProtocolWrapper.java ########## @@ -0,0 +1,110 @@ +/* + * 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.thrift.wrapper; + +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import org.apache.skywalking.apm.agent.core.context.CarrierItem; +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.tag.StringTag; +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.logging.api.ILog; +import org.apache.skywalking.apm.agent.core.logging.api.LogManager; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.thrift.TException; +import org.apache.thrift.protocol.TField; +import org.apache.thrift.protocol.TMap; +import org.apache.thrift.protocol.TMessage; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.protocol.TType; + +/** + * Wrapping server input protocol for reading and parsing the trace header from the original protocol. It is a special + * field which will be skipped without one deal with. + */ +public class ServerInProtocolWrapper extends AbstractProtocolWrapper { + private static final ILog LOGGER = LogManager.getLogger(ServerInProtocolWrapper.class); + private static final StringTag TAG_ARGS = new StringTag("args"); + private AbstractContext context; + + public ServerInProtocolWrapper(final TProtocol protocol) { + super(protocol); + } + + public void initial(AbstractContext context) { + this.context = context; + } + + @Override + public TField readFieldBegin() throws TException { + final TField field = super.readFieldBegin(); + if (field.id == SW_MAGIC_FIELD_ID && field.type == TType.MAP) { + try { + TMap tMap = super.readMapBegin(); + Map<String, String> header = new HashMap(tMap.size); + + for (int i = 0; i < tMap.size; i++) { + header.put(readString(), readString()); + } + + super.readMessageEnd(); + super.readFieldEnd(); + + AbstractSpan span = ContextManager.createEntrySpan( + context.getOperatorName(), createContextCarrier(header)); + span.start(context.startTime); + span.tag(TAG_ARGS, context.getArguments()); + span.setComponent(ComponentsDefine.THRIFT_SERVER); + SpanLayer.asRPCFramework(span); + } catch (Throwable throwable) { + LOGGER.error("Failed to create EntrySpan.", throwable); + } finally { + context = null; + return readFieldBegin(); + } + } + return field; + } + + private ContextCarrier createContextCarrier(Map<String, String> header) { + ContextCarrier carrier = new ContextCarrier(); + if (Objects.nonNull(header)) { + CarrierItem items = carrier.items(); + while (items.hasNext()) { + items = items.next(); + items.setHeadValue(header.get(items.getHeadKey())); + } + } + return carrier; + } + + @Override + public TMessage readMessageBegin() throws TException { + final TMessage message = super.readMessageBegin(); + try { + context.setup(message.name); + } finally { + return message; Review comment: *Finally:* If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead. ########## File path: apm-sniffer/apm-sdk-plugin/thrift-plugin/src/main/java/org/apache/skywalking/apm/plugin/thrift/client/TServiceClientInterceptor.java ########## @@ -0,0 +1,111 @@ +/* + * 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.thrift.client; + +import java.lang.reflect.Method; +import java.util.Objects; +import org.apache.skywalking.apm.agent.core.context.ContextManager; +import org.apache.skywalking.apm.agent.core.context.tag.StringTag; +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.InstanceConstructorInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.InstanceMethodsAroundInterceptor; +import org.apache.skywalking.apm.agent.core.plugin.interceptor.enhance.MethodInterceptResult; +import org.apache.skywalking.apm.network.trace.component.ComponentsDefine; +import org.apache.skywalking.apm.plugin.thrift.commons.ReflectionUtils; +import org.apache.skywalking.apm.plugin.thrift.wrapper.ClientOutProtocolWrapper; +import org.apache.thrift.TBase; +import org.apache.thrift.TFieldIdEnum; +import org.apache.thrift.TServiceClient; +import org.apache.thrift.protocol.TProtocol; + +/** + * @see TServiceClient is synchronized client. + */ +public class TServiceClientInterceptor implements InstanceConstructorInterceptor, InstanceMethodsAroundInterceptor { + private static final StringTag TAG_ARGS = new StringTag("args"); + + @Override + public void onConstruct(EnhancedInstance objInst, + Object[] allArguments) throws NoSuchFieldException, IllegalAccessException { + if (!(allArguments[1] instanceof ClientOutProtocolWrapper)) { + TProtocol protocol = (TProtocol) allArguments[1]; + ReflectionUtils.setValue( + TServiceClient.class, + objInst, + "oprot_", + new ClientOutProtocolWrapper(protocol) + ); + Object dynamicField = ((EnhancedInstance) protocol.getTransport()).getSkyWalkingDynamicField(); + objInst.setSkyWalkingDynamicField(Objects.isNull(dynamicField) ? "UNKNOWN" : dynamicField); + } + } + + @Override + public void beforeMethod(EnhancedInstance objInst, + Method method, + Object[] allArguments, + Class<?>[] argumentsTypes, + MethodInterceptResult result) throws Throwable { + AbstractSpan span = ContextManager.createExitSpan( + objInst.getClass().getName() + "." + allArguments[0], + (String) objInst.getSkyWalkingDynamicField() + ); + SpanLayer.asRPCFramework(span); + span.setComponent(ComponentsDefine.THRIFT_CLIENT); + span.tag(TAG_ARGS, getArguments((String) allArguments[0], (TBase) allArguments[1])); + } + + @Override + public Object afterMethod(EnhancedInstance objInst, + Method method, + Object[] allArguments, + Class<?>[] argumentsTypes, + Object ret) throws Throwable { + return ret; + } + + @Override + public void handleMethodException(EnhancedInstance objInst, + Method method, + Object[] allArguments, + Class<?>[] argumentsTypes, + Throwable t) { + if (ContextManager.isActive()) { + ContextManager.activeSpan().errorOccurred().log(t); + } + } + + private String getArguments(String method, TBase base) { + int idx = 0; + StringBuffer buffer = new StringBuffer(method).append("("); Review comment: *JdkObsolete:* StringBuffer performs synchronization that is usually unnecessary; prefer StringBuilder. ---------------------------------------------------------------- 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: us...@infra.apache.org
