HPCC-32248 Add tracing to rowservice #19314

jpmcmu · 2024-11-25T18:46:17Z

Added opentelemetry tracing to rowservice

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 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 · 2024-11-25T18:46:34Z

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

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

jpmcmu · 2024-11-25T18:57:26Z

fs/dafsserver/dafsserver.cpp

@@ -162,6 +162,69 @@ static ISecureSocket *createSecureSocket(ISocket *sock, bool disableClientCertVe
 }
 #endif

+//------------------------------------------------------------------------------


ActiveSpanScope is very similar to ThreadedSpanScope described by Gavin here: https://hpccsystems.atlassian.net/jira/software/c/projects/HPCC/issues/HPCC-32982. I liked the name ActiveSpanScope because I believe the class has utility outside of multithreaded contexts, IE: time slicing. Would it be worthwhile to move this out of dafilesrv into jtrace?

yes. These utility changes should be in jlib, ideally as separate PRs/requests. It would be worth merging your other PR, and having a PR that implements an agreed solution to HPCC-32982, and then rebasing this PR on it.
I'm open to discussing what the different classes should be called.

jpmcmu · 2024-11-25T19:00:48Z

fs/dafilesrv/dafilesrv.cpp

@@ -366,13 +366,31 @@ version: 1.0
    detail: 100
 )!!";

+IPropertyTree * loadConfigurationWithGlobalDefault(const char * defaultYaml, Owned<IPropertyTree>& globalConfig, const char * * argv, const char * componentTag, const char * envPrefix)


This function is similar to work Jake has done in HPCC-32991, might be worthwhile to retarget to master and call the overloaded doLoadConfiguration instead?

yes. Should reuse the from HPCC-32991. If this change are wanted in to 9.8, we could consider cherry-picking back the changed in HPCC-32991 to 9.8.

jpmcmu · 2024-11-25T19:02:45Z

fs/dafsserver/dafsserver.cpp

+        std::string traceParent = fullTraceContext ? fullTraceContext : "";
+        traceParent = traceParent.substr(0,traceParent.find_last_of("-"));
+
+        if (!traceParent.empty() && requestTraceParent != traceParent)


Note: I am checking if the traceParent has changed every time process is called here because the client side may use multiple spans during the lifetime a single CRemoteRequest. See below screenshots for an example.

jpmcmu · 2024-11-25T19:05:25Z

Goal:
The goal of this PR is to add initial tracing support to the row service in dafilesrv, which will improve debuggability for downstream row service clients as well as reducing the time the platform team spends debugging issues.

Current Tracing Limitations:
There is limited support for intercepting errors and adding them to the tracing spans, adding annotations and/or statistics to spans, and no internal spans tracking work within the row service. These limitations are intentional to keep the initial PR as simple as possible, and will be addressed in future PRs.

Exported Tracing example:
Note that during the read the client side creates more than one span over the lifetime of connection to the row service. The row service tracing supports this and correct handles the batching the client side is doing.

- Added opentelemetry tracing to rowservice Signed-off-by: James McMullan [email protected]

ghalliday

@jpmcmu looks good. A few minor comments. It would be good to rationalise the helper span scope classes so they cover all the options.

ghalliday · 2024-11-28T13:34:58Z

fs/dafsserver/dafsserver.cpp

@@ -162,6 +162,69 @@ static ISecureSocket *createSecureSocket(ISocket *sock, bool disableClientCertVe
 }
 #endif

+//------------------------------------------------------------------------------


yes. These utility changes should be in jlib, ideally as separate PRs/requests. It would be worth merging your other PR, and having a PR that implements an agreed solution to HPCC-32982, and then rebasing this PR on it.
I'm open to discussing what the different classes should be called.

ghalliday · 2024-11-28T13:37:50Z

fs/dafsserver/dafsserver.cpp

+        const char* fullTraceContext = requestTree->queryProp("_trace/traceparent");
+
+        // We only want to compare the trace-id & span-id, so remove the last "sampling" group
+        std::string traceParent = fullTraceContext ? fullTraceContext : "";


minor: You can use strrchr on the const char * to avoid cloning the string. Alternatively use std::string_view and assign to a new string.

ghalliday · 2024-11-28T13:39:05Z

fs/dafsserver/dafsserver.cpp

+        {
+            // Check to see if we have an existing span that needs to be closed out, this can happen
+            // when the span parent changes on the client side
+            if (requestSpan != nullptr)


This should be automatic when requestSpan is cleared. That would come if the changes in HPCC-32982 are implemented. Possibly requires a boolean to indicate success..

ghalliday · 2024-11-28T13:40:43Z

fs/dafsserver/dafsserver.cpp

+            Owned<IProperties> traceHeaders = createProperties();
+            traceHeaders->setProp("traceparent", fullTraceContext);
+
+            std::string requestSpanName;


minor efficiency: use a const char * and avoid a string being cloned.

ghalliday · 2024-11-28T13:44:40Z

fs/dafsserver/dafsserver.cpp

+                if (traceParent != nullptr)
+                {
+                    Owned<IProperties> traceHeaders = createProperties();
+                    traceHeaders->setProp("traceparent", traceParent);


Should this also have the sampling suffix removed?

jakesmith

@jpmcmu - please see comments.

jakesmith · 2024-12-04T17:42:33Z

fs/dafsserver/dafsserver.cpp

+//------------------------------------------------------------------------------
+// ActiveSpanScope Design Notes:
+//------------------------------------------------------------------------------
+// ActiveSpanScope updates the threadActiveSpan when it is intstantiated


intstantiated -> instantiated

jakesmith · 2024-12-04T17:49:33Z

fs/dafsserver/dafsserver.cpp

                if (0 == cursorHandle)
-                    throw createDafsException(DAFSERR_cmdstream_protocol_failure, "cursor handle not supplied to 'close' command");
+                {
+                    IException* exception = createDafsException(DAFSERR_cmdstream_protocol_failure, "cursor handle not supplied to 'close' command");


this should be changed to : IDAFS_Exception* exception = createDafsException..

otherwise the throw exception (of an IException) will not be caught by catch (IDAFS_Exception *e) handlers.

jakesmith · 2024-12-04T17:58:13Z

fs/dafilesrv/dafilesrv.cpp

+    Owned<IPropertyTree> env = getHPCCEnvironment();
+    IPropertyTree* globalTracing = env->queryPropTree("Software/tracing");
+    if (globalTracing != nullptr)
+        extractedGlobalConfig->addPropTree("tracing", globalTracing);


I think this should be LINK(globalTracing) (or env->getPropTree)

jakesmith · 2024-12-04T18:00:57Z

fs/dafilesrv/dafilesrv.cpp

+#endif
+
+    // NB: bare-metal dafilesrv does not have a component specific xml, extracting relevant global configuration instead
+    Owned<IPropertyTree> config = loadConfigurationWithGlobalDefault(defaultYaml, extractedGlobalConfig, argv, "dafilesrv", "DAFILESRV");


instead of new function/adding to global, could you add 'tracing' to the component config instead?
e.g.:

#ifndef _CONTAINERIZED Owned<IPropertyTree> env = getHPCCEnvironment(); IPropertyTree* tracing = env->getPropTree("Software/tracing"); if (tracing) config->setPropTree("tracing", tracing); #endif

(and combine with #else // __CONTAINERIZED block below)

jpmcmu requested review from ghalliday and jakesmith November 25, 2024 18:46

jpmcmu commented Nov 25, 2024

View reviewed changes

HPCC-32248 Add tracing to rowservice

deb8270

- Added opentelemetry tracing to rowservice Signed-off-by: James McMullan [email protected]

jpmcmu force-pushed the HPCC-32248 branch from 5426202 to deb8270 Compare November 25, 2024 20:17

ghalliday reviewed Nov 28, 2024

View reviewed changes

jakesmith reviewed Dec 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32248 Add tracing to rowservice #19314

HPCC-32248 Add tracing to rowservice #19314

jpmcmu commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

jpmcmu Nov 25, 2024 •

edited

Loading

ghalliday Nov 28, 2024

jpmcmu Nov 25, 2024

jakesmith Dec 4, 2024

jpmcmu Nov 25, 2024

jpmcmu commented Nov 25, 2024 •

edited

Loading

ghalliday left a comment

ghalliday Nov 28, 2024

ghalliday Nov 28, 2024

ghalliday Nov 28, 2024

ghalliday Nov 28, 2024

ghalliday Nov 28, 2024

jakesmith left a comment

jakesmith Dec 4, 2024

jakesmith Dec 4, 2024

jakesmith Dec 4, 2024

jakesmith Dec 4, 2024

HPCC-32248 Add tracing to rowservice #19314

Are you sure you want to change the base?

HPCC-32248 Add tracing to rowservice #19314

Conversation

jpmcmu commented Nov 25, 2024

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 25, 2024

jpmcmu Nov 25, 2024 • edited Loading

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 Nov 25, 2024 • edited Loading

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

jakesmith 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

jpmcmu Nov 25, 2024 •

edited

Loading

jpmcmu commented Nov 25, 2024 •

edited

Loading