HPCC-30401 JTrace should optionally suppress trace/span ids generation #17935

rpastrana · 2023-10-23T02:54:03Z

Feature request somewhat ambiguous
Copies global.tracing to container helm values
Generates traceid if server doesn't receive remote parent

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 · 2023-10-23T02:54:32Z

https://track.hpccsystems.com/browse/HPCC-30401
Jira updated

rpastrana · 2023-10-23T02:55:17Z

@ghalliday it wasn't clear exactly when we wanted to force traceids. This PR is not complete but wanted to get in synch with you before spending any more time down this path...

rpastrana · 2023-10-23T14:35:49Z

@ghalliday sample output from first commit doesn't make much sense. Hoping 2nd commit is closer to your intention.

"!!No remoteParent received, generating remoteParent IDs: Trace '30d658bd5fbc5c6583c3dedca16b8ba0' span '208c54bca6cd4d0a' "
00000078 PRG INF 2023-10-23 13:52:02.392     7   286 UNK     "Span start: {"Type":"Server","Name":"request","GlobalID":"JD8LowRvXm3ujvmHS82NAT","LocalID":"JD8LowSwvRDqbiZ7dRDJdt","SpanID":"621030d51af8737e","TraceID":"30d658bd5fbc5c6583c3dedca16b8ba0","ParentSpanID":"208c54bca6cd4d0a"}

ghalliday

@rpastrana looks good. A couple of questions.

ghalliday · 2023-10-25T11:02:40Z

helm/hpcc/templates/thor.yaml

@@ -32,7 +32,7 @@ Pass in dict with root and me
 {{- $thorAgentScope := dict "name" .thorAgentName "replicas" .thorAgentReplicas "maxActive" .me.maxGraphs | merge (pick .me "keepJobs") }}
 {{- $eclAgentResources := .me.eclAgentResources | default dict -}}
 {{- $hthorScope := dict "name" $hthorName "jobMemorySectionName" "eclAgentMemory" | merge (pick .me "multiJobLinger" "maxGraphStartupTime") | merge (dict "resources" (deepCopy $eclAgentResources)) }}
-{{- $thorScope := omit .me "eclagent" "thoragent" "hthor" "logging" "env" "eclAgentResources" "eclAgentUseChildProcesses" "eclAgentReplicas" "thorAgentReplicas" "eclAgentType" }}
+{{- $thorScope := omit .me "eclagent" "thoragent" "hthor" "logging" "Tracing" "env" "eclAgentResources" "eclAgentUseChildProcesses" "eclAgentReplicas" "thorAgentReplicas" "eclAgentType" }}


should this be a lower case "tracing"?

definitely, good catch

ghalliday · 2023-10-25T11:05:43Z

system/jlib/jtrace.cpp

    {
        bool createLocalId = !isEmptyString(hpccGlobalId);
        if (createLocalId)
            hpccLocalId.set(ln_uid::createUniqueIdString().c_str());

-        auto provider = opentelemetry::trace::Provider::GetTracerProvider();
+        //we don't always want to create trace/span IDs


Does this preserve trace-ids if this flags are false?

Does this preserve trace-ids if this flags are false?

Good point, opentel remote/local parentSpans won't have a span to associate with...

We use the otel span object to track many attributes including parent trace/span ID, do we care to keep track of them ourselves if !hasMask(flags, SpanFlags::EnsureTraceId) && !queryTraceManager().alwaysCreateTraceIds()

Update, we actually do track
opentelemetry::v1::trace::SpanContext remoteParentSpanCtx and
CSpan * localParentSpan
We just have to ensure tolog/tostring etc draws from these members and not just the span member

removing this comment to avoid confusion

Latest amended commit simply inherits traceID from parent when no parent otel span available:

sample output for no traceID generated mode serverSpan(propagated parent)->clientSpanserverchild->internalSpan->internalSpan2

15:07:40.802583 27843 Span end: {"Type":"Internal","Name":"internalSpan2","TraceID":"beca49ca8f3138a2842e5cf21402bfff","ParentSpanID": ""} 15:07:40.802618 27843 Span end: {"Type":"Internal","Name":"internalSpan","TraceID":"beca49ca8f3138a2842e5cf21402bfff","ParentSpanID": ""} 15:07:40.802628 27843 Span end: {"Type":"Client","Name":"clientSpanserverchild","TraceID":"beca49ca8f3138a2842e5cf21402bfff","ParentSpanID": ""} 15:07:40.802637 27843 Span end: {"Type":"Server","Name":"propegatedServerSpan","GlobalID":"IncomingUGID","CallerID":"IncomingCID","LocalID":"JDbF4xnv7LSWDV4Eug1SpJ","TraceID":"beca49ca8f3138a2842e5cf21402bfff","ParentSpanID":"4b960b3e4647da3f"}

ghalliday · 2023-10-25T11:06:14Z

system/jlib/jtrace.cpp

@@ -637,6 +638,33 @@ class CServerSpan : public CSpan
                opts.parent = remoteParentSpanCtx;
            }
        }
+
+       /*if ((!httpHeaders ||  !httpHeaders->hasProp("traceparent")) && queryTraceManager().alwaysCreateTraceIds())


When this is a full PR this should be moved elsewhere.

When this is a full PR this should be moved elsewhere.

I initially assumed this is what we wanted to do in this feature, but quickly realized forcing a remote trace/span ID didn't really make sense but left the commented out block to inspire this conversation...

Do we ever envision forcing a synthetic remote trace/span ID?

I don't think you ever want to create a synthetic remote id.

ghalliday · 2023-10-25T11:08:03Z

I'm not sure why "Feature request somewhat ambiguous" is in the commit. If it is ambiguous it is worth clarifying before creating the PR.

rpastrana · 2023-10-25T12:58:21Z

I'm not sure why "Feature request somewhat ambiguous" is in the commit. If it is ambiguous it is worth clarifying before creating the PR.

Because I found the description somewhat ambiguous and the creator was away and this feature was identified as priority.
Anyway, the point of that statement was for the reviewer to understand why the draft PR might have missed the gist of the feature request before significant time was spent on the implementation.

ghalliday

@rpastrana I think this is close, but it isn't quite right yet.

ghalliday · 2023-11-02T12:41:57Z

system/jlib/jtrace.cpp

@@ -16,7 +16,7 @@
 ############################################################################## */


-
+//#include "opentelemetry/sdk/trace/random_id_generator_factory.h"


trivial: remove

ghalliday · 2023-11-02T12:43:33Z

system/jlib/jtrace.cpp

@@ -185,18 +190,19 @@ class CSpan : public CInterfaceOf<ISpan>
        if (!isEmptyString(hpccLocalId.get()))
            out.append(",\"LocalID\":\"").append(hpccLocalId.get()).append("\"");

+        //traceID not always tied to span object
+        out.append(",\"TraceID\":\"").append(traceID.get()).append("\"");


Should this check for isEmptyString()

@rpastrana I think this question is still relevant

ghalliday · 2023-11-02T12:45:56Z

system/jlib/jtrace.cpp

@@ -637,6 +638,33 @@ class CServerSpan : public CSpan
                opts.parent = remoteParentSpanCtx;
            }
        }
+
+       /*if ((!httpHeaders ||  !httpHeaders->hasProp("traceparent")) && queryTraceManager().alwaysCreateTraceIds())


I don't think you ever want to create a synthetic remote id.

ghalliday · 2023-11-02T12:50:06Z

system/jlib/jtrace.cpp

-        //what if tracerName is empty?
-        auto tracer = provider->GetTracer(tracerName.get());
+        //we don't always want to create trace/span IDs
+        if (hasMask(flags, SpanFlags::EnsureTraceId) || queryTraceManager().alwaysCreateTraceIds())


This doesn't seem quite right. You still want to create a span if a trace-id was provided by the caller. It is only omitted if it is not provided and ids are not created by default.

ghalliday · 2023-11-02T12:51:55Z

system/jlib/jtrace.cpp

@@ -171,6 +171,11 @@ class CSpan : public CInterfaceOf<ISpan>
        return spanID.get();
    }

+    const char * getTraceID() const
+    {
+        return traceID.get();


This shouldn't really be stored for non-server spans, similar to caller and local spans. The span class structure would benefit from some refactoring (but as a separate PR)

ghalliday

@rpastrana one question comment, but otherwise it looks ready to merge.

ghalliday · 2023-11-06T17:05:06Z

system/jlib/jtrace.cpp

@@ -185,18 +190,19 @@ class CSpan : public CInterfaceOf<ISpan>
        if (!isEmptyString(hpccLocalId.get()))
            out.append(",\"LocalID\":\"").append(hpccLocalId.get()).append("\"");

+        //traceID not always tied to span object
+        out.append(",\"TraceID\":\"").append(traceID.get()).append("\"");


@rpastrana I think this question is still relevant

ghalliday

@rpastrana please squash and rebase. Thanks.

ghalliday · 2023-11-10T10:16:40Z

@rpastrana unfortunately needs rebasing again because I merged your other test.
Also, can you clarify what "disabled" means. Does it mean don't propagate anything? How does it differ from the createXXX options?

rpastrana · 2023-11-10T16:46:35Z

@rpastrana unfortunately needs rebasing again because I merged your other test. Also, can you clarify what "disabled" means. Does it mean don't propagate anything? How does it differ from the createXXX options?

Rebased, but yes, the introduction of the create

@rpastrana unfortunately needs rebasing again because I merged your other test. Also, can you clarify what "disabled" means. Does it mean don't propagate anything? How does it differ from the createXXX options?

Rebased.
Yes, the newly created alwaysCreateTraceIds and its ultimate implementation does cloud the definition of the global.tracing.disabled flag...

Initially, disabled would suppress configuration of Otel's tracer(span processor/exporter, etc), and thereby avoid all otel trace/span creation/processing/exporting.
The other flags control non-otel functionality.
As-is, the disabled and alwaysCreateTraceIds flags have an overlap.... if disabled, no valid trace IDs can be generated even if we've been instructed to "Always create TraceIds".

rpastrana · 2023-11-15T15:41:35Z

@ghalliday this was showing a conflict that shouldn't have been there. After re-rebasing, I manually resolved the conflict.

ghalliday · 2023-11-16T12:39:04Z

@rpastrana the commits are not quite right. I suspect you may need to rebase your branch onto the most recent 9.4.x

- Copies global.tracing to component helm values - Revises logic to create otel span -- Span created if per-span flag set -- or global/component config flag set -- or serverSpan receives valid remote parent - Adds cppunit (cannot test scenario where no span created yet) Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana requested a review from ghalliday October 23, 2023 02:54

ghalliday reviewed Oct 25, 2023

View reviewed changes

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch from 2528c90 to 623b1a7 Compare October 26, 2023 19:15

rpastrana marked this pull request as ready for review October 26, 2023 19:19

rpastrana changed the title ~~HPCC-30401 WIP~~ HPCC-30401 JTrace should optionally suppress trace/span ids generation Oct 26, 2023

rpastrana requested a review from ghalliday October 31, 2023 20:51

ghalliday reviewed Nov 2, 2023

View reviewed changes

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch from 623b1a7 to 2d5d45b Compare November 3, 2023 21:20

rpastrana requested a review from ghalliday November 3, 2023 21:21

ghalliday reviewed Nov 6, 2023

View reviewed changes

rpastrana requested a review from ghalliday November 7, 2023 04:12

ghalliday approved these changes Nov 9, 2023

View reviewed changes

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch 2 times, most recently from e1c7fba to 0668208 Compare November 9, 2023 14:23

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch from 0668208 to 2806c9f Compare November 10, 2023 14:05

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch from 2806c9f to 07783ea Compare November 15, 2023 15:36

rpastrana force-pushed the HPCC-30401-JTrace-optional-traceid-support2 branch from 21bd30b to b8d0e49 Compare November 16, 2023 15:08

ghalliday merged commit 11c0506 into hpcc-systems:candidate-9.4.x Nov 16, 2023
42 of 45 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30401 JTrace should optionally suppress trace/span ids generation #17935

HPCC-30401 JTrace should optionally suppress trace/span ids generation #17935

rpastrana commented Oct 23, 2023 •

edited

Loading

github-actions bot commented Oct 23, 2023

rpastrana commented Oct 23, 2023

rpastrana commented Oct 23, 2023

ghalliday left a comment

ghalliday Oct 25, 2023

rpastrana Oct 25, 2023

ghalliday Oct 25, 2023

rpastrana Oct 25, 2023 •

edited

Loading

rpastrana Oct 26, 2023 •

edited

Loading

rpastrana Oct 26, 2023

ghalliday Oct 25, 2023

rpastrana Oct 25, 2023

ghalliday Nov 2, 2023

ghalliday commented Oct 25, 2023

rpastrana commented Oct 25, 2023

ghalliday left a comment

ghalliday Nov 2, 2023

ghalliday Nov 2, 2023

ghalliday Nov 6, 2023

ghalliday Nov 2, 2023

ghalliday Nov 2, 2023

ghalliday Nov 2, 2023

ghalliday left a comment

ghalliday Nov 6, 2023

ghalliday left a comment

ghalliday commented Nov 10, 2023

rpastrana commented Nov 10, 2023

rpastrana commented Nov 15, 2023

ghalliday commented Nov 16, 2023

		@@ -16,7 +16,7 @@
		############################################################################## */



		//#include "opentelemetry/sdk/trace/random_id_generator_factory.h"

HPCC-30401 JTrace should optionally suppress trace/span ids generation #17935

HPCC-30401 JTrace should optionally suppress trace/span ids generation #17935

Conversation

rpastrana commented Oct 23, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Oct 23, 2023

rpastrana commented Oct 23, 2023

rpastrana commented Oct 23, 2023

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

rpastrana Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

rpastrana Oct 26, 2023 • 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

ghalliday commented Oct 25, 2023

rpastrana commented Oct 25, 2023

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

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

ghalliday left a comment

Choose a reason for hiding this comment

ghalliday commented Nov 10, 2023

rpastrana commented Nov 10, 2023

rpastrana commented Nov 15, 2023

ghalliday commented Nov 16, 2023

rpastrana commented Oct 23, 2023 •

edited

Loading

rpastrana Oct 25, 2023 •

edited

Loading

rpastrana Oct 26, 2023 •

edited

Loading