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.

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.

fs/dafsserver/dafsserver.cpp

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.

fs/dafsserver/dafsserver.cpp

fs/dafilesrv/dafilesrv.cpp

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)

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

ghalliday · 2025-01-09T13:48:26Z

fs/dafsserver/dafsserver.cpp


-        if (!traceParent.empty() && requestTraceParent != traceParent)
+        if (strlen(traceParent) != 0 && requestTraceParent != traceParent)


trivial: isEmptyString is more efficient (since it only checks the 1st character)

ghalliday · 2025-01-09T13:49:36Z

fs/dafsserver/dafsserver.cpp


-        if (!traceParent.empty() && requestTraceParent != traceParent)
+        if (strlen(traceParent) != 0 && requestTraceParent != traceParent)


I know I made a comment "minor: You can use strrchr on the const char * to avoid cloning the string.".

I think that comment should have more explicit - you should still be creating a temporary string from the result. The modified code is not equivalent to the previous code - it is probably better as it was.

Previously the code set traceParent to the text before the last '-'
now it sets traceParent to the last '-' and the text that follows it.
Also the comparison will implicitly convert it to a std:string anyway - it would be better if that was explicit.

Oh yeah, I am not sure what I was thinking here..

ghalliday · 2025-01-09T13:50:59Z

fs/dafsserver/dafsserver.cpp

                const char* traceParent = requestTree->queryProp("_trace/traceparent");
                if (traceParent != nullptr)
                {
                    Owned<IProperties> traceHeaders = createProperties();
                    traceHeaders->setProp("traceparent", traceParent);

-                    versionSpan = queryTraceManager().createServerSpan("VersionRequest", traceHeaders);
+                    versionSpan.set(queryTraceManager().createServerSpan("VersionRequest", traceHeaders));


Should be setown, otherwise I think the object will leak.

jakesmith

@jpmcmu - couple of comments.

jakesmith · 2025-01-09T16:48:03Z

fs/dafilesrv/dafilesrv.cpp

 int main(int argc, const char* argv[])
 {
    InitModuleObjects();

    EnableSEHtoExceptionMapping();
 #ifndef __64BIT__
+


trivial/formatting. extra newline added incidentally (and one removed on line 369, but that looks more deliberate)

jakesmith · 2025-01-09T16:52:12Z

fs/dafilesrv/dafilesrv.cpp

+
+    const char* componentTag = "dafilesrv";
+    Owned<IPropertyTree> componentDefault;
+    if (defaultYaml)


why conditional? defaultYaml is always defined

jakesmith · 2025-01-09T17:04:27Z

fs/dafilesrv/dafilesrv.cpp

+    {
+        Owned<IPropertyTree> defaultConfig = createPTreeFromYAMLString(defaultYaml, 0, ptr_ignoreWhiteSpace, nullptr);
+        componentDefault.set(defaultConfig->queryPropTree(componentTag));
+        if (!componentDefault)


this should never be true, the const defaultYaml explicitly defines "dafilesrv" so I don't think there's any point in checking (code currently leads me to think it may be missing)

jakesmith · 2025-01-09T17:19:07Z

fs/dafilesrv/dafilesrv.cpp

+            throw makeStringExceptionV(99, "Default configuration does not contain the tag %s", componentTag);
+    }
+    else
+        componentDefault.setown(createPTree(componentTag));


I think the above would be better simplified to:

const char* componentTag = "dafilesrv"; Owned<IPropertyTree> defaultConfig = createPTreeFromYAMLString(defaultYaml, 0, ptr_ignoreWhiteSpace, nullptr); Owned<IPropertyTree> componentDefault = defaultConfig->getPropTree(componentTag);

or I might be tempted to encapsulate this pattern into a another loadConfiguration variety (that calls onto loadConfiguration(IPropertyTree * defaultConfig, IPropertyTree * globalConfig, ...).

ghalliday

@jpmcmu a couple of comments/issues.

jpmcmu · 2025-01-10T13:52:50Z

@jakesmith @ghalliday Thanks for the reviews, I have implemented those changes

jakesmith

@jpmcmu - 1 follow up issue.

jakesmith · 2025-01-10T15:25:25Z

fs/dafilesrv/dafilesrv.cpp

-    else
-        componentDefault.setown(createPTree(componentTag));
+    Owned<IPropertyTree> defaultConfig = createPTreeFromYAMLString(defaultYaml, 0, ptr_ignoreWhiteSpace, nullptr);
+    Owned<IPropertyTree> componentDefault(defaultConfig->queryPropTree(componentTag));


-ve leak. Needs to be a getPropTree

Ok, right, I need to pay more attention to query/get and set/setown, it is slightly unituitive to me for some reason. So, is the following correct: get* methods will increase the reference count, query* methods will not. Owned constructor does NOT increase reference count, set increases reference count while setown does not.

AttilaVamos · 2025-01-10T19:39:44Z

@jpmcmu It seems this PR went timeout on DFUAccessTest (unit test). Can you try to run that in your local environment?

/opt/HPCCSystems/bin/unittests DFUAccessTest

Usually it takes seconds to finish.

AttilaVamos · 2025-01-10T20:07:22Z

Some stack trace:

Thread 1 (Thread 0x7f5934987300 (LWP 83703)):
#0  0x00007f59367dc301 in poll () from /lib64/libc.so.6
#1  0x00007f59396c5e70 in CSocket::wait_read(unsigned int) () from /opt/HPCCSystems/lib/libjlib.so
#2  0x00007f59396c96c2 in CSocket::readtms(void*, unsigned int, unsigned int, unsigned int&, unsigned int, bool) () from /opt/HPCCSystems/lib/libjlib.so
#3  0x00007f593c51bf17 in receiveDaFsBufferSize(ISocket*, unsigned int, CTimeMon*) () from /opt/HPCCSystems/lib/libdafsclient.so
#4  0x00007f593c51c0f6 in receiveDaFsBuffer(ISocket*, MemoryBuffer&, unsigned int, unsigned int) () from /opt/HPCCSystems/lib/libdafsclient.so
#5  0x00007f593c51cefe in CRemoteBase::sendRemoteCommand(MemoryBuffer&, MemoryBuffer&, bool, bool, bool) () from /opt/HPCCSystems/lib/libdafsclient.so
#6  0x00007f5933326e36 in dafsstream::CDFUPartFlatWriter::finalize() () from /opt/HPCCSystems/lib/libdafsstream.so
#7  0x00007f593332197b in dafsstream::CDaFileSrvClientBase::beforeDispose() () from /opt/HPCCSystems/lib/libdafsstream.so
#8  0x00007f5933322ac8 in non-virtual thunk to dafsstream::CDFUPartWriterBase::Release() const () from /opt/HPCCSystems/lib/libdafsstream.so
#9  0x00007f5933132e43 in ?? () from /opt/HPCCSystems/lib/libwsdfuaccess.so
#10 0x00007f593f157fa2 in CppUnit::TestCaseMethodFunctor::operator()() const () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#11 0x00007f593f14df0e in CppUnit::DefaultProtector::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) ()
   from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
--Type <RET> for more, q to quit, c to continue without paging--
#12 0x00007f593f155110 in CppUnit::ProtectorChain::protect(CppUnit::Functor const&, CppUnit::ProtectorContext const&) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#13 0x00007f593f15f794 in CppUnit::TestResult::protect(CppUnit::Functor const&, CppUnit::Test*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#14 0x00007f593f157c59 in CppUnit::TestCase::run(CppUnit::TestResult*) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#15 0x00007f593f158233 in CppUnit::TestComposite::doRunChildTests(CppUnit::TestResult*) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#16 0x00007f593f1582b7 in CppUnit::TestComposite::run(CppUnit::TestResult*) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#17 0x00007f593f15f6f2 in CppUnit::TestResult::runTest(CppUnit::Test*) () from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#18 0x00007f593f161f9d in CppUnit::TestRunner::run(CppUnit::TestResult&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
   from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#19 0x00007f593f163ff2 in CppUnit::TextTestRunner::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool, bool, bool) ()
   from /opt/HPCCSystems/lib/libcppunit-1.15.so.1
#20 0x000000000044d62d in ?? ()
#21 0x00007f59366f2d85 in __libc_start_main () from /lib64/libc.so.6
#22 0x000000000045011e in ?? ()

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

jpmcmu commented Nov 25, 2024

View reviewed changes

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

jpmcmu added 2 commits January 8, 2025 09:18

HPCC-32248 Add tracing to rowservice

6aa073f

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

Code review changes

3b0458d

jpmcmu force-pushed the HPCC-32248 branch from deb8270 to 3b0458d Compare January 8, 2025 14:19

jpmcmu changed the base branch from candidate-9.8.x to master January 8, 2025 14:20

jpmcmu requested review from jakesmith and ghalliday January 8, 2025 14:23

ghalliday requested changes Jan 9, 2025

View reviewed changes

jakesmith reviewed Jan 9, 2025

View reviewed changes

ghalliday requested changes Jan 10, 2025

View reviewed changes

Code review changes

a007e25

jpmcmu requested review from ghalliday and jakesmith January 10, 2025 13:52

jakesmith reviewed Jan 10, 2025

View reviewed changes

Code review changes

961a329

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

jakesmith left a comment

jakesmith Dec 4, 2024

ghalliday Jan 9, 2025

ghalliday Jan 9, 2025

jpmcmu Jan 10, 2025

ghalliday Jan 9, 2025

jakesmith left a comment

jakesmith Jan 9, 2025

jakesmith Jan 9, 2025

jakesmith Jan 9, 2025

jakesmith Jan 9, 2025

ghalliday left a comment

jpmcmu commented Jan 10, 2025

jakesmith left a comment

jakesmith Jan 10, 2025

jpmcmu Jan 10, 2025 •

edited

Loading

AttilaVamos commented Jan 10, 2025

AttilaVamos commented Jan 10, 2025


		if (!traceParent.empty() && requestTraceParent != traceParent)
		if (strlen(traceParent) != 0 && requestTraceParent != traceParent)

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

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

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

ghalliday left a comment

Choose a reason for hiding this comment

jpmcmu commented Jan 10, 2025

jakesmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpmcmu Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

AttilaVamos commented Jan 10, 2025

AttilaVamos commented Jan 10, 2025

jpmcmu Nov 25, 2024 •

edited

Loading

jpmcmu commented Nov 25, 2024 •

edited

Loading

jpmcmu Jan 10, 2025 •

edited

Loading