ctubbsii commented on code in PR #5691: URL: https://github.com/apache/accumulo/pull/5691#discussion_r2191090449
########## core/src/main/java/org/apache/accumulo/core/rpc/AccumuloProtocolFactory.java: ########## @@ -0,0 +1,238 @@ +/* + * 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 + * + * https://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.accumulo.core.rpc; + +import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.data.InstanceId; +import org.apache.accumulo.core.trace.TraceUtil; +import org.apache.thrift.TException; +import org.apache.thrift.protocol.TCompactProtocol; +import org.apache.thrift.protocol.TMessage; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.transport.TTransport; + +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.context.Scope; + +/** + * Factory for creating instances of the AccumuloProtocol. + * <p> + * This protocol includes a custom header to ensure compatibility between different versions of the + * protocol. It also traces RPC calls. + */ +public class AccumuloProtocolFactory extends TCompactProtocol.Factory { + + private static final long serialVersionUID = 1L; + + private final boolean isClient; + private final InstanceId instanceId; + + public static class AccumuloProtocol extends TCompactProtocol { + + static final int MAGIC_NUMBER = 0x41434355; // "ACCU" in ASCII + static final byte PROTOCOL_VERSION = 1; + + private final boolean isClient; + private final InstanceId instanceId; + + private Span span = null; + private Scope scope = null; + + public AccumuloProtocol(TTransport transport, InstanceId instanceId, boolean isClient) { + super(transport); + this.instanceId = instanceId; + this.isClient = isClient; + } + + /** + * For client calls, write the Accumulo protocol header before writing the message + */ + @Override + public void writeMessageBegin(TMessage message) throws TException { + span = TraceUtil.startClientRpcSpan(this.getClass(), message.name); + scope = span.makeCurrent(); Review Comment: I was trying to figure out if we need to do any tracing stuff in here at all. Then, I see that this got carried over from the TraceProtocolFactory, and that led me down a rabbit hole: 1. I think this protocol method gets called when the server creates a return message with the results of an operation. So, it's very confusing that this TraceUtil method creates a span of SpanKind.CLIENT, when it is not always the case. 2. The SpanKind.CLIENT type is supposed to wrap spans that cross process boundaries. I don't think this one does. It appears that writeMessageBegin and writeMessageEnd all occur on the senders side, before anything happens on the remote side. So, I think this is creating the wrong SpanKind. I think this is a bug that got introduced with the predecessor TraceProtocolFactory due to a misunderstanding of when these methods get called by Thrift. 3. We don't seem to actually have a client side invocation handler that would actually be the best place to put a SpanKind.CLIENT span type like we do on the server side. -- 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]
