HPCC-32372 Support tracing from testsocket #19613

rpastrana · 2025-03-12T17:56:34Z

Adds testsocket option to generate and propagate OTel based trace context
Adds option to customize JTrace

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 improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2025-03-12T17:56:50Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32372

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

Copilot

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

rpastrana · 2025-03-12T18:51:57Z

Disclaimer, if custom JTrace configuration is provided, we should expect JLOG output akin to this:
00000000 OPR 2025-03-12 13:36:37.381 710123 710123 "Exporter type: OTLP-GRPC"
00000001 OPR 2025-03-12 13:36:37.381 710123 710123 "Tracing exporter set to OTLP/GRPC to: (192.168.68.111:4317/)"

and also, potential OTel export debug info such as:
00000003 OPR 2025-03-12 13:36:58.858 710123 710247 "ERROR: JTrace: [OTLP TRACE GRPC Exporter] Export() failed with status_code: "UNAVAILABLE" error_message: "failed to connect to all addresses; last error: UNKNOWN: ipv4:192.168.68.111:4317: Failed to connect to remote host: Connection refused""

Also, if debug build, Jtrace log exporter will be automatically enabled and will generate output such as:
00000002 MON 2025-03-12 13:36:56.571 710123 710123 "{ "type": "span", "name": "testsocket_sendQuery", "trace_id": "9bc7727c61d90e3bf06b5fe222199e3d", "span_id": "c996a62a62b5cdd5", "start": 1741800997417554077, "duration": 19161849004, "parent_span_id": "093bde7626153368" }"

Also, the preexisting "trace" file will contain the resulting trace id

rpastrana · 2025-03-12T18:52:11Z

@mckellyln please review

rpastrana · 2025-03-12T19:06:42Z

Sample program args:
"program": "${workspaceFolder}/build/Debug/bin/testsocket",
"args": [
"localhost:9876", "-gcs", "-ctc" "{global: {tracing: {disable: false, resourceAttributes: {deploymentEnvironment: development}, exporters: [{type: OTLP-GRPC, endpoint: '192.168.68.111:4317/', useSslCredentials: false}]}}}", "-f", "/home/ubuntu/GIT/roxietracetest.ecl"
],
where roxietracetest.ecl: <_trace>00-5a8fea933a38ef0aab31d2c1008675d6-f186c3292b4308e9-01Jf4xTnd5VvXeH8PQGM8NnGJf4xTngoqZU9j99iBnsXYR</_trace>33434

mckellyln · 2025-03-18T17:00:26Z

tools/testsocket/testsocket.cpp

+        }
+        else if (strieq(argv[arg], "-ctc")) 
+        {
+            tracingConfig.set(argv[++arg]);


good to check if ++arg doesnt >= argc, but we dont check that everywhere above like we should.

mckellyln · 2025-03-20T01:49:34Z

tools/testsocket/testsocket.cpp

@@ -619,8 +623,19 @@ int doSendQuery(const char * ip, unsigned port, const char * base)
            newQuery.appendf("</%sRequest></Body></Envelope>", queryName);
            base = newQuery.str();
        }
+
+        StringBuffer tpHeader;
+        Owned<IProperties> retrievedClientHeaders = createProperties();


Should this move to inside the if below, since retrievedClientHeaders is only used in the if block ?
Doesnt really make a difference tho.

mckellyln

Approved.
Just one small comment.
Good to squash.

rpastrana · 2025-03-21T15:13:53Z

Suggested change incorporated, and squashed.
@ghalliday ready for merge.

ghalliday

@rpastrana Looks useful. A couple of trivial comments, one suggestion for a simpler implementation and a couple of leaks.

ghalliday · 2025-03-31T14:09:27Z

tools/testsocket/testsocket.cpp

+
+            if (retrievedClientHeaders->hasProp("traceparent"))
+            {
+                //disassemble xml in oder to inject _trace header


typo: order

ghalliday · 2025-03-31T14:11:01Z

tools/testsocket/testsocket.cpp

+                    ForEach(*traceElemets)
+                    {
+                        IPTree &traceHeaderElem = traceElemets->query();
+                        if (strcmp(traceHeaderElem.queryName(), "traceparent") != 0) //if not _trace


comment references the wrong label.

ghalliday · 2025-03-31T14:13:15Z

tools/testsocket/testsocket.cpp

+
+                fullQuery.setf("<%s><_trace><traceparent>%s</traceparent>", queryName, retrievedClientHeaders->queryProp("traceparent"));
+
+                IPropertyTree * traceHeader = p ->queryPropTree("_trace");


Would it work (and be simpler) to code as:

if (traceHeader) traceHeader->setProp("traceparent", retrievedClientHeaders->queryProp("traceparent");

and then convert the whole property tree to xml again?

ghalliday · 2025-03-31T14:15:32Z

tools/testsocket/testsocket.cpp

+    }
+    else
+    {
+        serverSpan.set(getNullSpan());


This will leak - it should be setown

ghalliday · 2025-03-31T14:15:44Z

tools/testsocket/testsocket.cpp

+
+            initTraceManager("testsocket", traceConfig, nullptr);
+
+            serverSpan.set(queryTraceManager().createServerSpan("testsocket_server", nullptr)); //can we avoid creating a server span?


This will leak, it should be setown

ghalliday

@rpastrana looks good, please squash

ghalliday · 2025-04-01T08:04:40Z

tools/testsocket/testsocket.cpp

-                IPropertyTree * traceHeader = p ->queryPropTree("_trace");
-
-                if (traceHeader)
+                IPropertyTree * traceHeaderTree = fullQueryTree->queryPropTree("_trace");


For future, simpler would be

IPropertyTree * traceHeaderTree = ensurePTree(fullQueryTree, "_trace"); traceHeaderTree->setProp("traceparent", retrievedClientHeaders->queryProp("traceparent"));

- Adds testsocket option to generate and propagate OTel based trace context - Adds option to customize JTrace - Tests for valid yaml - Updates usage message Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana · 2025-04-01T14:35:35Z

commits are squashed.

github-actions · 2025-04-03T10:16:25Z

Jirabot Action Result:
Added fix version: 9.10.18
Workflow Transition: 'Resolve issue'

rpastrana requested a review from Copilot March 12, 2025 18:04

Copilot AI reviewed Mar 12, 2025

View reviewed changes

rpastrana force-pushed the HPCC-32372-testsocket-tracing branch from 3bd2f8c to 3a0e4b1 Compare March 12, 2025 18:47

rpastrana requested a review from mckellyln March 12, 2025 18:49

mckellyln reviewed Mar 18, 2025

View reviewed changes

rpastrana requested a review from mckellyln March 18, 2025 18:07

mckellyln reviewed Mar 20, 2025

View reviewed changes

mckellyln approved these changes Mar 20, 2025

View reviewed changes

rpastrana force-pushed the HPCC-32372-testsocket-tracing branch from 2758cb0 to 4863882 Compare March 21, 2025 15:13

ghalliday requested changes Mar 31, 2025

View reviewed changes

rpastrana requested a review from ghalliday March 31, 2025 18:53

ghalliday approved these changes Apr 1, 2025

View reviewed changes

HPCC-32372 Support tracing from testsocket

65b52b6

- Adds testsocket option to generate and propagate OTel based trace context - Adds option to customize JTrace - Tests for valid yaml - Updates usage message Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana force-pushed the HPCC-32372-testsocket-tracing branch from 683f5a6 to 65b52b6 Compare April 1, 2025 14:25

ghalliday merged commit 86de2d5 into hpcc-systems:candidate-9.10.x Apr 3, 2025
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32372 Support tracing from testsocket #19613

HPCC-32372 Support tracing from testsocket #19613

rpastrana commented Mar 12, 2025 •

edited

Loading

github-actions bot commented Mar 12, 2025

Copilot AI left a comment

rpastrana commented Mar 12, 2025

rpastrana commented Mar 12, 2025

rpastrana commented Mar 12, 2025

mckellyln Mar 18, 2025

mckellyln Mar 20, 2025

mckellyln left a comment

rpastrana commented Mar 21, 2025

ghalliday left a comment

ghalliday Mar 31, 2025

ghalliday Mar 31, 2025

ghalliday Mar 31, 2025

ghalliday Mar 31, 2025

ghalliday Mar 31, 2025

ghalliday left a comment

ghalliday Apr 1, 2025

rpastrana commented Apr 1, 2025

github-actions bot commented Apr 3, 2025


		fullQuery.setf("<%s><_trace><traceparent>%s</traceparent>", queryName, retrievedClientHeaders->queryProp("traceparent"));

		IPropertyTree * traceHeader = p ->queryPropTree("_trace");


		initTraceManager("testsocket", traceConfig, nullptr);

		serverSpan.set(queryTraceManager().createServerSpan("testsocket_server", nullptr)); //can we avoid creating a server span?

HPCC-32372 Support tracing from testsocket #19613

HPCC-32372 Support tracing from testsocket #19613

Conversation

rpastrana commented Mar 12, 2025 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Mar 12, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

rpastrana commented Mar 12, 2025

rpastrana commented Mar 12, 2025

rpastrana commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckellyln left a comment

Choose a reason for hiding this comment

rpastrana commented Mar 21, 2025

ghalliday 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

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Apr 1, 2025

github-actions bot commented Apr 3, 2025

rpastrana commented Mar 12, 2025 •

edited

Loading