From e6e4b8f0497aa6ecf90fc0bd5a1061266d9f7199 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Wed, 6 Aug 2025 12:30:15 +0200 Subject: [PATCH] fix(rc): improve trace sampling rules parsing 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] --- src/datadog/config_manager.cpp | 210 ++++++++++++++++++++------ src/datadog/json_serializer.h | 20 ++- src/datadog/trace_sampler_config.cpp | 2 +- test/system-tests/request_handler.cpp | 20 +++ test/test_config_manager.cpp | 37 +++++ 5 files changed, 227 insertions(+), 62 deletions(-) diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index 36b054e5..6b6462f8 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -2,7 +2,6 @@ #include -#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; +using Tags = std::unordered_map; + +Expected 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 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 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()); + 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()); - 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 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 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 index d1e3adc8..014dd680 100644 --- 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 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 { - type = value.type_name(); + [&json](StringView property, const nlohmann::json& value, + StringView expected_type) -> Optional { + const StringView type = value.type_name(); if (type == expected_type) { return nullopt; } @@ -41,7 +40,7 @@ inline Expected 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 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 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 index 9981cd12..b29d799f 100644 --- a/src/datadog/trace_sampler_config.cpp +++ b/src/datadog/trace_sampler_config.cpp @@ -209,7 +209,7 @@ Expected 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 index 7e9b4dbc..95e5e3cb 100644 --- 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 index c3e6bbb5..4443cb64 100644 --- 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); + } + } }