RATIS-2397. Add trace support for Log Appender#1443
RATIS-2397. Add trace support for Log Appender#1443taklwu wants to merge 6 commits intoapache:masterfrom
Conversation
taklwu
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@taklwu , thanks for working on this!
Please see the comments inlined and also https://issues.apache.org/jira/secure/attachment/13082105/1443_review.patch
| : TraceUtils.extractContextFromProto(spanContext); | ||
| return TraceUtils.traceAsyncMethod(action, () -> { | ||
| final Span span = TraceUtils.getGlobalTracer() | ||
| .spanBuilder("raft.server.appendEntriesAsync") |
There was a problem hiding this comment.
Let's add a static constant for "raft.server.appendEntriesAsync".
There was a problem hiding this comment.
I made a new class SpanNames and add this into it, also it comes with other refactoring
| for (LogEntryProto e : entries) { | ||
| final SpanContextProto sc = traceByIndex.get(e.getIndex()); | ||
| if (sc != null && !sc.getContextMap().isEmpty()) { | ||
| return sc; | ||
| } | ||
| } |
There was a problem hiding this comment.
Since entries is a list, there could be multiple PendingRequest(s). Should we return multiple SpanContextProto(s)?
There was a problem hiding this comment.
AFAIK that AppendEntriesRequestProto is a single RPC with a batch of append entries, so, we should only have a single parent spancontext for this batch.
| for (LogEntryProto e : entries) { | ||
| final SpanContextProto sc = traceByIndex.get(e.getIndex()); |
There was a problem hiding this comment.
Since it needs to loop for entries in consecutive indices, it is more efficient to use a NavigableMap. I suggest to use TreeMap with a read-write lock.
There was a problem hiding this comment.
ack, I followed your change.
| private final ConcurrentHashMap<Long, SpanContextProto> replicationTraceByLogIndex = | ||
| new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Let's create a new class, say LeaderTracer, and move all the tracing code there.
1. use LeaderTracer 2. refactor constants
taklwu
left a comment
taklwu
left a comment
There was a problem hiding this comment.
thanks for your time! and please review again.
| : TraceUtils.extractContextFromProto(spanContext); | ||
| return TraceUtils.traceAsyncMethod(action, () -> { | ||
| final Span span = TraceUtils.getGlobalTracer() | ||
| .spanBuilder("raft.server.appendEntriesAsync") |
There was a problem hiding this comment.
I made a new class SpanNames and add this into it, also it comes with other refactoring
| for (LogEntryProto e : entries) { | ||
| final SpanContextProto sc = traceByIndex.get(e.getIndex()); | ||
| if (sc != null && !sc.getContextMap().isEmpty()) { | ||
| return sc; | ||
| } | ||
| } |
There was a problem hiding this comment.
AFAIK that AppendEntriesRequestProto is a single RPC with a batch of append entries, so, we should only have a single parent spancontext for this batch.
| for (LogEntryProto e : entries) { | ||
| final SpanContextProto sc = traceByIndex.get(e.getIndex()); |
There was a problem hiding this comment.
ack, I followed your change.
|
|
2 participants
What changes were proposed in this pull request?
Adding tracing support for AppendEntries / AppendEntriesAsync
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2397
How was this patch tested?