fix(rc): improve trace sampling rules parsing (#232)

dmehala · web-flow · commit 274149a14b6c · 2025-09-03T11:38:08.000+02:00
Correctly parse `tags` from remote trace sampling rules and improves
overall rule parsing reliability through stricter validation and clearer
error reporting.

Changes:
  - Add support for `tags` in trace sampling rules.
  - Improve validation with explicit type checks and more descriptive
    error messages.
  - Update catch-all rule handling to replace an existing one instead of
    always prepending, ensuring consistent rule ordering.
  - Extended system test request handler to support `span_tags` from
    incoming requests.

[APMAPI-863]
[APMAPI-866]
diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp
@@ -2,7 +2,6 @@
 
 #include <datadog/telemetry/telemetry.h>
 
-#include "json_serializer.h"
 #include "parse_util.h"
 #include "string_util.h"
 #include "trace_sampler.h"
@@ -26,69 +25,168 @@ nlohmann::json to_json(const SpanDefaults& defaults) {
 }
 
 using Rules = std::vector<TraceSamplerRule>;
+using Tags = std::unordered_map<std::string, std::string>;
+
+Expected<Tags> parse_tags_from_sampling_rules(const nlohmann::json& json_tags) {
+  assert(json_tags.is_array());
+
+  Tags tags;
+  for (const auto& json_tag_entry : json_tags) {
+    auto key = json_tag_entry.find("key");
+    if (key == json_tag_entry.cend() || key->is_string() == false) {
+      std::string err_msg =
+          "Failed to parse tags: the required \"key\" field is either missing "
+          "or incorrectly formatted. (input: ";
+      err_msg += json_tags.dump();
+      err_msg += ")";
+      return Error{Error::TRACE_SAMPLING_RULES_INVALID_JSON,
+                   std::move(err_msg)};
+    }
 
-Expected<Rules> parse_trace_sampling_rules(const nlohmann::json& json_rules) {
-  Rules parsed_rules;
+    auto value = json_tag_entry.find("value_glob");
+    if (value == json_tag_entry.cend() || value->is_string() == false) {
+      std::string err_msg =
+          "Failed to parse tags: the required \"value_glob\" field is either "
+          "missing or incorrectly formatted. (input: ";
+      err_msg += json_tags.dump();
+      err_msg += ")";
+      return Error{Error::TRACE_SAMPLING_RULES_INVALID_JSON,
+                   std::move(err_msg)};
+    }
+
+    tags.emplace(*key, *value);
+  }
+
+  return tags;
+}
 
-  std::string type = json_rules.type_name();
-  if (type != "array") {
+Expected<TraceSamplerRule> parse_rule(const nlohmann::json& json_rule) {
+  assert(json_rule.is_object());
+
+  auto make_error = [&json_rule](StringView field_name) {
+    std::string err_msg = "Failed to parse sampling rule: the required \"";
+    append(err_msg, field_name);
+    err_msg += "\" field is missing. (input: ";
+    err_msg += json_rule.dump();
+    err_msg += ")";
+    return Error{Error::TRACE_SAMPLING_RULES_INVALID_JSON, std::move(err_msg)};
+  };
+
+  const auto make_property_error = [&json_rule](StringView property,
+                                                const nlohmann::json& value,
+                                                StringView expected_type) {
     std::string message;
-    return Error{Error::TRACE_SAMPLING_RULES_WRONG_TYPE, std::move(message)};
+    message += "Rule property \"";
+    append(message, property);
+    message += "\" should have type \"";
+    append(message, expected_type);
+    message += "\", but has type \"";
+    message += value.type_name();
+    message += "\": ";
+    message += value.dump();
+    message += " in rule ";
+    message += json_rule.dump();
+    return Error{Error::RULE_PROPERTY_WRONG_TYPE, std::move(message)};
+  };
+
+  TraceSamplerRule rule;
+
+  // Required: service, resource, sample_rate, provenance.
+  if (auto service = json_rule.find("service"); service != json_rule.cend()) {
+    if (service->is_string() == false) {
+      return make_property_error("service", *service, "string");
+    }
+    rule.matcher.service = *service;
+  } else {
+    return make_error("service");
   }
 
-  for (const auto& json_rule : json_rules) {
-    auto matcher = from_json(json_rule);
-    if (auto* error = matcher.if_error()) {
-      std::string prefix;
-      return error->with_prefix(prefix);
+  if (auto resource = json_rule.find("resource");
+      resource != json_rule.cend()) {
+    if (resource->is_string() == false) {
+      return make_property_error("resource", *resource, "string");
     }
+    rule.matcher.resource = *resource;
+  } else {
+    return make_error("resource");
+  }
 
-    TraceSamplerRule rule;
-    rule.matcher = std::move(*matcher);
-
-    if (auto sample_rate = json_rule.find("sample_rate");
-        sample_rate != json_rule.end()) {
-      type = sample_rate->type_name();
-      if (type != "number") {
-        std::string message;
-        return Error{Error::TRACE_SAMPLING_RULES_SAMPLE_RATE_WRONG_TYPE,
-                     std::move(message)};
-      }
+  if (auto sample_rate = json_rule.find("sample_rate");
+      sample_rate != json_rule.end()) {
+    if (sample_rate->is_number() == false) {
+      return make_property_error("sample_rate", *sample_rate, "number");
+    }
 
-      auto maybe_rate = Rate::from(*sample_rate);
-      if (auto error = maybe_rate.if_error()) {
-        return *error;
-      }
+    auto maybe_rate = Rate::from(*sample_rate);
+    if (auto error = maybe_rate.if_error()) {
+      return *error;
+    }
 
-      rule.rate = *maybe_rate;
+    rule.rate = *maybe_rate;
+  } else {
+    return make_error("sample_rate");
+  }
+
+  if (auto provenance_it = json_rule.find("provenance");
+      provenance_it != json_rule.cend()) {
+    if (!provenance_it->is_string()) {
+      return make_property_error("provenance", *provenance_it, "string");
+    }
+
+    auto provenance = to_lower(provenance_it->get<StringView>());
+    if (provenance == "customer") {
+      rule.mechanism = SamplingMechanism::REMOTE_RULE;
+    } else if (provenance == "dynamic") {
+      rule.mechanism = SamplingMechanism::REMOTE_ADAPTIVE_RULE;
     } else {
-      return Error{Error::TRACE_SAMPLING_RULES_INVALID_JSON,
-                   "Missing \"sample_rate\" field"};
+      std::string err_msg = "Failed to parse sampling rule: unknown \"";
+      err_msg += provenance;
+      err_msg += "\" value. Expected either \"customer\" or \"dynamic\"";
+      return Error{Error::TRACE_SAMPLING_RULES_UNKNOWN_PROPERTY,
+                   std::move(err_msg)};
+    }
+  } else {
+    return make_error("provenance");
+  }
+
+  // Parse optional fields: name, tags
+  if (auto name = json_rule.find("name"); name != json_rule.cend()) {
+    if (name->is_string() == false) {
+      return make_property_error("name", *name, "string");
     }
+    rule.matcher.name = *name;
+  }
 
-    if (auto provenance_it = json_rule.find("provenance");
-        provenance_it != json_rule.cend()) {
-      if (!provenance_it->is_string()) {
-        std::string message;
-        return Error{Error::TRACE_SAMPLING_RULES_SAMPLE_RATE_WRONG_TYPE,
-                     std::move(message)};
-      }
+  if (auto tags = json_rule.find("tags"); tags != json_rule.cend()) {
+    if (tags->is_array() == false) {
+      return make_property_error("tags", *tags, "array");
+    }
 
-      auto provenance = to_lower(provenance_it->get<StringView>());
-      if (provenance == "customer") {
-        rule.mechanism = SamplingMechanism::REMOTE_RULE;
-      } else if (provenance == "dynamic") {
-        rule.mechanism = SamplingMechanism::REMOTE_ADAPTIVE_RULE;
-      } else {
-        return Error{Error::TRACE_SAMPLING_RULES_UNKNOWN_PROPERTY,
-                     "Unknown \"provenance\" value"};
-      }
-    } else {
-      return Error{Error::TRACE_SAMPLING_RULES_INVALID_JSON,
-                   "Missing \"provenance\" field"};
+    auto maybe_tags = parse_tags_from_sampling_rules(*tags);
+    if (auto* error = maybe_tags.if_error()) {
+      return *error;
+    }
+
+    rule.matcher.tags = std::move(*maybe_tags);
+  }
+
+  return rule;
+}
+
+Expected<Rules> parse_trace_sampling_rules(const nlohmann::json& json_rules) {
+  if (json_rules.is_array() == false) {
+    std::string message;
+    return Error{Error::TRACE_SAMPLING_RULES_WRONG_TYPE, std::move(message)};
+  }
+
+  Rules parsed_rules;
+  for (const auto& json_rule : json_rules) {
+    auto maybe_rule = parse_rule(json_rule);
+    if (auto error = maybe_rule.if_error()) {
+      return *error;
     }
 
-    parsed_rules.emplace_back(std::move(rule));
+    parsed_rules.emplace_back(std::move(*maybe_rule));
   }
 
   return parsed_rules;
@@ -215,7 +313,19 @@ std::vector<ConfigMetadata> ConfigManager::apply_update(
     rule.rate = *rate;
     rule.matcher = catch_all;
     rule.mechanism = SamplingMechanism::RULE;
-    rules.emplace(rules.cbegin(), std::move(rule));
+
+    // Convention: Catch-all rules should ALWAYS be the last in the list.
+    // If a catch-all rule already exists, replace it.
+    // If NOT, add the new one at the end of the rules list.
+    if (rules.empty()) {
+      rules.emplace_back(std::move(rule));
+    } else {
+      if (auto& last_rule = rules.back(); last_rule.matcher == catch_all) {
+        last_rule = rule;
+      } else {
+        rules.emplace_back(std::move(rule));
+      }
+    }
 
     metadata.emplace_back(std::move(trace_sampling_metadata));
   }
diff --git a/src/datadog/json_serializer.h b/src/datadog/json_serializer.h
@@ -17,20 +17,19 @@ inline void to_json(nlohmann::json& j, const SpanMatcher& matcher) {
 inline Expected<SpanMatcher> from_json(const nlohmann::json& json) {
   SpanMatcher result;
 
-  std::string type = json.type_name();
-  if (type != "object") {
+  if (json.is_object() == false) {
     std::string message;
     message += "A rule must be a JSON object, but this is of type \"";
-    message += type;
+    message += json.type_name();
     message += "\": ";
     message += json.dump();
     return Error{Error::RULE_WRONG_TYPE, std::move(message)};
   }
 
   const auto check_property_type =
-      [&](StringView property, const nlohmann::json& value,
-          StringView expected_type) -> Optional<Error> {
-    type = value.type_name();
+      [&json](StringView property, const nlohmann::json& value,
+              StringView expected_type) -> Optional<Error> {
+    const StringView type = value.type_name();
     if (type == expected_type) {
       return nullopt;
     }
@@ -41,7 +40,7 @@ inline Expected<SpanMatcher> from_json(const nlohmann::json& json) {
     message += "\" should have type \"";
     append(message, expected_type);
     message += "\", but has type \"";
-    message += type;
+    append(message, type);
     message += "\": ";
     message += value.dump();
     message += " in rule ";
@@ -70,13 +69,12 @@ inline Expected<SpanMatcher> from_json(const nlohmann::json& json) {
         return *error;
       }
       for (const auto& [tag_name, tag_value] : value.items()) {
-        type = tag_value.type_name();
-        if (type != "string") {
+        if (tag_value.is_string() == false) {
           std::string message;
           message += "Rule tag pattern must be a string, but ";
           message += tag_value.dump();
           message += " has type \"";
-          message += type;
+          message += tag_value.type_name();
           message += "\" for tag named \"";
           message += tag_name;
           message += "\" in rule: ";
@@ -86,7 +84,7 @@ inline Expected<SpanMatcher> from_json(const nlohmann::json& json) {
         result.tags.emplace(std::string(tag_name), std::string(tag_value));
       }
     } else {
-      // Unknown properties are OK.  `SpanMatcher` is used as a base class for
+      // Unknown properties are OK. `SpanMatcher` is used as a base class for
       // trace sampling rules and span sampling rules.  Those derived types
       // will have additional properties in their JSON representations.
     }
diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp
@@ -209,7 +209,7 @@ Expected<FinalizedTraceSamplerConfig> finalize_config(
   }
 
   // If `sample_rate` was specified, then it translates to a "catch-all" rule
-  // appended to the end of `rules`.  First, though, we have to make sure the
+  // appended to the end of `rules`. First, though, we have to make sure the
   // sample rate is valid.
   if (sample_rate) {
     auto maybe_rate = Rate::from(*sample_rate);
diff --git a/test/system-tests/request_handler.cpp b/test/system-tests/request_handler.cpp
@@ -70,6 +70,9 @@ void RequestHandler::on_trace_config(const httplib::Request& /* req */,
   res.set_content(response_body.dump(), "application/json");
 }
 
+// TODO: Refact endpoint handler to return 404 when an unknown field is passed
+// in the payload. that would send a clear message instead of silently creating
+// a span.
 void RequestHandler::on_span_start(const httplib::Request& req,
                                    httplib::Response& res) {
   const auto request_json = nlohmann::json::parse(req.body);
@@ -94,6 +97,23 @@ void RequestHandler::on_span_start(const httplib::Request& req,
     span_cfg.resource = *resource;
   }
 
+  if (auto tags = request_json.find("span_tags");
+      tags != request_json.cend() && tags->is_array()) {
+    for (const auto& tag : *tags) {
+      if (tag.size() != 2) {
+        // TODO: refactor to log errors
+        continue;
+      }
+
+      if (tag[0].is_string() == false || tag[1].is_string() == false) {
+        // TODO: refactor to log errors
+        continue;
+      }
+
+      span_cfg.tags.emplace(tag[0], tag[1]);
+    }
+  }
+
   auto success = [](const datadog::tracing::Span& span,
                     httplib::Response& res) {
     // clang-format off
diff --git a/test/test_config_manager.cpp b/test/test_config_manager.cpp
@@ -184,4 +184,41 @@ CONFIG_MANAGER_TEST("remote configuration handling") {
     const auto reverted_tracing_status = config_manager.report_traces();
     CHECK(old_tracing_status == reverted_tracing_status);
   }
+
+  SECTION("handling of `tracing_sampling_rules`") {
+    SECTION("valid input") {
+      config_update.content = R"({
+        "lib_config": {
+          "library_language": "all",
+          "library_version": "latest",
+          "service_name": "testsvc",
+          "env": "test",
+          "tracing_sampling_rules": [
+            {
+              "service": "foo",
+              "resource": "GET /hello",
+              "sample_rate": 0.1,
+              "provenance": "customer",
+              "name": "test",
+              "tags": [
+                { "key": "tag1", "value_glob": "value1" }
+              ]
+            }
+          ]
+        },
+        "service_target": {
+           "service": "testsvc",
+           "env": "test"
+        }
+      })";
+
+      const auto old_sampler_cfg =
+          config_manager.trace_sampler()->config_json();
+      const auto err = config_manager.on_update(config_update);
+      const auto new_sampler_cfg =
+          config_manager.trace_sampler()->config_json();
+
+      CHECK(old_sampler_cfg != new_sampler_cfg);
+    }
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -209,7 +209,7 @@ Expected<FinalizedTraceSamplerConfig> finalize_config(`
`209`	`209`	`}`
`210`	`210`
`211`	`211`	// If `sample_rate` was specified, then it translates to a "catch-all" rule
`212`		- // appended to the end of `rules`. First, though, we have to make sure the
	`212`	+ // appended to the end of `rules`. First, though, we have to make sure the
`213`	`213`	`// sample rate is valid.`
`214`	`214`	`if (sample_rate) {`
`215`	`215`	`auto maybe_rate = Rate::from(*sample_rate);`