szetszwo commented on code in PR #1341: URL: https://github.com/apache/ratis/pull/1341#discussion_r2874084628
########## ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.ratis.trace; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.context.Context; +import io.opentelemetry.context.propagation.TextMapPropagator; +import io.opentelemetry.context.propagation.TextMapGetter; +import org.apache.ratis.proto.RaftProtos; +import org.apache.ratis.util.VersionInfo; + +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +public final class TraceUtils { + + private TraceUtils() { + } + + public static Tracer getGlobalTracer() { + return GlobalOpenTelemetry.getTracer("org.apache.ratis", VersionInfo.getSoftwareInfoVersion()); Review Comment: Add a static field, so we don't have to create the same Tracer multiple times. ```java private static final Tracer TRACER = GlobalOpenTelemetry.getTracer("org.apache.ratis", VersionInfo.getSoftwareInfoVersion()); ``` ########## pom.xml: ########## @@ -189,6 +189,9 @@ <shaded.protobuf.version>3.25.8</shaded.protobuf.version> <shaded.grpc.version>1.75.0</shaded.grpc.version> + <!-- OpenTelemetry versions --> + <opentelemetry.version>1.57.0</opentelemetry.version> + <opentelemetry-semconv.version>1.37.0</opentelemetry-semconv.version> Review Comment: How about using the latest 1.59.0 and 1.40.0? ########## ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.ratis.trace; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.context.Context; +import io.opentelemetry.context.propagation.TextMapPropagator; +import io.opentelemetry.context.propagation.TextMapGetter; +import org.apache.ratis.proto.RaftProtos; +import org.apache.ratis.util.VersionInfo; + +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +public final class TraceUtils { + + private TraceUtils() { + } + + public static Tracer getGlobalTracer() { + return GlobalOpenTelemetry.getTracer("org.apache.ratis", VersionInfo.getSoftwareInfoVersion()); + } + + /** + * Create a span which parent is from remote, i.e, passed through rpc. + * </p> + * We will set the kind of the returned span to {@link SpanKind#SERVER}, as this should be the top + * most span at server side. + */ + public static Span createRemoteSpan(String name, Context ctx) { + return getGlobalTracer().spanBuilder(name).setParent(ctx).setSpanKind(SpanKind.SERVER) + .startSpan(); + } + + private static final TextMapPropagator PROPAGATOR = + GlobalOpenTelemetry.getPropagators().getTextMapPropagator(); + + public static RaftProtos.SpanContextProto injectContextToProto(Context context) { + Map<String, String> carrier = new HashMap<>(); + PROPAGATOR.inject(context, carrier, (map, key, value) -> map.put(key, value)); Review Comment: Question: What exactly will be injected, ATTR_MEMBER_ID and ATTR_CALLER_ID? Anything else? ########## ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.ratis.trace; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.context.Context; +import io.opentelemetry.context.propagation.TextMapPropagator; +import io.opentelemetry.context.propagation.TextMapGetter; +import org.apache.ratis.proto.RaftProtos; Review Comment: Import SpanContextProto instead. The code will be easier to read. ```java import org.apache.ratis.proto.RaftProtos.SpanContextProto; ``` ########## ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java: ########## @@ -0,0 +1,34 @@ +/* + * 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.ratis.trace; + +import io.opentelemetry.api.common.AttributeKey; + +/** + * The constants in this class correspond with the guidance outlined by the OpenTelemetry <a href= + * "https://github.com/open-telemetry/semantic-conventions";>Semantic + * Conventions</a>. + */ +public final class RatisAttributes { + public static final AttributeKey<String> ATTR_MEMBER_ID = AttributeKey.stringKey("raft.member.id"); + public static final AttributeKey<String> ATTR_CALLER_ID = AttributeKey.stringKey("raft.caller.id"); Review Comment: Is it the same as [requestorId](https://github.com/apache/ratis/blob/a5651f6e9a95f5b5ac44ff01363b7de1c8b4c026/ratis-proto/src/main/proto/Raft.proto#L114) ? If yes, let's use "raft.requestor.id". ########## ratis-common/src/main/java/org/apache/ratis/protocol/RaftClientRequest.java: ########## @@ -397,6 +404,8 @@ public static RaftClientRequest toWriteRequest(RaftClientRequest r, Message mess private final boolean toLeader; + private SpanContextProto spanContext; Review Comment: Please add `final`. ########## ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java: ########## @@ -0,0 +1,85 @@ +/* + * 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.ratis.trace; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.trace.Span; +import io.opentelemetry.api.trace.SpanKind; +import io.opentelemetry.api.trace.Tracer; +import io.opentelemetry.context.Context; +import io.opentelemetry.context.propagation.TextMapPropagator; +import io.opentelemetry.context.propagation.TextMapGetter; +import org.apache.ratis.proto.RaftProtos; +import org.apache.ratis.util.VersionInfo; + +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +public final class TraceUtils { + + private TraceUtils() { + } + + public static Tracer getGlobalTracer() { + return GlobalOpenTelemetry.getTracer("org.apache.ratis", VersionInfo.getSoftwareInfoVersion()); + } + + /** + * Create a span which parent is from remote, i.e, passed through rpc. + * </p> + * We will set the kind of the returned span to {@link SpanKind#SERVER}, as this should be the top + * most span at server side. + */ + public static Span createRemoteSpan(String name, Context ctx) { + return getGlobalTracer().spanBuilder(name).setParent(ctx).setSpanKind(SpanKind.SERVER) + .startSpan(); + } + + private static final TextMapPropagator PROPAGATOR = + GlobalOpenTelemetry.getPropagators().getTextMapPropagator(); + + public static RaftProtos.SpanContextProto injectContextToProto(Context context) { + Map<String, String> carrier = new HashMap<>(); Review Comment: For String, would TreeMap be better? It will also sort them. ########## ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java: ########## @@ -943,16 +949,44 @@ CompletableFuture<RaftClientReply> executeSubmitClientRequestAsync(RaftClientReq @Override public CompletableFuture<RaftClientReply> submitClientRequestAsync( RaftClientRequest request) throws IOException { - assertLifeCycleState(LifeCycle.States.RUNNING); - LOG.debug("{}: receive client request({})", getMemberId(), request); - final Timekeeper timer = raftServerMetrics.getClientRequestTimer(request.getType()); - final Optional<Timekeeper.Context> timerContext = Optional.ofNullable(timer).map(Timekeeper::time); - return replyFuture(request).whenComplete((clientReply, exception) -> { - timerContext.ifPresent(Timekeeper.Context::stop); - if (exception != null || clientReply.getException() != null) { - raftServerMetrics.incFailedRequestCount(request.getType()); - } - }); + final Context remoteContext = TraceUtils.extractContextFromProto(request.getSpanContext()); + final Span span = TraceUtils.createRemoteSpan("raft.server.submitClientRequestAsync", remoteContext); + span.setAttribute(RatisAttributes.ATTR_MEMBER_ID, getMemberId().toString()); + span.setAttribute(RatisAttributes.ATTR_CALLER_ID, String.valueOf(request.getCallId())); + try (Scope ignored = span.makeCurrent()) { + assertLifeCycleState(LifeCycle.States.RUNNING); + LOG.debug("{}: receive client request({})", getMemberId(), request); + final Timekeeper timer = raftServerMetrics.getClientRequestTimer(request.getType()); + final Optional<Timekeeper.Context> timerContext = Optional.ofNullable(timer).map(Timekeeper::time); + span.addEvent("Processing client request"); + final CompletableFuture<RaftClientReply> future = replyFuture(request); + return future.whenComplete((clientReply, exception) -> { + try (Scope completeScope = span.makeCurrent()) { + timerContext.ifPresent(Timekeeper.Context::stop); + if (exception != null || (clientReply != null && clientReply.getException() != null)) { + raftServerMetrics.incFailedRequestCount(request.getType()); + } + if (exception != null) { + span.recordException(exception); + span.setStatus(StatusCode.ERROR, exception.getMessage()); + } else if (clientReply != null && clientReply.getException() != null) { + span.recordException(clientReply.getException()); + span.setStatus(StatusCode.ERROR, clientReply.getException().getMessage()); + } + } finally { + span.addEvent("Completed client request"); + span.end(); + } + }); + } catch (Exception | Error e) { + // this catch block is for exceptions thrown before the future is returned. + // Any exception thrown after the future is returned should be handled in + // the whenComplete callback above. + span.recordException(e); + span.setStatus(StatusCode.ERROR, e.getMessage()); + span.end(); + throw e; + } Review Comment: Let add traceAsyncMethod(..) to TraceUtils and move the opentelemetry code there. -- 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]
