HPCC-33145: Optimize ESP server span creation #19373

timothyklemm · 2024-12-20T18:10:17Z

Create the server span as soon as the request information needed to decide
if a span should be created is available.
Change the server span's start time to reflect the time when the first line
of the incoming request is read.

Signed-off-by: Tim Klemm [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-12-20T18:53:20Z

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

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

ghalliday

A couple of minor comments

ghalliday · 2025-01-02T14:59:54Z

esp/bindings/http/platform/httptransport.cpp

@@ -2089,6 +2089,7 @@ int CHttpRequest::processHeaders(IMultiException *me)
    char oneline[MAX_HTTP_HEADER_LEN + 2];

    int lenread = m_bufferedsocket->readline(oneline, MAX_HTTP_HEADER_LEN + 1, me);
+    m_receivedAt.now(); // use receipt of a first loe as the start time for a server span


typo: line?

ghalliday · 2025-01-02T15:00:00Z

esp/bindings/http/platform/httpservice.cpp

+                wantTracing = !(methodName.isEmpty() ||
+                    strieq(methodName, "files") ||
+                    strieq(methodName, "xslt") ||
+                    strieq(methodName, "frame") ||


is "body" deliberately missing?

How do we ensure this stays in sync with the code later in the function? Should at least be a comment.

Good catch. Not only is "body" not intentionally missing, but this doesn't account for the underscore that could be added to each name.

I'm thinking about defining a mapping of method name to enumerated value. This tracing check will only need to check the map for a matching entry. The subsequent handling code will be refactored to switch on the enumerated value. Only one set of string comparisons will be necessary, for a small performance boost.

ghalliday

@timothyklemm I like the change. There is one issue.

The other two comments I would not have added if there was no other comments. They are there for discussion/to help the clarity of the code.

ghalliday · 2025-01-07T09:30:13Z

esp/bindings/http/platform/httpservice.cpp

+                {
+                    // The presence of a method name in the get request map is sufficient to
+                    // suppress trace output. The enumerated value will be used later.
+                    untracedRequestIt = getRequests.find(methodName);


This will not be executed if !isTracingEnabled, which means the switch statement later on will not work. This needs to be unconditional - and because it is now using a map it should be ok to be unconditional.

ghalliday · 2025-01-07T09:38:26Z

esp/bindings/http/platform/httpservice.cpp

-                    return onGetFile(m_request.get(), m_response.get(), pathEx.str());
-                }
-                else if (!stricmp(methodName.str(), "xslt"))
+                if (untracedRequestIt != getRequests.end())


picky style/encapsulation: Rather than comparing against the map null entry I would add a None entry to UntracedGetMethod, and have a local variable of that type, which was filled in. Why? Because the later test is reasonably separated from where the variable is set up, and a test against getRequests.end() isn't immediately obvious that what it is checking.

ghalliday · 2025-01-07T09:38:35Z

esp/bindings/http/platform/httpservice.cpp

@@ -297,6 +299,50 @@ void CEspHttpServer::traceRequest(IEspContext* ctx, const char* normalizeMethod)
    span->setSpanAttribute("url.full", full);
 }

+// Enumeration of "esp" service methods that are never traced. Add new values as additional
+// requests become relevant.
+enum class UntracedGetMethod


picky naming: What characteristic do these methods have that means they are not traced? Is it they are simple file requests? Something else? That adjective should be used to describe the method, rather than "Untraced" - because they are untraced for that reason, not the other way around.

ghalliday

Looks a close. A couple of minor comment.

ghalliday · 2025-01-09T16:48:00Z

esp/bindings/http/platform/httpservice.cpp

-    {"soapreq", UntracedGetMethod::SoapReq},
+using EspGetMethodMap = std::map<const char*, EspGetMethod, EspGetMethodNameComparator>;
+// Association of method names to specific "esp" service requests. This mapping allows
+// pprocessRequest to decide if the request should be traced before processing the request, without


typo: pprocessRequest

ghalliday · 2025-01-09T16:48:02Z

esp/bindings/http/platform/httpservice.cpp

@@ -402,29 +411,30 @@ int CEspHttpServer::processRequest()
        // so maximize the amount of request processing that can be traced. Specifically, user
        // authentication and authorization may generate trace output.
        bool wantTracing = true;


minor: Should be initialised to queryTraceManager().isTracingEnabled()
otherwise it will be true if it is an unhandled get method even if that function returns false.

ghalliday

Looks good afaics. Thanks, please squash.

- Create the server span as soon as the request information needed to decide if a span should be created is available. - Change the server span's start time to reflect the time when the first line of the incoming request is read. Signed-off-by: Tim Klemm <[email protected]>

timothyklemm · 2025-01-10T11:08:20Z

@ghalliday it's squashed.

timothyklemm · 2025-01-10T11:40:09Z

@ghalliday there's a build break on master unrelated to this change. I just ran into it after updating my local build.

timothyklemm requested a review from ghalliday December 23, 2024 12:19

ghalliday reviewed Jan 2, 2025

View reviewed changes

timothyklemm force-pushed the hpcc-33145-esp-server-span-creation branch from e5d40b8 to 80b6799 Compare January 2, 2025 23:44

timothyklemm requested a review from ghalliday January 3, 2025 02:51

ghalliday requested changes Jan 7, 2025

View reviewed changes

timothyklemm requested a review from ghalliday January 7, 2025 14:03

ghalliday reviewed Jan 9, 2025

View reviewed changes

timothyklemm requested a review from ghalliday January 9, 2025 18:52

ghalliday approved these changes Jan 10, 2025

View reviewed changes

timothyklemm force-pushed the hpcc-33145-esp-server-span-creation branch from 2d79881 to be76f8c Compare January 10, 2025 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-33145: Optimize ESP server span creation #19373

HPCC-33145: Optimize ESP server span creation #19373

timothyklemm commented Dec 20, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024

ghalliday left a comment

ghalliday Jan 2, 2025

timothyklemm Jan 2, 2025

ghalliday Jan 2, 2025

timothyklemm Jan 2, 2025

ghalliday left a comment

ghalliday Jan 7, 2025

ghalliday Jan 7, 2025

ghalliday Jan 7, 2025

ghalliday left a comment

ghalliday Jan 9, 2025

ghalliday Jan 9, 2025

ghalliday left a comment

timothyklemm commented Jan 10, 2025

timothyklemm commented Jan 10, 2025

HPCC-33145: Optimize ESP server span creation #19373

Are you sure you want to change the base?

HPCC-33145: Optimize ESP server span creation #19373

Conversation

timothyklemm commented Dec 20, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Dec 20, 2024

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

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

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

ghalliday left a comment

Choose a reason for hiding this comment

timothyklemm commented Jan 10, 2025

timothyklemm commented Jan 10, 2025

timothyklemm commented Dec 20, 2024 •

edited

Loading