HPCC-33298 Code review

- Rename configuration element from sampler to sampling
- Update documentation, and samples with new config element name
- Reversed conditional flow order
- Updated error message
- Removed duplicate default sampler creation
- Updated developer comments

Signed-off-by: Rodrigo Pastrana <[email protected]>

Author: rpastrana

helm/examples/tracing/README.md

@@ -13,10 +13,10 @@ All configuration options detailed here are part of the HPCC Systems Helm chart,
 - optAlwaysCreateTraceIds - If true components generate trace/span ids if none are provided by the remote caller.
 - enableDefaultLogExporter - If true, creates a trace exporter outputting to the log using the default options
 - enableOTELDebugLogging - If true, OTel library logging level set to debug, otherwise warning
-- sampler - Defines head sampling strategy. Decision to sample or drop a span or trace is not made by inspecting the trace as a whole. https://opentelemetry.io/docs/concepts/sampling/
+- sampling - Defines head sampling strategy. Decision to sample or drop a span or trace is not made by inspecting the trace as a whole. https://opentelemetry.io/docs/concepts/sampling/
   - type "AlwaysOff" | "AlwaysOn" | "Ratio"
-  - argument - Optional sampler type configuration value. Currently, only supported value applies to the "Ratio" sampler type. The argument value is a string representing a numeric value betwen 0.0 and 1.0. This value represents the ratio of trace/spans to sample
-  - parentBased - Optional boolean. Determines if the sampler policy honors the remote root span sampled flag
+  - argument - Optional sampling type configuration value. Currently, only supported value applies to the "Ratio" sampling type. The argument value is a string representing a numeric value betwen 0.0 and 1.0. This value represents the ratio of trace/spans to sample
+  - parentBased - Optional boolean. Determines if the sampling policy honors the remote root span sampled flag
 - resourceAttributes: - Defines OTel specific resource attribute configuration values
                         which are appended to the runtime OTEL_RESOURCE_ATTRIBUTES. See OTel doc: https://opentelemetry.io/docs/specs/otel/configuration/sdk-environment-variables/#general-sdk-configuration
   - deploymentEnvironment - Defines deployment.environment, which is used to specify

helm/examples/tracing/baremetal-otlp-http-localhost-sample.xml

@@ -5,7 +5,7 @@
    <exporters consoleDebug="true" endpoint="http://localhost:4318/v1/traces" type="OTLP-HTTP">
     <batch enabled="false" maxQueueSize="4095" scheduledDelayMillis="6001" maxExportBatchSize="511"/>
    </exporters>
-   <sampler type="Ratio" argument="0.1" parentBased="true"/>
+   <sampling type="Ratio" argument="0.1" parentBased="true"/>
   </tracing>
  </Software>
 </Environment>

helm/examples/tracing/otlp-http-collector-default-sampled.yaml

@@ -1,6 +1,6 @@
 global:
   tracing:
-    sampler:
+    sampling:
       type: Ratio       #Head sampling based on simple ratio
       argument: "0.1"   #only sample 10% of traces/spans
       parentBased: true #Sampling strategy based on parent's sampling

helm/hpcc/values.schema.json

@@ -1171,7 +1171,7 @@
           "type": "boolean",
           "description": "If true, sets OTEL library logging to debug level (otherwise set to warning)"
         },
-        "sampler": {
+        "sampling": {
           "type": "object",
           "properties": {
             "type": {
@@ -1181,11 +1181,11 @@
             },
             "argument" : {
               "type": "string",
-              "description": "Optional sampler type configuration value"
+              "description": "Optional - Sampling type specific configuration value. Such as the ratio for the Ratio sampling type."
             },
             "parentBased" : {
               "type": "boolean",
-              "description": "Optional sets the sampling decision based on the Span’s parent, or absence of parent, to know which secondary sampler to use."
+              "description": "Optional - Determines if the sampling policy honors the remote root span sampled flag."
             },
             "additionalProperties": false
           }

system/jlib/jtrace.cpp

@@ -1407,19 +1407,19 @@ static std::unique_ptr<opentelemetry::sdk::trace::Sampler> createSampler(IProper
             }
             else if (streq(samplerType,"Ratio"))
             {
-                double ratio = samplerTree->getPropReal("@argument");
-                if (ratio < 0 || ratio > 1)
+                double ratio = samplerTree->getPropReal("@argument", -1.0);
+                if (ratio >= 0.0 && ratio <= 1.0)
                 {
-                    OERRLOG("JTrace invalid ratio sampler configuration. Ratio must be LE 1.0 or GE 0.0");
+                    sampler.reset(new opentelemetry::sdk::trace::TraceIdRatioBasedSampler(ratio));
                 }
                 else
                 {
-                    sampler.reset(new opentelemetry::sdk::trace::TraceIdRatioBasedSampler(ratio));
+                    OERRLOG("JTrace invalid ratio sampling configuration. Ratio must be between 0.0 and 1.0");
                 }
             }
             else
             {
-                WARNLOG("JTrace initialization: Invalid sampler type configured: '%s'", samplerType);
+                WARNLOG("JTrace initialization: Invalid sampling type configured: '%s'", samplerType);
             }
 
             if (sampler && samplerTree->getPropBool("@parentBased", true))
@@ -1429,12 +1429,6 @@ static std::unique_ptr<opentelemetry::sdk::trace::Sampler> createSampler(IProper
         }
     }
 
-    if (!sampler) 
-    {
-        sampler.reset(new opentelemetry::sdk::trace::AlwaysOnSampler());
-        WARNLOG("JTrace sampler set to 'Always ON' by default!");
-    }
-
     return sampler;
 }
 
@@ -1464,7 +1458,7 @@ void CTraceManager::initTracerProviderAndGlobalInternals(const IPropertyTree * t
     bool enableDefaultLogExporter = isDebugBuild();
     if (traceConfig)
     {
-        sampler = createSampler(traceConfig->queryPropTree("sampler"));
+        sampler = createSampler(traceConfig->queryPropTree("sampling"));
 
         IPropertyTree * resourceAttributesTree = traceConfig->queryPropTree("resourceAttributes");
         if (resourceAttributesTree)
@@ -1532,13 +1526,16 @@ 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
+        sampling:                       #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.
-        exporters:                       #optional - Controls how trace data is exported/reported
-        -   type: OTLP                    #OS|OTLP|Prometheus|JLOG
-            endpoint: "localhost:4317"    #exporter specific key/value pairs
+          argument:                     #optional - Generic argument dependent on sampling type used.
+                                        #           For example, if Ratio sampling used, 'argument' represents
+                                        #           the sampling ratio [0.0 - 1.0]
+          parentBased:                  #optional - Sets OTel's parentbased sampling option as defined here:
+                                        #           https://opentelemetry.io/docs/languages/erlang/sampling/#parentbasedsampler
+        exporters:                      #optional - Controls how trace data is exported/reported
+        -   type: OTLP                  #OS|OTLP|Prometheus|JLOG
+            endpoint: "localhost:4317"  #exporter specific key/value pairs
             useSslCredentials: true
             sslCredentialsCACcert: "ssl-certificate"
             batch:                        #optional - Controls span processing style