HPCC4J-611 Add OpenTelemetry tracing to dfsclient #722

jpmcmu · 2024-07-10T13:28:59Z

Updated event attribute reporting
Fixed trace structure in dfsclient protocol
Added tracing to FileUtility

Signed-off-by: James McMullan [email protected]

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change is a breaking change (fix or feature that will cause existing behavior to change).

Checklist:

Testing:

github-actions · 2024-07-10T13:29:24Z

Jira Issue: https://hpccsystems.atlassian.net/browse/HPCC4J-611

Jirabot Action Result:
Workflow Transition: Merge Pending
Additional PR: #722

jpmcmu · 2024-07-16T20:07:47Z

@rpastrana Wrapped up code review changes, please review

rpastrana

@jpmcmu looks good, but left you a few suggestions. Let's discuss.

rpastrana · 2024-07-16T21:12:39Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

@@ -129,6 +161,15 @@ public boolean hasOperation()

        public void startOperation(String operationName)
        {
+            if (taskSpan != null)
+            {
+                taskSpan = Utils.createChildSpan(taskSpan, operationName);


does it makes sense to have a single Utils.createSpan(parentSpan, spanName)? If parentSpan != null, it sets it as the parent

rpastrana · 2024-07-16T21:15:01Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+            }
+
+            taskSpan.end();
+
            long totalOperationTime = System.nanoTime();
            totalOperationTime -= operationStart;



some of these context values seem worth reporting in the span

rpastrana · 2024-07-16T21:38:19Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

@@ -1095,7 +1149,8 @@ private static void performRead(String[] args, TaskContext context)
        for (int i = 0; i < datasets.length; i++)
        {
            String datasetName = datasets[i];
-            context.startOperation("Read " + datasetName);
+            context.startOperation("FileUtility.Read_" + datasetName);
+            context.setTaskSpanAttributes(Attributes.of(AttributeKey.stringKey("server.url"), connString));


is there a way to add individual attributes after this initial set attributes call?

did we expose addTaskSpanAttribute(Attribute)?

rpastrana · 2024-07-16T21:39:36Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

-        context.startOperation("Read Test " + datasetName);
+        context.startOperation("FileUtility.ReadTest_" + datasetName);
+
+        context.setTaskSpanAttributes(Attributes.of(AttributeKey.stringKey("server.url"), connString));


Are there other attributes we might want to report to help track issues?

rpastrana · 2024-07-16T21:40:39Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

@@ -1463,7 +1521,9 @@ private static void performCopy(String[] args, TaskContext context)
            String srcFile = copyPairs[i];
            String destFile = copyPairs[i+1];

-            context.startOperation("Copy " + srcFile + " -> " + destFile);
+            context.startOperation("FileUtility.Copy_ " + srcFile + " -> " + destFile);
+            context.setTaskSpanAttributes(Attributes.of(AttributeKey.stringKey("server.src.url"), srcURL,


does the context.staroperation call add any relevant attributes to the span?

rpastrana · 2024-07-16T21:43:14Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/HPCCRemoteFileWriter.java

+
+    private static FileWriteContext constructReadContext(FieldDef recordDef, CompressionAlgorithm fileCompression, int connectTimeoutMs, int socketOpTimeoutMs)
+    {
+        FileWriteContext context = new FileWriteContext();


do all/some of these end up tracked in a span?

dfsclient/src/main/java/org/hpccsystems/dfs/client/Utils.java

rpastrana · 2024-07-16T21:56:14Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/Utils.java

    }

    public static Span createSpan(String name)
    {
-        return Utils.getTelemetryTracer().spanBuilder(name)
+        Span span = Utils.getTelemetryTracer().spanBuilder(name)


can we combine the two functions?
Span createSpan(String name, Span parent)

are we combining these 2?

jpmcmu · 2024-07-19T15:34:30Z

@rpastrana please review

rpastrana

@jpmcmu reviwed

rpastrana · 2024-07-19T15:47:46Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+            public AtomicLong bytesRead = new AtomicLong(0);
+            public AtomicLong bytesWritten = new AtomicLong(0);
+
+            public Span taskSpan = null;


is this the operation span?

rpastrana · 2024-07-19T15:54:28Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+        private Stack<TaskOperation> operations = new Stack<TaskOperation>();
+        public List<JSONObject> operationResults = new ArrayList<JSONObject>();
+
+        public void setTaskSpanAttributes(Attributes attributes)


setCurrentOperationSpanAttributes?

rpastrana · 2024-07-19T15:57:43Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

            }
        }

        public void addWarn(String warn)
        {
-            synchronized(warnMessages)
+            if (operations.empty())


not a big deal, but since you created the hasOperation function, why not use it here?

rpastrana · 2024-07-19T15:59:01Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+            {
+                return;
+            }
+            TaskOperation op = operations.peek();


since you created the getOperation() function, why not use it here and in all the TaskContext methods

rpastrana · 2024-07-19T16:00:24Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+        private static class TaskOperation
+        {
+            public String currentOperationDesc = "";
+            public long operationStart = 0;


minor, but the var name is somewhat ambiguous, operationStartTimeNS ? or startTimeNS?

rpastrana · 2024-07-19T16:04:10Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

-            double writeBandwidth = (double) bytesWritten.get() / (1_000_000.0 * timeInSeconds);
-            results.put("Write Bandwidth", String.format("%.2f MB/s", writeBandwidth));
-
+            TaskOperation op = getOperation();


I'm not sure if this makes sense, but could L291-294 be condensed into the following?

operationResults.add(endCurrentOperation());

rpastrana · 2024-07-19T16:18:37Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

@@ -659,6 +764,10 @@ private static void executeTasks(Runnable[] tasks, int numThreads) throws Except

                public void run()
                {
+                    // Make the task span is current for the thread, otherwise spans created


I think there's a typo in the comment

rpastrana · 2024-07-19T16:20:20Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

@@ -1095,7 +1149,8 @@ private static void performRead(String[] args, TaskContext context)
        for (int i = 0; i < datasets.length; i++)
        {
            String datasetName = datasets[i];
-            context.startOperation("Read " + datasetName);
+            context.startOperation("FileUtility.Read_" + datasetName);
+            context.setTaskSpanAttributes(Attributes.of(AttributeKey.stringKey("server.url"), connString));


did we expose addTaskSpanAttribute(Attribute)?

rpastrana · 2024-07-19T16:51:26Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/FileUtility.java

+            return true;
+        }
+
+        public TaskOperation getOperation()


and if for nothing other than symmetry, addOperation? setOperation?

I want all new operations to go through startOperation / endOperation. So I don't want to expose an addOperation etc

rpastrana · 2024-07-19T17:09:07Z

dfsclient/src/main/java/org/hpccsystems/dfs/client/Utils.java

    }

    public static Span createSpan(String name)
    {
-        return Utils.getTelemetryTracer().spanBuilder(name)
+        Span span = Utils.getTelemetryTracer().spanBuilder(name)


are we combining these 2?

jpmcmu · 2024-07-19T18:23:52Z

Tracing results from Kibana, second screenshot is the list of events under the read span.

- Updated event attribute reporting - Fixed trace structure in dfsclient protocol - Added tracing to FileUtility - Improved operation tracking in TaskContext - Added FileReadContext & FileWriteContext to DFSClient APIs Signed-off-by: James McMullan [email protected]

jpmcmu · 2024-07-19T20:17:47Z

With ESP tracing enabled:

rpastrana · 2024-07-19T20:22:28Z

Tracing results from Kibana, second screenshot is the list of events under the read span.

@jakesmith once we get dafilesrv instrumentation we'll be able to track these transactions end-to-end. We're able to deploy small, local Elastic stack based trace aggregator which would be very helpful in the instrumentation process. We can share that info if interested.

jpmcmu requested a review from rpastrana July 10, 2024 13:29

jpmcmu force-pushed the HPCC4J-611 branch from d75f0f5 to fce00d0 Compare July 16, 2024 20:06

rpastrana requested changes Jul 16, 2024

View reviewed changes

jpmcmu force-pushed the HPCC4J-611 branch from e122844 to 18b9ff6 Compare July 19, 2024 15:34

rpastrana requested changes Jul 19, 2024

View reviewed changes

rpastrana approved these changes Jul 19, 2024

View reviewed changes

jpmcmu force-pushed the HPCC4J-611 branch from 61580c9 to fde0817 Compare July 19, 2024 18:26

rpastrana merged commit a27f95b into hpcc-systems:candidate-9.6.x Jul 19, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC4J-611 Add OpenTelemetry tracing to dfsclient #722

HPCC4J-611 Add OpenTelemetry tracing to dfsclient #722

jpmcmu commented Jul 10, 2024

github-actions bot commented Jul 10, 2024

jpmcmu commented Jul 16, 2024

rpastrana left a comment

rpastrana Jul 16, 2024

rpastrana Jul 16, 2024

rpastrana Jul 16, 2024

rpastrana Jul 19, 2024

rpastrana Jul 16, 2024

rpastrana Jul 16, 2024

rpastrana Jul 16, 2024

rpastrana Jul 16, 2024

rpastrana Jul 19, 2024

jpmcmu commented Jul 19, 2024

rpastrana left a comment

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

rpastrana Jul 19, 2024

jpmcmu Jul 19, 2024

rpastrana Jul 19, 2024

jpmcmu commented Jul 19, 2024 •

edited

Loading

jpmcmu commented Jul 19, 2024

rpastrana commented Jul 19, 2024

HPCC4J-611 Add OpenTelemetry tracing to dfsclient #722

HPCC4J-611 Add OpenTelemetry tracing to dfsclient #722

Conversation

jpmcmu commented Jul 10, 2024

Type of change:

Checklist:

Testing:

github-actions bot commented Jul 10, 2024

jpmcmu commented Jul 16, 2024

rpastrana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpmcmu commented Jul 19, 2024

rpastrana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpmcmu commented Jul 19, 2024 • edited Loading

jpmcmu commented Jul 19, 2024

rpastrana commented Jul 19, 2024

jpmcmu commented Jul 19, 2024 •

edited

Loading