Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dmehala merged 1 commit into from
Sep 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 160 additions & 50 deletions src/datadog/config_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto make_error = [&json_rule](StringView field_name) {
auto make_missing_error = [&json_rule](StringView field_name) {

nit: as we specify for property it might be easier to read to specify here as well

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;
Expand Down Expand Up @@ -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;
Comment thread
zacharycmontoya marked this conversation as resolved.
} else {
rules.emplace_back(std::move(rule));
}
}

metadata.emplace_back(std::move(trace_sampling_metadata));
}
Expand Down
20 changes: 9 additions & 11 deletions src/datadog/json_serializer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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 ";
Expand Down Expand Up @@ -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: ";
Expand All @@ -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.
}
Expand Down
2 changes: 1 addition & 1 deletion src/datadog/trace_sampler_config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 20 additions & 0 deletions test/system-tests/request_handler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down
37 changes: 37 additions & 0 deletions test/test_config_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines +215 to +219
Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check that the right values have been set as well ? Currently if an error happened, it might tell that old_sampler_cfg != new_sampler_cfg but maybe the values are not the right ones

Copy link
Copy Markdown
Collaborator Author

@dmehala dmehala Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your question related to the trace sampler values? If so, config_json() returns a JSON with said values, if there's a change, then the JSON from old/new_sampler_cfg will differ.

Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not understand everything clearly but from my understanding it is not because there was a change that the right values were set

Copy link
Copy Markdown
Collaborator Author

@dmehala dmehala Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from your take is that, it would be preferable to check the rules in the JSON payload are equals to the rules in new_sampler_cfg? For example, we should make sure there's a rule for service foo, resource GET /hello and so on?

Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can check that there is indeed a rule for the resource get /hello with the right value/tags, it ensures the correctness of the apply. That may be what you are doing in #234 though

Copy link
Copy Markdown
Collaborator Author

@dmehala dmehala Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Currently we are not validating the correctness because the TraceSampler interface doesn't provide a way to retrieve the rules, and I don't think there's any meaning except for testing purposes. I am planning to revamp the configuration part, one of the requirement is to be able to validate the config sent to telemetry. This would help to validate the correctness.

Copy link
Copy Markdown
Contributor

@dubloom dubloom Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, perfect. Ping me if you need an additional hand.


CHECK(old_sampler_cfg != new_sampler_cfg);
}
}
}