refactor: apply review feedback — move rules parser, simplify helpers, catch specifics

bm1549 · claude · bm1549 · commit 32d2d5b47719 · 2026-03-21T10:43:03.000-04:00
Co-Authored-By: Claude Opus 4.6 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/WORKSPACE b/WORKSPACE
@@ -40,6 +40,9 @@ http_archive(
     urls = ["https://github.com/bazelbuild/rules_cc/releases/download/0.2.14/rules_cc-0.2.14.tar.gz"],
 )
 
+# This pulls the upstream yaml-cpp 0.8.0 tarball directly.
+# MODULE.bazel uses "0.8.0.bcr.1" because that is the BCR (Bazel Central
+# Registry) patched release; the underlying library version is the same 0.8.0.
 http_archive(
     name = "yaml_cpp",
     sha256 = "fbe74bbdcee21d656715688706da3c8becfd946d92cd44705cc6098bb23b3a16",
diff --git a/include/datadog/config.h b/include/datadog/config.h
@@ -64,6 +64,11 @@ struct ConfigMetadata {
       : name(n), value(std::move(v)), origin(orig), error(std::move(err)) {}
 };
 
+// 3-parameter overload (env, user, default) kept for backward compatibility
+// with external projects (e.g., nginx-datadog, httpd-datadog) that include
+// this public header.  New internal code should prefer the 5-parameter
+// overload that also accepts fleet and local stable config sources.
+//
 // Returns the final configuration value using the following
 // precedence order: environment > user code > default, and populates metadata:
 // `metadata`: Records ALL configuration sources that were provided,
diff --git a/include/datadog/trace_sampler_config.h b/include/datadog/trace_sampler_config.h
@@ -20,6 +20,7 @@
 namespace datadog {
 namespace tracing {
 
+class Logger;
 struct StableConfigs;
 
 struct TraceSamplerRule final {
@@ -44,7 +45,8 @@ struct TraceSamplerConfig {
 
 class FinalizedTraceSamplerConfig {
   friend Expected<FinalizedTraceSamplerConfig> finalize_config(
-      const TraceSamplerConfig& config, const StableConfigs* stable_configs);
+      const TraceSamplerConfig& config, const StableConfigs* stable_configs,
+      Logger* logger);
   friend class FinalizedTracerConfig;
 
   FinalizedTraceSamplerConfig() = default;
@@ -61,7 +63,7 @@ class FinalizedTraceSamplerConfig {
 
 Expected<FinalizedTraceSamplerConfig> finalize_config(
     const TraceSamplerConfig& config,
-    const StableConfigs* stable_configs = nullptr);
+    const StableConfigs* stable_configs = nullptr, Logger* logger = nullptr);
 
 }  // namespace tracing
 }  // namespace datadog
diff --git a/src/datadog/span_sampler_config.cpp b/src/datadog/span_sampler_config.cpp
@@ -15,6 +15,55 @@ namespace datadog {
 namespace tracing {
 namespace {
 
+// Parse a stable config JSON string as an array of sampling rules.
+// `customize_rule` is a callable that receives (Rule&, const json_rule&) to set
+// rule-specific fields beyond the base matcher and sample_rate.
+// Returns nullopt on any parse error (stable config errors are non-fatal).
+template <typename Rule, typename Json, typename Customize>
+Optional<std::vector<Rule>> parse_stable_config_rules(
+    const StableConfig &cfg, const std::string &key, Logger &logger,
+    Customize customize_rule) {
+  auto val = cfg.lookup(key);
+  if (!val || val->empty()) return nullopt;
+
+  try {
+    auto json_rules = Json::parse(*val);
+    if (!json_rules.is_array()) {
+      logger.log_error([&key](std::ostream &log) {
+        log << "Unable to parse JSON sampling rules from " << key
+            << ": expected a JSON array";
+      });
+      return nullopt;
+    }
+
+    std::vector<Rule> rules;
+    for (const auto &json_rule : json_rules) {
+      auto matcher = from_json(json_rule);
+      if (matcher.if_error()) {
+        logger.log_error([&key](std::ostream &log) {
+          log << "Unable to parse JSON sampling rules from " << key
+              << ": invalid rule matcher";
+        });
+        return nullopt;
+      }
+
+      Rule rule{*matcher};
+      if (auto sr = json_rule.find("sample_rate");
+          sr != json_rule.end() && sr->is_number()) {
+        rule.sample_rate = *sr;
+      }
+      customize_rule(rule, json_rule);
+      rules.emplace_back(std::move(rule));
+    }
+    return rules;
+  } catch (...) {
+    logger.log_error([&key](std::ostream &log) {
+      log << "Unable to parse JSON sampling rules from " << key;
+    });
+    return nullopt;
+  }
+}
+
 std::string to_string(const std::vector<SpanSamplerConfig::Rule> &rules) {
   nlohmann::json res;
   for (const auto &r : rules) {
@@ -242,10 +291,10 @@ Expected<FinalizedSpanSamplerConfig> finalize_config(
   Optional<std::vector<SpanSamplerConfig::Rule>> fleet_rules;
   Optional<std::vector<SpanSamplerConfig::Rule>> local_rules;
   if (stable_configs) {
-    auto parse_span_rules = [](const StableConfig &cfg,
-                               const std::string &key) {
+    auto parse_span_rules = [&logger](const StableConfig &cfg,
+                                      const std::string &key) {
       return parse_stable_config_rules<SpanSamplerConfig::Rule, nlohmann::json>(
-          cfg, key,
+          cfg, key, logger,
           [](SpanSamplerConfig::Rule &rule, const nlohmann::json &json_rule) {
             if (auto mps = json_rule.find("max_per_second");
                 mps != json_rule.end() && mps->is_number()) {
diff --git a/src/datadog/stable_config.h b/src/datadog/stable_config.h
@@ -17,9 +17,6 @@
 
 #include <string>
 #include <unordered_map>
-#include <vector>
-
-#include "json_serializer.h"
 
 namespace datadog {
 namespace tracing {
@@ -55,38 +52,5 @@ struct StableConfigs {
 // Returns empty configs (no error) if files don't exist.
 StableConfigs load_stable_configs(Logger& logger);
 
-// Parse a stable config JSON string as an array of sampling rules.
-// `customize_rule` is a callable that receives (Rule&, const json_rule&) to set
-// rule-specific fields beyond the base matcher and sample_rate.
-// Returns nullopt on any parse error (stable config errors are non-fatal).
-template <typename Rule, typename Json, typename Customize>
-Optional<std::vector<Rule>> parse_stable_config_rules(
-    const StableConfig& cfg, const std::string& key, Customize customize_rule) {
-  auto val = cfg.lookup(key);
-  if (!val || val->empty()) return nullopt;
-
-  try {
-    auto json_rules = Json::parse(*val);
-    if (!json_rules.is_array()) return nullopt;
-
-    std::vector<Rule> rules;
-    for (const auto& json_rule : json_rules) {
-      auto matcher = from_json(json_rule);
-      if (matcher.if_error()) return nullopt;
-
-      Rule rule{*matcher};
-      if (auto sr = json_rule.find("sample_rate");
-          sr != json_rule.end() && sr->is_number()) {
-        rule.sample_rate = *sr;
-      }
-      customize_rule(rule, json_rule);
-      rules.emplace_back(std::move(rule));
-    }
-    return rules;
-  } catch (...) {
-    return nullopt;
-  }
-}
-
 }  // namespace tracing
 }  // namespace datadog
diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp
@@ -1,4 +1,5 @@
 #include <datadog/environment.h>
+#include <datadog/logger.h>
 #include <datadog/trace_sampler_config.h>
 #include <datadog/trace_source.h>
 
@@ -7,6 +8,7 @@
 
 #include "json.hpp"
 #include "json_serializer.h"
+#include "null_logger.h"
 #include "parse_util.h"
 #include "stable_config.h"
 #include "string_util.h"
@@ -156,20 +158,74 @@ Optional<double> stable_config_double(const StableConfig &cfg,
   return *result;
 }
 
+// Parse a stable config JSON string as an array of sampling rules.
+// `customize_rule` is a callable that receives (Rule&, const json_rule&) to set
+// rule-specific fields beyond the base matcher and sample_rate.
+// Returns nullopt on any parse error (stable config errors are non-fatal).
+template <typename Rule, typename Json, typename Customize>
+Optional<std::vector<Rule>> parse_stable_config_rules(
+    const StableConfig &cfg, const std::string &key, Logger &logger,
+    Customize customize_rule) {
+  auto val = cfg.lookup(key);
+  if (!val || val->empty()) return nullopt;
+
+  try {
+    auto json_rules = Json::parse(*val);
+    if (!json_rules.is_array()) {
+      logger.log_error([&key](std::ostream &log) {
+        log << "Unable to parse JSON sampling rules from " << key
+            << ": expected a JSON array";
+      });
+      return nullopt;
+    }
+
+    std::vector<Rule> rules;
+    for (const auto &json_rule : json_rules) {
+      auto matcher = from_json(json_rule);
+      if (matcher.if_error()) {
+        logger.log_error([&key](std::ostream &log) {
+          log << "Unable to parse JSON sampling rules from " << key
+              << ": invalid rule matcher";
+        });
+        return nullopt;
+      }
+
+      Rule rule{*matcher};
+      if (auto sr = json_rule.find("sample_rate");
+          sr != json_rule.end() && sr->is_number()) {
+        rule.sample_rate = *sr;
+      }
+      customize_rule(rule, json_rule);
+      rules.emplace_back(std::move(rule));
+    }
+    return rules;
+  } catch (...) {
+    logger.log_error([&key](std::ostream &log) {
+      log << "Unable to parse JSON sampling rules from " << key;
+    });
+    return nullopt;
+  }
+}
+
 // Try to parse a stable config string value as trace sampling rules JSON.
-// Returns empty vector on any parse error (stable config errors are non-fatal).
+// Returns nullopt on any parse error (stable config errors are non-fatal).
 Optional<std::vector<TraceSamplerConfig::Rule>> stable_config_sampling_rules(
-    const StableConfig &cfg, const std::string &key) {
+    const StableConfig &cfg, const std::string &key, Logger &logger) {
   return parse_stable_config_rules<TraceSamplerConfig::Rule, nlohmann::json>(
-      cfg, key, [](TraceSamplerConfig::Rule &, const nlohmann::json &) {});
+      cfg, key, logger,
+      [](TraceSamplerConfig::Rule &, const nlohmann::json &) {});
 }
 
 }  // namespace
 
 TraceSamplerConfig::Rule::Rule(const SpanMatcher &base) : SpanMatcher(base) {}
 
 Expected<FinalizedTraceSamplerConfig> finalize_config(
-    const TraceSamplerConfig &config, const StableConfigs *stable_configs) {
+    const TraceSamplerConfig &config, const StableConfigs *stable_configs,
+    Logger *logger) {
+  NullLogger null_logger;
+  Logger &log = logger ? *logger : static_cast<Logger &>(null_logger);
+
   Expected<TraceSamplerConfig> env_config = load_trace_sampler_env_config();
   if (auto error = env_config.if_error()) {
     return *error;
@@ -184,9 +240,9 @@ Expected<FinalizedTraceSamplerConfig> finalize_config(
   Optional<std::vector<TraceSamplerConfig::Rule>> local_rules;
   if (stable_configs) {
     fleet_rules = stable_config_sampling_rules(stable_configs->fleet,
-                                               "DD_TRACE_SAMPLING_RULES");
+                                               "DD_TRACE_SAMPLING_RULES", log);
     local_rules = stable_config_sampling_rules(stable_configs->local,
-                                               "DD_TRACE_SAMPLING_RULES");
+                                               "DD_TRACE_SAMPLING_RULES", log);
   }
 
   if (fleet_rules) {
diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp
diff --git a/src/datadog/yaml_parser.cpp b/src/datadog/yaml_parser.cpp

Original file line number	Diff line number	Diff line change
`@@ -40,6 +40,9 @@ http_archive(`
`40`	`40`	`urls = ["https://github.com/bazelbuild/rules_cc/releases/download/0.2.14/rules_cc-0.2.14.tar.gz"],`
`41`	`41`	`)`
`42`	`42`
	`43`	`+# This pulls the upstream yaml-cpp 0.8.0 tarball directly.`
	`44`	`+# MODULE.bazel uses "0.8.0.bcr.1" because that is the BCR (Bazel Central`
	`45`	`+# Registry) patched release; the underlying library version is the same 0.8.0.`
`43`	`46`	`http_archive(`
`44`	`47`	`name = "yaml_cpp",`
`45`	`48`	`sha256 = "fbe74bbdcee21d656715688706da3c8becfd946d92cd44705cc6098bb23b3a16",`