szetszwo commented on code in PR #1385:
URL: https://github.com/apache/ratis/pull/1385#discussion_r2984046149
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -48,13 +50,21 @@ public final class TraceUtils {
private static final Logger LOG = LoggerFactory.getLogger(TraceUtils.class);
+ private static final RaftProperties PROPERTIES = new RaftProperties();
Review Comment:
We cannot `new RaftProperties()` here since it will get all the default
values.
Ratis is a library and does not manage any conf files. The RaftProperties is
passed from the application when building RaftClient/RaftServer
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -94,6 +104,36 @@ public static <T, THROWABLE extends Throwable>
CompletableFuture<T> traceAsyncMe
return enabled ? traceAsyncMethod(action, request, memberId, spanName) :
action.get();
}
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncImplSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server) throws THROWABLE {
+ return isTraceEnabled() ? traceAsyncClientSend(action, type, server,
"AsyncImpl::send") : action.get();
+ }
+
+ private static boolean isTraceEnabled() {
+ return TraceConfigKeys.enabled(PROPERTIES);
+ }
+
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncClientSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server, String spanName) throws
THROWABLE {
+ return traceAsyncMethod(action, () -> createClientOperationSpan(type,
server, spanName));
+ }
+
+ private static Span createClientOperationSpan(RaftClientRequest.Type type,
RaftPeerId server,
+ String spanName) {
+ String name = (spanName == null || spanName.isEmpty()) ? DEFAULT_OPERATION
: spanName;
Review Comment:
If the spanName is missing, should it be a bug? We probably should check
null but not setting DEFAULT_OPERATION.
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -94,6 +104,36 @@ public static <T, THROWABLE extends Throwable>
CompletableFuture<T> traceAsyncMe
return enabled ? traceAsyncMethod(action, request, memberId, spanName) :
action.get();
}
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncImplSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server) throws THROWABLE {
+ return isTraceEnabled() ? traceAsyncClientSend(action, type, server,
"AsyncImpl::send") : action.get();
+ }
+
+ private static boolean isTraceEnabled() {
+ return TraceConfigKeys.enabled(PROPERTIES);
+ }
+
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncClientSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server, String spanName) throws
THROWABLE {
+ return traceAsyncMethod(action, () -> createClientOperationSpan(type,
server, spanName));
+ }
+
+ private static Span createClientOperationSpan(RaftClientRequest.Type type,
RaftPeerId server,
+ String spanName) {
+ String name = (spanName == null || spanName.isEmpty()) ? DEFAULT_OPERATION
: spanName;
+ String peerId = server == null ? UNKNOWN_PEER : String.valueOf(server);
Review Comment:
When `server == null`, the client request will be sent to the leader. So,
let's use
```java
private static final String LEADER = "LEADER";
```
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -48,13 +50,21 @@ public final class TraceUtils {
private static final Logger LOG = LoggerFactory.getLogger(TraceUtils.class);
+ private static final RaftProperties PROPERTIES = new RaftProperties();
+ private static final String DEFAULT_OPERATION = "DEFAULT_OPERATION";
+ private static final String UNKNOWN_PEER = "UNKNOWN_PEER";
+
private TraceUtils() {
}
public static Tracer getGlobalTracer() {
return TRACER;
}
+ public static void setTracingEnabled(boolean enabled) {
+ TraceConfigKeys.setEnabled(PROPERTIES, enabled);
+ }
Review Comment:
Let's change TRACER to AtomicReference. Then, it is enabled if it is not
null.
```java
private static final AtomicReference<Tracer> TRACER = new
AtomicReference<>();
public static void setEnabled(RaftProperties properties) {
setEnabled(TraceConfigKeys.enabled(properties));
}
public static void setEnabled(boolean enabled) {
if (enabled) {
TRACER.updateAndGet(previous -> previous != null ? previous
: GlobalOpenTelemetry.getTracer("org.apache.ratis",
VersionInfo.getSoftwareInfoVersion()));
} else {
TRACER.set(null);
}
}
static boolean isEnabled() {
return TRACER.get() != null;
}
```
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -94,6 +104,36 @@ public static <T, THROWABLE extends Throwable>
CompletableFuture<T> traceAsyncMe
return enabled ? traceAsyncMethod(action, request, memberId, spanName) :
action.get();
}
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncImplSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server) throws THROWABLE {
+ return isTraceEnabled() ? traceAsyncClientSend(action, type, server,
"AsyncImpl::send") : action.get();
Review Comment:
- Let's check enabled in traceAsyncMethod(..).
```java
@@ -62,6 +78,10 @@ public final class TraceUtils {
*/
static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncMethod(
CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
Supplier<Span> spanSupplier) throws THROWABLE {
+ if (!isEnabled()) {
+ return action.get();
+ }
```
##########
ratis-common/src/main/java/org/apache/ratis/trace/RatisAttributes.java:
##########
@@ -29,6 +29,10 @@ public final class RatisAttributes {
public static final AttributeKey<String> MEMBER_ID =
AttributeKey.stringKey("raft.member.id");
public static final AttributeKey<String> CALL_ID =
AttributeKey.stringKey("raft.call.id");
+ public static final AttributeKey<String> PEER_ID =
AttributeKey.stringKey("rpc.peer.id");
+ public static final AttributeKey<String> OPERATION_NAME =
AttributeKey.stringKey("ratis.operation.name");
+ public static final AttributeKey<String> OPERATION_TYPE =
AttributeKey.stringKey("ratis.operation.type");
Review Comment:
Let's keep using "raft" prefix.
```java
public static final AttributeKey<String> PEER_ID =
AttributeKey.stringKey("raft.peer.id");
public static final AttributeKey<String> OPERATION_NAME =
AttributeKey.stringKey("raft.operation.name");
public static final AttributeKey<String> OPERATION_TYPE =
AttributeKey.stringKey("raft.operation.type");
```
##########
ratis-common/src/main/java/org/apache/ratis/trace/TraceUtils.java:
##########
@@ -94,6 +104,36 @@ public static <T, THROWABLE extends Throwable>
CompletableFuture<T> traceAsyncMe
return enabled ? traceAsyncMethod(action, request, memberId, spanName) :
action.get();
}
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncImplSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server) throws THROWABLE {
+ return isTraceEnabled() ? traceAsyncClientSend(action, type, server,
"AsyncImpl::send") : action.get();
+ }
+
+ private static boolean isTraceEnabled() {
+ return TraceConfigKeys.enabled(PROPERTIES);
+ }
+
+ public static <T, THROWABLE extends Throwable> CompletableFuture<T>
traceAsyncClientSend(
+ CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
+ RaftClientRequest.Type type, RaftPeerId server, String spanName) throws
THROWABLE {
+ return traceAsyncMethod(action, () -> createClientOperationSpan(type,
server, spanName));
+ }
Review Comment:
- We may combine traceAsyncClientSend and traceAsyncImplSend.
- BTW, let move the client related code to a new TraceClient class. We
should also move the server related code to a new TraceServer class. Then,
TraceUtils will contain only the common methods.
```java
public static <T, THROWABLE extends Throwable> CompletableFuture<T>
asyncSend(
CheckedSupplier<CompletableFuture<T>, THROWABLE> action,
RaftClientRequest.Type type, RaftPeerId server) throws THROWABLE {
return TraceUtils.traceAsyncMethod(action, () ->
createClientOperationSpan(type, server, "Async::send"));
}
```
--
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]
