Here's an initial implementation of the tracing. Some more annotations and instrumentation could be added if needed.

WIP path.

This one has testing for the read and write paths started.

Here's an example of what I have currently. I'm still trying to balance what should be instrumented.

Instrumented Sender and Receiver (Though some of those code paths are not hit as well).
better read side instrumentation.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12606051/HDFS-5274-2.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.server.namenode.TestEditLogJournalFailures
org.apache.hadoop.hdfs.qjournal.TestNNWithQJM

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5072//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5072//console

This message is automatically generated.

Fix for tests.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12606168/HDFS-5274-3.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5078//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5078//console

This message is automatically generated.

Here's a lot more rigorous testing.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12607280/HDFS-5274-4.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

-1 javac. The patch appears to cause the build to fail.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5131//console

This message is automatically generated.

Another example

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12607290/HDFS-5274-5.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 1 new or modified test files.

-1 javac. The patch appears to cause the build to fail.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5133//console

This message is automatically generated.

Here's a patch that adds annotations for DFSInputStream.seek

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12607415/HDFS-5274-6.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. The javadoc tool did not generate any warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/5143//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/5143//console

This message is automatically generated.

Ping ?

This facility is excellent.

A few notes on the patch.

Long line and formatting seems off here:

+      if (header.hasTraceInfo() && header.getTraceInfo() != null && header.getTraceInfo().hasTraceId()) {
+        String traceDescription = rpcRequest.toString();
+
+        // If the incoming RPC included tracing info, always continue the trace
+          TraceInfo parentSpan = new TraceInfo(header.getTraceInfo().getTraceId(),
+              header.getTraceInfo().getParentId());
+          traceSpan = Trace.startSpan(traceDescription, parentSpan).detach();
+        }

This is a javadoc fix?

• * @param buf - SASL wrapped request of one or more RPCs
  + * @param inBuf - SASL wrapped request of one or more RPCs

Are you breaking your pattern of putting trace string building behind a check that tracing is enabled in the below?

+ TraceScope traceSpan = Trace.startSpan("Call " + method.getName() + " to " +
+ remoteId, CLIENT_TRACE_SAMPLER);

nit: declare and define all in the one line rather than do initialization in constructor: + receivers = new HashSet<SpanReceiver>();

Any reason we take config on construction and in init for SpanReceiverHost?

SpanReceiverHost is on only when trace is enabled, right? If so, say so in class comment.

Has to be a shutdown hook? ShutdownHookManager.get().addShutdownHook ? This is fine unless we envision someone having to override it which I suppose should never happen for an optionally enabled, rare, trace function?

HTraceConfiguration is for testing only? Should be @visiblefortesting only or a comment at least?

nit: Do Classname.simple() here instead? + TraceScope scope = Trace.startSpan(DFSInputStream.class.getSimpleName());

Should there be defines for a few of these? "DFSInputStream.close" seems fine... only used once DFSInputStream.read?

Is there something off in the formatting here?

• private static class Packet {
• private static final long HEART_BEAT_SEQNO = -1L;
• long seqno; // sequencenumber of buffer in block
• final long offsetInBlock; // offset in block
• boolean syncBlock; // this packet forces the current block to disk
• int numChunks; // number of chunks currently in packet
• final int maxChunks; // max chunks in packet
  + private Span traceSpan = null;
  +
  + private static class Packet {
  + private static final long HEART_BEAT_SEQNO = -1L;
  + long seqno; // sequencenumber of buffer in block
  + final long offsetInBlock; // offset in block
  + boolean syncBlock; // this packet forces the current block to disk
  + int numChunks; // number of chunks currently in packet
  + final int maxChunks; // max chunks in packet

Or is it just git cleaning whitespace?

Gratuitous change?

• ClientOperationHeaderProto header =
  + ClientOperationHeaderProto.Builder builder =
  ClientOperationHeaderProto.newBuilder()
  .setBaseHeader(buildBaseHeader(blk, blockToken))
• .setClientName(client)
• .build();
• return header;
  + .setClientName(client);
  + return builder.build();

Is an annotation of 'J' only intentional?

+ if (traceSpan != null) {
+ traceSpan.addTimelineAnnotation("J");

Put inside a isTracing check? + Trace.addTimelineAnnotation("Connecting to downstream " + mirrorNode);

ditto: + Trace.addTimelineAnnotation("Waiting for connect ack from downstream");

Hi Elliott Clark,
Are you working on this issue now?
If you do not have enough time, I would like to help fixing and rebasing this patch.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12607415/HDFS-5274-6.patch
against trunk revision .

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6158//console

This message is automatically generated.

Masatake Iwasaki I'm pretty sure Elliott would be happy if you took it. I can help (not sure if that a hinderance or a help – smile).

Thanks stack! I'm on it.

I am attaching the patch rebased and updated based on review comments.

Any reason we take config on construction and in init for SpanReceiverHost?

I removed conf from constructor argument.

SpanReceiverHost is on only when trace is enabled, right? If so, say so in class comment.

SpanReceiverHost is always on, though it do nothing if no SpanReceiver is configured. I added a line in class comment.

Has to be a shutdown hook? ShutdownHookManager.get().addShutdownHook ? This is fine unless we envision someone having to override it which I suppose should never happen for an optionally enabled, rare, trace function?

Overriding SpanReceiverHost is not necessary, though there could be someone who implement SpanReceiver. I think it is useful to wait for receivers to process all the tracing data on crash scenario.

HTraceConfiguration is for testing only? Should be @visiblefortesting only or a comment at least?

HTraceConfiguration is used by SpanReceiver implementation, not for testing only.

Should there be defines for a few of these? "DFSInputStream.close" seems fine... only used once DFSInputStream.read?

I think it is fine not to define "DFSInputStream.read" now.

There are some fixes in addition to above such as,

• removed timing dependency from TestTracing.
• added guard by Trace.isTracing() around startSpan() in DFSInputStream, FsShell and WritableRpcEngine.
• removed SpanReceiverHost from FsShell and DFSClient. I will add options or config properties to turn on tracing from shell later on another JIRA issue.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12629828/HDFS-5274-7.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.server.namenode.ha.TestHASafeMode
org.apache.hadoop.hdfs.server.namenode.TestCacheDirectives

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6174//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6174//console

This message is automatically generated.

My guess is that the failures are unrelated. We can rerun the patch or just wait on next iteration.

Patch looks great to me. Have you tried it outside of the unit tests to make sure you get sensible looking spans and numbers? Perhaps I can help here?

Fix these in next patch:

"+ * This class do nothing If no SpanReceiver is configured ."

"+ * Trancing information of HTrace, if exists."

Is formatting ok here?

+ if (source != null)

+ send(out, Op.WRITE_BLOCK, proto.build());
+ } finally

In BlockReceiver, should traceSpan be getting closed?

Is it possible that below throws an exception?

+ scope.getSpan().addKVAnnotation(
+ "stream".getBytes(),
+ jas.getCurrentStream().toString().getBytes());

i..e. we can hope out w/o closing the span since the try/finally only happens later.

This is in JournalSet in a few places.

TraceInfo and RPCTInfo seem to be same datastructure? Should we define it onetime only and share?'

Hi stack, thanks for your review comments.

Have you tried it outside of the unit tests to make sure you get sensible looking spans and numbers?

I checked the trace of putting and getting a big file by Zipkin today. There seems to be too many spans concerning "DFSInputStream.read" and "DFSOutputStream.write". I will fix this in the next version of patch.

Is there any plan on making HTrace libraries org.cloudera.htrace.* available in Hadoop common?

Hi Suresh. As one of the primary authors of HTrace I'd say that there are no plans to put it in Hadoop Common. Personally I think Hadoop Common as a grab-bag of all things Hadoop gets pretty messy, since it's harder to piece-meal upgrade different components. It also means that dependent apps like HBase would need to wait on new versions of Common and have even more difficulty building the same code against different versions of Hadoop.

Happy to take contributions to HTrace via github pull request if you like, though.

Masatake Iwasaki Thanks. When you are done, I'll try hooking it up w/ hbase to make sure we get a trace that spans the two systems.

Suresh Srinivas Wouldn't adding htrace to the common pom.xml make it "...available in Hadoop common"? Thanks.

Wouldn't adding htrace to the common pom.xml make it "...available in Hadoop common"? Thanks.

I agree with the comment Doug Cutting had made - https://issues.apache.org/jira/browse/HADOOP-10311?focusedCommentId=13886809&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13886809. I would love to see the code in Hadoop community itself, if possible. Agreed, this is not different using google guava or other such libraries. But I am afraid this could start a trend of capabilities hosted/attributed especially to vendor companies making its way into Hadoop.

With that said, I am -0 and would rather not see this becoming a trend.

Masatake Iwasaki You know, I was thinking... Maybe it ok that there are so many spans? Tracing doesn't cost unless enabled. When debugging, you might want to see in the trace that HDFS is doing a bunch of small reads?

I am attaching updated patch and screen shots of trace of putting and getting a 200MB file.

Fix these in next patch:

fixed.

Is formatting ok here?

fixed.

In BlockReceiver, should traceSpan be getting closed?

added description to span and calling close().

Is it possible that below throws an exception?

+ scope.getSpan().addKVAnnotation(
+ "stream".getBytes(),
+ jas.getCurrentStream().toString().getBytes());

i..e. we can hope out w/o closing the span since the try/finally only happens later.

This is in JournalSet in a few places.

I moved these code in try block to make sure.

TraceInfo and RPCTInfo seem to be same datastructure? Should we define it onetime only and share?'

I prefer keeping this as is because of simplicity and independency between datatransfer protocol and o.a.h.ipc.

I checked the trace of putting and getting a big file by Zipkin today. There seems to be too many spans concerning "DFSInputStream.read" and "DFSOutputStream.write". I will fix this in the next version of patch.

just removed those spans from DFSInputStream and DFSOutputStream.

Masatake Iwasaki You know, I was thinking... Maybe it ok that there are so many spans? Tracing doesn't cost unless enabled. When debugging, you might want to see in the trace that HDFS is doing a bunch of small reads?

I just missed your comment on uploading v.8 patch.
Because there were so many spans than I expected and receiver's queue was filled, I think disabling those spans is safer as a starting point.

14/02/19 22:25:23 ERROR impl.ZipkinSpanReceiver: Error trying to append span (DFSOutputStream.write) to the queue.  Blocking Queue was full.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12630259/ss-5274v8-get.png
against trunk revision .

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6204//console

This message is automatically generated.

But I am afraid this could start a trend of capabilities hosted/attributed especially to vendor companies making its way into Hadoop.

Suresh Srinivas I just got perms on this lib. I'll go get a domain, convert the code package names, make a new release, and publish to maven central... will report back when done.

Thanks stack!

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12630393/HDFS-5274-8.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.hdfs.web.TestWebHDFS

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6208//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6208//console

This message is automatically generated.

I put up a SNAPSHOT of the tracing jar w/ its groupId changed from org.cloudera.htrace to org.htrace. The version is currently 3.0-SNAPSHOT (all is the same except for the redone domain). If it works for you Masatake Iwasaki, I'll make a release (if the former owners of this jar are good w/ my manglings). Thanks boss.

Thanks stack! It seems not yet to be visible in repo.. I will check again a few hours later.

Hi stack, it worked! Please make a release. I will attach a rebased patch later.

Hmm... Add this repository in the top level hadoop pom?

+ <repository>
+ <id>oss.sonatype</id>
+ <name>Sonatype OSS Repository</name>
+ <url>https://oss.sonatype.org/content/groups/public</url>
+ <snapshots>
+ <enabled>true</enabled>
+ </snapshots>
+ <releases>
+ <enabled>true</enabled>
+ </releases>
+ </repository>

That makes it work for me. Thanks Masatake Iwasaki

attaching updated patch.

• rebased to trunk,
• changed package name from org.cloudera.htrace to org.htrace,
• added repository to top level pom.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12631366/HDFS-5274-9.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The test build failed in hadoop-hdfs-project/hadoop-hdfs

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6249//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6249//console

This message is automatically generated.

Rebase and updated the patch to refer to released htrace (which is now in maven central so no need of the sonatype repo reference).

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12632181/HDFS-5274-10.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

-1 findbugs. The patch appears to introduce 3 new Findbugs (version 1.3.9) warnings.

-1 release audit. The applied patch generated 1 release audit warnings.

-1 core tests. The test build failed in hadoop-hdfs-project/hadoop-hdfs

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6274//testReport/
Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6274//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6274//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6274//console

This message is automatically generated.

+ Fix double-close in Server.java
+ Minor fix of javadoc comment
+ Fix few places where we were not testing if trace enabled.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12632352/HDFS-5274-11.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

-1 findbugs. The patch appears to introduce 6 new Findbugs (version 1.3.9) warnings.

-1 release audit. The applied patch generated 1 release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.tracing.TestTracing

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6281//testReport/
Release audit warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6281//artifact/trunk/patchprocess/patchReleaseAuditProblems.txt
Findbugs warnings: https://builds.apache.org/job/PreCommit-HDFS-Build/6281//artifact/trunk/patchprocess/newPatchFindbugsWarningshadoop-hdfs.html
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6281//console

This message is automatically generated.

fixed the test if tracing in Receiver.

added tests for the cases tracing disabled.

+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12632559/HDFS-5274-12.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

+1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6297//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6297//console

This message is automatically generated.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12632567/HDFS-5274-13.patch
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 4 new or modified test files.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

+1 javadoc. There were no new javadoc warning messages.

+1 eclipse:eclipse. The patch built with eclipse:eclipse.

+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

org.apache.hadoop.cli.TestAclCLI

+1 contrib tests. The patch passed contrib unit tests.

Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/6298//testReport/
Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6298//console

This message is automatically generated.

adding screen shots of tracing on 3node cluster: 3node_put_200mb.png and 3node_get_200mb.png

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12633485/3node_get_200mb.png
against trunk revision .

-1 patch. The patch command could not apply the patch.

Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/6349//console

This message is automatically generated.