You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "taklwu (via GitHub)" <gi...@apache.org> on 2026/03/23 18:12:16 UTC
[PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
taklwu opened a new pull request, #1385:
URL: https://github.com/apache/ratis/pull/1385
## What changes were proposed in this pull request?
After RATIS-2393 / #1341, this change aims to provide the first end-to-end tracing triggered from the any async implementation when sending a rpc request.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2395
## How was this patch tested?
unit tests and extend existing tests in ratis-test module with feature enabled.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
Posted by "taklwu (via GitHub)" <gi...@apache.org>.
taklwu commented on code in PR #1385:
URL: https://github.com/apache/ratis/pull/1385#discussion_r2989502961
##########
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:
got it, I will create the tracer in the client and server when reading the properties.
##########
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:
The suggestion is good, I should have refactored them and keep the new classes in ratis-common
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1385:
URL: https://github.com/apache/ratis/pull/1385
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1385:
URL: https://github.com/apache/ratis/pull/1385#issuecomment-4129351268
@taklwu , checkstyle failed. Please take a look.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2395. Add Client Span for AyncImpl [ratis]
Posted by "taklwu (via GitHub)" <gi...@apache.org>.
taklwu commented on PR #1385:
URL: https://github.com/apache/ratis/pull/1385#issuecomment-4129679697
@szetszwo there was a leftover unused import, fixed.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org