HPCC-33298 JTrace support sampling configuration #19456

rpastrana · 2025-01-24T20:16:27Z

Adds Jtrace sampler configuration
Adds OTel sampler initialization logic
Updates JTrace configuration README
Provides samples
Jlog trace/span ids suppressed if not sampled (not sure if this is wanted)

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-01-24T20:16:45Z

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

Jirabot Action Result:
Assigning user: [email protected]
Workflow Transition To: Merge Pending
Updated PR

rpastrana · 2025-01-28T22:12:46Z

@jakesmith made some wholesale changes in order to support the parentBased sampler annotation just in case you were in the middle of reviewing

jakesmith

@rpastrana - looks good, some very minor comments.

jakesmith · 2025-01-29T12:53:13Z

system/jlib/jtrace.cpp

+        if (!isEmptyString(samplerType))
+        {
+            const char * samplerArgument = samplerTree->queryProp("@argument");
+            if (strcmp("AlwaysOff", samplerType)==0)


trivial: could use jlib's streq

jakesmith · 2025-01-29T13:01:26Z

system/jlib/jtrace.cpp

+            else if (strcmp("Ratio", samplerType)==0)
+            {
+                size_t pos;
+                double ratio = std::stod(samplerArgument, &pos);


const char * samplerArgument = samplerTree->queryProp("@argument");

could move to here, as only use of argument, and do:

double ratio = samplerTree->getPropReal("@argument");

Agreed, it's meant to be a generic argument for future support, but in reality the ratio is likely to be the only use, will use your suggestion

jakesmith · 2025-01-29T13:03:02Z

system/jlib/jtrace.cpp

+
+            if (sampler && samplerTree->getPropBool("@parentBased", true))
+            {
+                return std::unique_ptr<opentelemetry::sdk::trace::ParentBasedSampler>(new opentelemetry::sdk::trace::ParentBasedSampler( std::move(sampler)));


trivial/formatting: extra unintended space before std::move in 'ParentBasedSampler( std::move(sampler))'

jakesmith · 2025-01-29T13:05:00Z

system/jlib/jtrace.cpp

+    if (!sampler) 
+    {
+        sampler = std::unique_ptr<opentelemetry::sdk::trace::AlwaysOnSampler>
+                                (new opentelemetry::sdk::trace::AlwaysOnSampler);


to be consistent, as per line 1378:

sampler.reset(new opentelemetry::sdk::trace::AlwaysOnSampler());

rpastrana · 2025-02-01T01:18:08Z

system/jlib/jtrace.cpp

@@ -768,9 +768,6 @@ class CSpan : public CInterfaceOf<ISpan>
        if (span == nullptr)
            return false;

-        if (!span->IsRecording()) //if not sampled, we shouldn't consider this valid?


maybe outside the scope of this issue, but found this causes a hang under certain parentbased configurations -- further analysis is needed, but removing this for now

@jakesmith this amended commit includes the code review changes you requested and approved + this change

jakesmith

@rpastrana - looks good. Please squash.

rpastrana · 2025-02-07T18:44:22Z

Do not merge yet, I'm not happy w/ the hang I've noticed (even if avoided by removing call to span->IsRecording

jakesmith

Do not merge yet, I'm not happy w/ the hang I've noticed (even if avoided by removing call to span->IsRecording

merely commenting, to keep the status of the PR on track (not pending review at this stage).

Do not merge yet, I'm not happy w/ the hang I've noticed (even if avoided by removing call to span->IsRecording

rpastrana · 2025-02-20T15:51:45Z

@jakesmith I've reviewed, and this is safe to merge.

jakesmith

@rpastrana - looks good. Please squash.

rpastrana · 2025-02-21T14:03:16Z

@ghalliday ready to be merged

Just noticed the conflicts (not sure how that happened), will resolve asap

Conflicts were resolved

rpastrana · 2025-02-25T18:07:10Z

@jakesmith in the process of rebasing due to conflicts, I noticed the sample yaml file was incorrect. Latest commit addresses that minor issue. Please review

jakesmith

@rpastrana - looks good afaics. Please squash.

rpastrana · 2025-02-26T15:44:12Z

Squashed and ready to merge

ghalliday

@rpastrana a few comments - mainly regarding ensuring the configuration makes sense from a user's point of view, rather than reflecting the underlying implementation.

ghalliday · 2025-03-03T11:28:37Z

system/jlib/jtrace.cpp

+    if (!sampler) 
+    {
+        sampler.reset(new opentelemetry::sdk::trace::AlwaysOnSampler());
+        WARNLOG("JTrace sampler set to 'Always ON' by default!");


I am not sure we want this tracing.
This if could be deleted and covered by the default case in the caller.

ghalliday · 2025-03-03T11:30:02Z

system/jlib/jtrace.cpp

+                double ratio = samplerTree->getPropReal("@argument");
+                if (ratio < 0 || ratio > 1)
+                {
+                    OERRLOG("JTrace invalid ratio sampler configuration. Ratio must be LE 1.0 or GE 0.0");


picky: "Ratio must be LE 1.0 or GE 0.0"
should be "and", and symbols probably clearer
"Ratio must >= 0.0 and <= 1.0" or "between 0.0 and 1.0"

really picky: For an if() statement, it is better to have the normal/expected case first. It is easier to read.

ghalliday · 2025-03-03T11:32:39Z

system/jlib/jtrace.cpp

+            }
+            else if (streq(samplerType,"Ratio"))
+            {
+                double ratio = samplerTree->getPropReal("@argument");


add a default value of -1, so that failing to specify a ratio is caught as a problem - rather than silently removing all items.

ghalliday · 2025-03-03T11:36:15Z

system/jlib/jtrace.cpp

@@ -1468,6 +1532,10 @@ Expected Configuration format:
        disabled: true                  #optional - disable OTel tracing
        alwaysCreateGlobalIds : false   #optional - should global ids always be created?
        alwaysCreateTraceIds            #optional - should trace ids always be created?
+        sampler:                        #optional - controls how traces are either suppressed or sampled
+          type:                         #"AlwaysOff" | "AlwaysOn" | "Ratio"
+          argument:                     #optional sampler type configuration value


From a user's point of view argument is very generic. ratio would be better.

You could argue this interface has been defined based on the implementation, rather than what would make sense from the user's viewpoint. I am not sure if you want to go this way, but you could remove the type field, and just rely on a ratio (or percentage) field. If it was 0 you would sample none, if 100 you would include all, otherwise you would create a ratio sampler.

ghalliday · 2025-03-03T11:38:09Z

system/jlib/jtrace.cpp

+        sampler:                        #optional - controls how traces are either suppressed or sampled
+          type:                         #"AlwaysOff" | "AlwaysOn" | "Ratio"
+          argument:                     #optional sampler type configuration value
+          parentBased:                  #optional sets the sampling decision based on the Span’s parent, or absence of parent, to know which secondary sampler to use.


What does this mean? does it mean exclude if the parent is not included in a sample? What does it mean from the user's perspective?

ghalliday · 2025-03-03T11:40:43Z

helm/hpcc/values.schema.json

+              "description": "Name of the Head Sampling type AlwaysOff|AlwaysOn|Ratio"
+            },
+            "argument" : {
+              "type": "string",


This would naturally be a number - see comments on "argument" v "ratio" below.

ghalliday · 2025-03-03T11:42:31Z

helm/hpcc/values.schema.json

@@ -1171,6 +1171,25 @@
          "type": "boolean",
          "description": "If true, sets OTEL library logging to debug level (otherwise set to warning)"
        },
+        "sampler": {


question: Would "sampling" be more intuitive to the user?

I think the usability improvement would be negligible.

ghalliday

A good set of changes, but I think the using a generic "argument" is wrong.
I don't think there are any other examples like it in the helm chart, and it isn't immediately obvious what it means, and requires the user to specify the value using the wrong type.

ghalliday

Looks good. Please squash, and a couple of things to tidy up.

ghalliday · 2025-03-05T09:22:24Z

helm/hpcc/values.schema.json

+              "enum": ["AlwaysOff", "AlwaysOn", "Ratio"],
+              "description": "Name of the Head Sampling type AlwaysOff|AlwaysOn|Ratio"
+            },
+              "ratio" : {


trivial: indented by 2 extra

ghalliday · 2025-03-05T09:24:03Z

system/jlib/jtrace.cpp

-            endpoint: "localhost:4317"    #exporter specific key/value pairs
+        sampling:                       #optional - controls how traces are either suppressed or sampled
+          type:                         #"AlwaysOff" | "AlwaysOn" | "Ratio"
+          argument:                     #optional - Generic argument dependent on sampling type used.


needs updating to refer to ratio instead.

rpastrana · 2025-03-05T20:40:12Z

squashed

- Adds Jtrace sampler configuration - Adds OTel sampler initialization logic - Updates JTrace configuration README - Provides samples - Jlog trace/span ids suppressed if not sampled (not sure if this is wanted) - Rename configuration element from sampler to sampling Signed-off-by: Rodrigo Pastrana <[email protected]>

github-actions · 2025-03-06T16:48:25Z

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

rpastrana requested a review from jakesmith January 24, 2025 20:16

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch 3 times, most recently from 5a8ea52 to c56a09b Compare January 28, 2025 22:11

jakesmith reviewed Jan 29, 2025

View reviewed changes

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 34965d1 to 1e85d43 Compare February 1, 2025 01:14

rpastrana requested a review from jakesmith February 1, 2025 01:14

rpastrana commented Feb 1, 2025

View reviewed changes

jakesmith previously approved these changes Feb 6, 2025

View reviewed changes

jakesmith self-requested a review February 13, 2025 17:14

jakesmith reviewed Feb 14, 2025

View reviewed changes

rpastrana requested a review from jakesmith February 20, 2025 15:51

jakesmith approved these changes Feb 21, 2025

View reviewed changes

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 1e85d43 to 3a50375 Compare February 21, 2025 14:02

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 3a50375 to 1653e52 Compare February 25, 2025 18:04

rpastrana requested a review from jakesmith February 25, 2025 18:05

jakesmith approved these changes Feb 26, 2025

View reviewed changes

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 1653e52 to 38d3a05 Compare February 26, 2025 15:42

ghalliday requested changes Mar 3, 2025

View reviewed changes

rpastrana requested a review from ghalliday March 3, 2025 19:55

ghalliday requested changes Mar 4, 2025

View reviewed changes

rpastrana requested a review from ghalliday March 4, 2025 23:41

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 5f23509 to 3f9dd92 Compare March 4, 2025 23:47

ghalliday approved these changes Mar 5, 2025

View reviewed changes

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 3f9dd92 to 4fceee8 Compare March 5, 2025 20:38

rpastrana force-pushed the HPCC-33298-JTrace-Sampling branch from 4fceee8 to 2bc7186 Compare March 6, 2025 15:02

ghalliday merged commit b3b6ad6 into hpcc-systems:candidate-9.8.x Mar 6, 2025
51 of 52 checks passed

HPCC-33298 JTrace support sampling configuration #19456

HPCC-33298 JTrace support sampling configuration #19456

Conversation

rpastrana commented Jan 24, 2025 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Jan 24, 2025

rpastrana commented Jan 28, 2025

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

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

rpastrana commented Feb 7, 2025

jakesmith left a comment

Choose a reason for hiding this comment

rpastrana commented Feb 20, 2025

jakesmith left a comment

Choose a reason for hiding this comment

rpastrana commented Feb 21, 2025 • edited Loading

rpastrana commented Feb 25, 2025

jakesmith left a comment

Choose a reason for hiding this comment

rpastrana commented Feb 26, 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

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

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

rpastrana commented Mar 5, 2025

github-actions bot commented Mar 6, 2025

rpastrana commented Jan 24, 2025 •

edited

Loading

rpastrana commented Feb 21, 2025 •

edited

Loading