From 9ea776a4df7441bc6be1e1635da91eb48918b6af Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Fri, 2 Feb 2024 17:06:31 +0100 Subject: [PATCH 1/9] feat: report configuration to telemetry TODO: - remote config: merge applied config metadata --- src/datadog/config_manager.cpp | 143 ++++++++++++---- src/datadog/config_manager.h | 46 ++++- src/datadog/config_update.h | 4 +- src/datadog/datadog_agent.cpp | 27 ++- src/datadog/datadog_agent.h | 5 +- src/datadog/parse_util.cpp | 71 +++++++- src/datadog/parse_util.h | 16 ++ src/datadog/remote_config.cpp | 70 +++----- src/datadog/remote_config.h | 6 +- src/datadog/tracer.cpp | 2 +- src/datadog/tracer_config.cpp | 284 ++++++++++++++++++++----------- src/datadog/tracer_config.h | 77 ++++++++- src/datadog/tracer_telemetry.cpp | 68 +++++++- src/datadog/tracer_telemetry.h | 22 ++- test/test_smoke.cpp | 2 + test/test_trace_segment.cpp | 2 + test/test_tracer_telemetry.cpp | 4 +- 17 files changed, 632 insertions(+), 217 deletions(-) diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index 3789c34a..9807432c 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -1,80 +1,159 @@ #include "config_manager.h" +#include + +#include "parse_util.h" #include "trace_sampler.h" namespace datadog { namespace tracing { +namespace { + +std::string join(const std::vector& values, + const char* const separator) { + if (values.empty()) return {}; + auto it = values.cbegin(); + + std::string res{*it}; + for (++it; it != values.cend(); ++it) { + res += separator; + append(res, *it); + } + + return res; +} + +// TODO: use `to_chars` +std::string to_string(double d) { + std::stringstream stream; + stream << std::fixed << std::setprecision(1) << d; + return stream.str(); +} + +} // namespace ConfigManager::ConfigManager(const FinalizedTracerConfig& config) : clock_(config.clock), - default_trace_sampler_( + default_config_metadata_(config.metadata), + trace_sampler_( std::make_shared(config.trace_sampler, clock_)), - current_trace_sampler_(default_trace_sampler_), - default_span_defaults_(std::make_shared(config.defaults)), - current_span_defaults_(default_span_defaults_), - default_report_traces_(config.report_traces), - current_report_traces_(default_report_traces_) {} + span_defaults_(std::make_shared(config.defaults)), + report_traces_(config.report_traces) {} std::shared_ptr ConfigManager::trace_sampler() { std::lock_guard lock(mutex_); - return current_trace_sampler_; + return trace_sampler_; } std::shared_ptr ConfigManager::span_defaults() { std::lock_guard lock(mutex_); - return current_span_defaults_; + return span_defaults_; } bool ConfigManager::report_traces() { std::lock_guard lock(mutex_); - return current_report_traces_; + return report_traces_; } -void ConfigManager::update(const ConfigUpdate& conf) { +std::vector ConfigManager::update(const ConfigUpdate& conf) { + std::vector telemetry_conf; + std::lock_guard lock(mutex_); - if (conf.trace_sampler) { - if (auto finalized_trace_sampler_cfg = - finalize_config(*conf.trace_sampler)) { - current_trace_sampler_ = - std::make_shared(*finalized_trace_sampler_cfg, clock_); + if (conf.trace_sampling_rate) { + ConfigMetadata trace_sampling_metadata( + ConfigName::TRACE_SAMPLING_RATE, to_string(*conf.trace_sampling_rate), + ConfigMetadata::Origin::REMOTE_CONFIG); + + TraceSamplerConfig trace_sampler_cfg; + trace_sampler_cfg.sample_rate = *conf.trace_sampling_rate; + trace_sampler_cfg.max_per_second = 200; + + auto finalized_trace_sampler_cfg = finalize_config(trace_sampler_cfg); + if (auto error = finalized_trace_sampler_cfg.if_error()) { + trace_sampling_metadata.error = *error; } else { - // TODO: report error + trace_sampler_ = + std::make_shared(*finalized_trace_sampler_cfg, clock_); } + + telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); } else { - current_trace_sampler_ = default_trace_sampler_; + if (!trace_sampler_.is_default()) { + trace_sampler_.reset(); + telemetry_conf.emplace_back( + default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); + } } if (conf.tags) { - auto new_span_defaults = - std::make_shared(*current_span_defaults_); - new_span_defaults->tags = std::move(*conf.tags); + ConfigMetadata tags_metadata(ConfigName::TAGS, join(*conf.tags, ","), + ConfigMetadata::Origin::REMOTE_CONFIG); + + auto parsed_tags = parse_tags(*conf.tags); + if (auto error = parsed_tags.if_error()) { + tags_metadata.error = *error; + } else { + auto new_span_defaults = + std::make_shared(*span_defaults_.get()); + new_span_defaults->tags = std::move(*parsed_tags); + + span_defaults_ = new_span_defaults; + } - current_span_defaults_ = new_span_defaults; + telemetry_conf.emplace_back(std::move(tags_metadata)); } else { - current_span_defaults_ = default_span_defaults_; + if (!span_defaults_.is_default()) { + span_defaults_.reset(); + telemetry_conf.emplace_back(default_config_metadata_[ConfigName::TAGS]); + } } if (conf.report_traces) { - current_report_traces_ = *conf.report_traces; + ConfigMetadata tags_metadata(ConfigName::REPORT_TRACES, + *conf.report_traces ? "true" : "false", + ConfigMetadata::Origin::REMOTE_CONFIG); + report_traces_ = *conf.report_traces; } else { - current_report_traces_ = default_report_traces_; + if (!report_traces_.is_default()) { + report_traces_.reset(); + telemetry_conf.emplace_back( + default_config_metadata_[ConfigName::REPORT_TRACES]); + } } + + return telemetry_conf; } -void ConfigManager::reset() { +std::vector ConfigManager::reset() { + std::vector config_metadata; + std::lock_guard lock(mutex_); - current_trace_sampler_ = default_trace_sampler_; - current_span_defaults_ = default_span_defaults_; - current_report_traces_ = default_report_traces_; + if (!trace_sampler_.is_default()) { + trace_sampler_.reset(); + config_metadata.emplace_back( + default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); + } + + if (!span_defaults_.is_default()) { + span_defaults_.reset(); + config_metadata.emplace_back(default_config_metadata_[ConfigName::TAGS]); + } + + if (!report_traces_.is_default()) { + report_traces_.reset(); + config_metadata.emplace_back( + default_config_metadata_[ConfigName::REPORT_TRACES]); + } + + return config_metadata; } nlohmann::json ConfigManager::config_json() const { std::lock_guard lock(mutex_); - return nlohmann::json{ - {"default", to_json(*current_span_defaults_)}, - {"trace_sampler", current_trace_sampler_->config_json()}, - {"report_traces", current_report_traces_}}; + return nlohmann::json{{"default", to_json(*span_defaults_.get())}, + {"trace_sampler", trace_sampler_.get()->config_json()}, + {"report_traces", report_traces_.get()}}; } } // namespace tracing diff --git a/src/datadog/config_manager.h b/src/datadog/config_manager.h index 51b3c6b0..522b591d 100644 --- a/src/datadog/config_manager.h +++ b/src/datadog/config_manager.h @@ -17,16 +17,44 @@ namespace datadog { namespace tracing { class ConfigManager { + template + class DynamicConf { + T default_value_; + T current_value_; + bool is_default_value_; + + public: + explicit DynamicConf(T v) + : default_value_(v), current_value_(v), is_default_value_(true) {} + + void update(T new_v) { + current_value_ = new_v; + is_default_value_ = false; + } + + void reset() { + if (!is_default_value_) { + current_value_ = default_value_; + is_default_value_ = true; + } + } + + bool is_default() const { return is_default_value_; } + + T get() const { return current_value_; } + + operator T() const { return current_value_; } + + void operator=(const T& rhs) { update(rhs); } + }; + mutable std::mutex mutex_; Clock clock_; - std::shared_ptr default_trace_sampler_; - std::shared_ptr current_trace_sampler_; - - std::shared_ptr default_span_defaults_; - std::shared_ptr current_span_defaults_; + std::unordered_map default_config_metadata_; - bool default_report_traces_; - bool current_report_traces_; + DynamicConf> trace_sampler_; + DynamicConf> span_defaults_; + DynamicConf report_traces_; public: ConfigManager(const FinalizedTracerConfig& config); @@ -41,11 +69,11 @@ class ConfigManager { bool report_traces(); // Apply the specified `conf` update. - void update(const ConfigUpdate& conf); + std::vector update(const ConfigUpdate& conf); // Restore the configuration that was passed to this object's constructor, // overriding any previous calls to `update`. - void reset(); + std::vector reset(); // Return a JSON representation of the current configuration managed by this // object. diff --git a/src/datadog/config_update.h b/src/datadog/config_update.h index 20001e22..cd7e07f4 100644 --- a/src/datadog/config_update.h +++ b/src/datadog/config_update.h @@ -15,8 +15,8 @@ namespace tracing { // remote configuration value. struct ConfigUpdate { Optional report_traces; - Optional trace_sampler; - Optional> tags; + Optional trace_sampling_rate; + Optional> tags; }; } // namespace tracing diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 457101c2..3b6146fd 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -360,8 +360,9 @@ void DatadogAgent::flush() { } } -void DatadogAgent::send_app_started() { - auto payload = tracer_telemetry_->app_started(); +void DatadogAgent::send_app_started( + const std::unordered_map& config_metadata) { + auto payload = tracer_telemetry_->app_started(config_metadata); auto post_result = http_client_->post(telemetry_endpoint_, set_content_type_json, std::move(payload), telemetry_on_response_, @@ -396,6 +397,19 @@ void DatadogAgent::send_app_closing() { } } +void DatadogAgent::send_configuration_change( + const std::vector& config) { + auto payload = tracer_telemetry_->configuration_change(config); + auto post_result = + http_client_->post(telemetry_endpoint_, set_content_type_json, + std::move(payload), telemetry_on_response_, + telemetry_on_error_, clock_().tick + request_timeout_); + if (auto* error = post_result.if_error()) { + logger_->log_error(error->with_prefix( + "Unexpected error submitting telemetry configuration-change event: ")); + } +} + void DatadogAgent::get_and_apply_remote_configuration_updates() { auto remote_configuration_on_response = [this](int response_status, const DictReader& /*response_headers*/, @@ -433,10 +447,11 @@ void DatadogAgent::get_and_apply_remote_configuration_updates() { } if (!response_json.empty()) { - // TODO (during Active Configuration): `process_response` should - // return a list of configuration update and should be consumed by - // telemetry. - remote_config_.process_response(response_json); + auto updated_configuration = + remote_config_.process_response(response_json); + if (!updated_configuration.empty()) { + send_configuration_change(updated_configuration); + } } }; diff --git a/src/datadog/datadog_agent.h b/src/datadog/datadog_agent.h index 1b7f6787..497a372b 100644 --- a/src/datadog/datadog_agent.h +++ b/src/datadog/datadog_agent.h @@ -73,7 +73,10 @@ class DatadogAgent : public Collector { std::vector>&& spans, const std::shared_ptr& response_handler) override; - void send_app_started(); + void send_app_started( + const std::unordered_map& config_metadata); + + void send_configuration_change(const std::vector& config); void get_and_apply_remote_configuration_updates(); diff --git a/src/datadog/parse_util.cpp b/src/datadog/parse_util.cpp index d7ecd1bc..7632419f 100644 --- a/src/datadog/parse_util.cpp +++ b/src/datadog/parse_util.cpp @@ -47,9 +47,9 @@ StringView strip(StringView input) { const auto not_whitespace = [](unsigned char ch) { return !std::isspace(ch); }; - const char* const begin = + const char *const begin = std::find_if(input.begin(), input.end(), not_whitespace); - const char* const end = + const char *const end = std::find_if(input.rbegin(), std::make_reverse_iterator(begin), not_whitespace) .base(); @@ -107,10 +107,75 @@ bool starts_with(StringView subject, StringView prefix) { prefix.end(); } -void to_lower(std::string& text) { +void to_lower(std::string &text) { std::transform(text.begin(), text.end(), text.begin(), [](unsigned char ch) { return std::tolower(ch); }); } +// List items are separated by an optional comma (",") and any amount of +// whitespace. +// Leading and trailing whitespace is ignored. +std::vector parse_list(StringView input) { + using uchar = unsigned char; + + input = strip(input); + std::vector items; + if (input.empty()) { + return items; + } + + const char *const end = input.end(); + + const char *current = input.begin(); + const char *begin_delim; + do { + const char *begin_item = + std::find_if(current, end, [](uchar ch) { return !std::isspace(ch); }); + begin_delim = std::find_if(begin_item, end, [](uchar ch) { + return std::isspace(ch) || ch == ','; + }); + + items.emplace_back(begin_item, std::size_t(begin_delim - begin_item)); + + const char *end_delim = std::find_if( + begin_delim, end, [](uchar ch) { return !std::isspace(ch); }); + + if (end_delim != end && *end_delim == ',') { + ++end_delim; + } + + current = end_delim; + } while (begin_delim != end); + + return items; +} + +Expected> parse_tags( + std::vector list) { + std::unordered_map tags; + + for (const StringView &token : list) { + const auto separator = std::find(token.begin(), token.end(), ':'); + if (separator == token.end()) { + std::string message; + message += "Unable to parse a key/value from the tag text \""; + append(message, token); + // message += + // "\" because it does not contain the separator character \":\". " + // "Error occurred in list of tags \""; + // append(message, input); + message += "."; + return Error{Error::TAG_MISSING_SEPARATOR, std::move(message)}; + } + + std::string key{token.begin(), separator}; + std::string value{separator + 1, token.end()}; + // If there are duplicate values, then the last one wins. + tags.insert_or_assign(std::move(key), std::move(value)); + } + + return tags; +} + } // namespace tracing } // namespace datadog diff --git a/src/datadog/parse_util.h b/src/datadog/parse_util.h index 65233e87..c2104447 100644 --- a/src/datadog/parse_util.h +++ b/src/datadog/parse_util.h @@ -4,6 +4,8 @@ #include #include +#include +#include #include "expected.h" #include "string_view.h" @@ -39,5 +41,19 @@ bool starts_with(StringView subject, StringView prefix); // Convert the specified `text` to lower case in-place. void to_lower(std::string& text); +// List items are separated by an optional comma (",") and any amount of +// whitespace. +// Leading and trailing whitespace are ignored. +std::vector parse_list(StringView input); + +Expected> parse_tags( + std::vector list); + +inline Expected> parse_tags( + StringView input) { + // Within a tag, the key and value are separated by a colon (":"). + return parse_tags(parse_list(input)); +} + } // namespace tracing } // namespace datadog diff --git a/src/datadog/remote_config.cpp b/src/datadog/remote_config.cpp index 4afc5031..08ce8c6a 100644 --- a/src/datadog/remote_config.cpp +++ b/src/datadog/remote_config.cpp @@ -51,57 +51,22 @@ constexpr std::array k_apm_capabilities = constexpr StringView k_apm_product = "APM_TRACING"; constexpr StringView k_apm_product_path_substring = "/APM_TRACING/"; -Expected> parse_tags( - const std::vector& list_of_tags) { - std::unordered_map tags; - - // Within a tag, the key and value are separated by a colon (":"). - for (const StringView& token : list_of_tags) { - const auto separator = std::find(token.begin(), token.end(), ':'); - if (separator == token.end()) { - std::string message; - message += "Unable to parse a key/value from the tag text \""; - append(message, token); - message += - "\" because it does not contain the separator character \":\"."; - return Error{Error::TAG_MISSING_SEPARATOR, std::move(message)}; - } - - std::string key{token.begin(), separator}; - std::string value{separator + 1, token.end()}; - // If there are duplicate values, then the last one wins. - tags.insert_or_assign(std::move(key), std::move(value)); - } - - return tags; -} - ConfigUpdate parse_dynamic_config(const nlohmann::json& j) { ConfigUpdate config_update; if (auto sampling_rate_it = j.find("tracing_sampling_rate"); sampling_rate_it != j.cend()) { - TraceSamplerConfig trace_sampler_cfg; - trace_sampler_cfg.sample_rate = *sampling_rate_it; - - config_update.trace_sampler = trace_sampler_cfg; + config_update.trace_sampling_rate = *sampling_rate_it; } if (auto tags_it = j.find("tracing_tags"); tags_it != j.cend()) { - auto parsed_tags = parse_tags(*tags_it); - if (parsed_tags.if_error()) { - // TODO: report to telemetry - } else { - config_update.tags = std::move(*parsed_tags); - } + config_update.tags = *tags_it; } if (auto tracing_enabled_it = j.find("tracing_enabled"); tracing_enabled_it != j.cend()) { if (tracing_enabled_it->is_boolean()) { config_update.report_traces = tracing_enabled_it->get(); - } else { - // TODO: report to telemetry } } @@ -171,7 +136,11 @@ nlohmann::json RemoteConfigurationManager::make_request_payload() { return j; } -void RemoteConfigurationManager::process_response(const nlohmann::json& json) { +std::vector RemoteConfigurationManager::process_response( + const nlohmann::json& json) { + std::vector config_update; + config_update.reserve(8); + state_.error_message = nullopt; try { @@ -189,10 +158,12 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) { if (client_configs_it == json.cend()) { if (!applied_config_.empty()) { std::for_each(applied_config_.cbegin(), applied_config_.cend(), - [this](const auto it) { revert_config(it.second); }); + [this, &config_update](const auto it) { + config_update = revert_config(it.second); + }); applied_config_.clear(); } - return; + return config_update; } // Keep track of config path received to know which ones to revert. @@ -223,7 +194,7 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) { "target file having path \""; append(*state_.error_message, config_path); *state_.error_message += '\"'; - return; + return config_update; } const auto config_json = nlohmann::json::parse( @@ -243,14 +214,14 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) { new_config.version = config_json.at("revision"); new_config.content = parse_dynamic_config(config_json.at("lib_config")); - apply_config(new_config); + config_update = apply_config(new_config); applied_config_[std::string{config_path}] = new_config; } // Applied configuration not present must be reverted. for (auto it = applied_config_.cbegin(); it != applied_config_.cend();) { if (!visited_config.count(it->first)) { - revert_config(it->second); + config_update = revert_config(it->second); it = applied_config_.erase(it); } else { it++; @@ -261,15 +232,20 @@ void RemoteConfigurationManager::process_response(const nlohmann::json& json) { error_message += e.what(); state_.error_message = std::move(error_message); + return config_update; } + + return config_update; } -void RemoteConfigurationManager::apply_config(Configuration config) { - config_manager_->update(config.content); +std::vector RemoteConfigurationManager::apply_config( + Configuration config) { + return config_manager_->update(config.content); } -void RemoteConfigurationManager::revert_config(Configuration) { - config_manager_->reset(); +std::vector RemoteConfigurationManager::revert_config( + Configuration) { + return config_manager_->reset(); } } // namespace tracing diff --git a/src/datadog/remote_config.h b/src/datadog/remote_config.h index 86ccfc2d..6d8310e1 100644 --- a/src/datadog/remote_config.h +++ b/src/datadog/remote_config.h @@ -62,17 +62,17 @@ class RemoteConfigurationManager { // Handles the response received from a remote source and udates the internal // state accordingly. - void process_response(const nlohmann::json& json); + std::vector process_response(const nlohmann::json& json); private: // Tell if a `config_path` is a new configuration update. bool is_new_config(StringView config_path, const nlohmann::json& config_meta); // Apply a remote configuration. - void apply_config(Configuration config); + std::vector apply_config(Configuration config); // Revert a remote configuration. - void revert_config(Configuration config); + std::vector revert_config(Configuration config); }; } // namespace tracing diff --git a/src/datadog/tracer.cpp b/src/datadog/tracer.cpp index 6a35d0dc..583ecd74 100644 --- a/src/datadog/tracer.cpp +++ b/src/datadog/tracer.cpp @@ -67,7 +67,7 @@ Tracer::Tracer(const FinalizedTracerConfig& config, collector_ = agent; if (tracer_telemetry_->enabled()) { - agent->send_app_started(); + agent->send_app_started(config.metadata); } } diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp index 244bdd41..5dc0da7d 100644 --- a/src/datadog/tracer_config.cpp +++ b/src/datadog/tracer_config.cpp @@ -20,48 +20,86 @@ namespace datadog { namespace tracing { namespace { -bool falsy(StringView text) { - auto lower = std::string{text}; - to_lower(lower); - return lower == "0" || lower == "false" || lower == "no"; -} +std::string to_string(bool b) { return b ? "true" : "false"; } + +// TODO: use `to_chars` +// std::string to_string(double d) { +// std::stringstream stream; +// stream << std::fixed << std::setprecision(1) << d; +// return stream.str(); +// } +// +// std::string to_string(const std::vector &rules) { +// nlohmann::json res; +// for (const auto &r : rules) { +// res.emplace_back(r.to_json()); +// } +// +// return res.dump(); +// } +// +// std::string to_string(const std::vector &rules) { +// nlohmann::json res; +// for (const auto &r : rules) { +// res.emplace_back(r.to_json()); +// } +// +// return res.dump(); +// } + +std::string join_propagation_styles( + const std::vector &values) { + auto to_string = [](PropagationStyle style) { + switch (style) { + case PropagationStyle::B3: + return "B3"; + case PropagationStyle::DATADOG: + return "Datadog"; + case PropagationStyle::W3C: + return "W3C"; + case PropagationStyle::NONE: + return "None"; + } + return ""; ///< unlikely + }; -// List items are separated by an optional comma (",") and any amount of -// whitespace. -// Leading and trailing whitespace is ignored. -std::vector parse_list(StringView input) { - using uchar = unsigned char; + if (values.empty()) return {}; + auto it = values.cbegin(); - input = strip(input); - std::vector items; - if (input.empty()) { - return items; + std::string res{to_string(*it)}; + for (++it; it != values.cend(); ++it) { + res += ','; + res += to_string(*it); } - const char *const end = input.end(); + return res; +} - const char *current = input.begin(); - const char *begin_delim; - do { - const char *begin_item = - std::find_if(current, end, [](uchar ch) { return !std::isspace(ch); }); - begin_delim = std::find_if(begin_item, end, [](uchar ch) { - return std::isspace(ch) || ch == ','; - }); +std::string join_tags( + const std::unordered_map &values) { + if (values.empty()) return {}; - items.emplace_back(begin_item, std::size_t(begin_delim - begin_item)); + auto it = values.cbegin(); - const char *end_delim = std::find_if( - begin_delim, end, [](uchar ch) { return !std::isspace(ch); }); + std::string res; + res += it->first; + res += ":"; + res += it->second; - if (end_delim != end && *end_delim == ',') { - ++end_delim; - } + for (++it; it != values.cend(); ++it) { + res += ","; + res += it->first; + res += ":"; + res += it->second; + } - current = end_delim; - } while (begin_delim != end); + return res; +} - return items; +bool falsy(StringView text) { + auto lower = std::string{text}; + to_lower(lower); + return lower == "0" || lower == "false" || lower == "no"; } Expected> parse_propagation_styles( @@ -108,33 +146,6 @@ Expected> parse_propagation_styles( return styles; } -Expected> parse_tags( - StringView input) { - std::unordered_map tags; - - // Within a tag, the key and value are separated by a colon (":"). - for (const StringView &token : parse_list(input)) { - const auto separator = std::find(token.begin(), token.end(), ':'); - if (separator == token.end()) { - std::string message; - message += "Unable to parse a key/value from the tag text \""; - append(message, token); - message += - "\" because it does not contain the separator character \":\". " - "Error occurred in list of tags \""; - append(message, input); - message += "\"."; - return Error{Error::TAG_MISSING_SEPARATOR, std::move(message)}; - } - std::string key{token.begin(), separator}; - std::string value{separator + 1, token.end()}; - // If there are duplicate values, then the last one wins. - tags.insert_or_assign(std::move(key), std::move(value)); - } - - return tags; -} - // Return a `std::vector` parsed from the specified `env_var`. // If `env_var` is not in the environment, return `nullopt`. If an error occurs, // throw an `Error`. @@ -268,7 +279,7 @@ Expected load_tracer_env_config(Logger &logger) { if (!value_override) { continue; } - // TODO: log + const auto var_name = name(var); const auto var_name_override = name(var_override); @@ -307,6 +318,19 @@ Expected load_tracer_env_config(Logger &logger) { return env_cfg; } +// TODO: Use SFINAE w/ is_trivially_constructible? +template +std::pair pick(const Optional &env, + const Optional &user, + U default_v) { + if (env) { + return {ConfigMetadata::Origin::ENVIRONMENT_VARIABLE, *env}; + } else if (user) { + return {ConfigMetadata::Origin::CODE, *user}; + } + return {ConfigMetadata::Origin::DEFAULT, default_v}; +} + } // namespace Expected finalize_config(const TracerConfig &config) { @@ -324,44 +348,113 @@ Expected finalize_config(const TracerConfig &user_config, } FinalizedTracerConfig final_config; - final_config.clock = clock; - final_config.logger = logger; - final_config.defaults.service = - value_or(env->service, user_config.service, ""); + + ConfigMetadata::Origin origin; + + std::tie(origin, final_config.defaults.service) = + pick(env->service, user_config.service, ""); + + if (final_config.defaults.service.empty()) { + return Error{Error::SERVICE_NAME_REQUIRED, "Service name is required."}; + } + + final_config.metadata[ConfigName::SERVICE_NAME] = ConfigMetadata( + ConfigName::SERVICE_NAME, final_config.defaults.service, origin); + final_config.defaults.service_type = value_or(env->service_type, user_config.service_type, "web"); - final_config.defaults.environment = - value_or(env->environment, user_config.environment, ""); - final_config.defaults.version = - value_or(env->version, user_config.version, ""); + + // DD_ENV + std::tie(origin, final_config.defaults.environment) = + pick(env->environment, user_config.environment, ""); + final_config.metadata[ConfigName::SERVICE_ENV] = ConfigMetadata( + ConfigName::SERVICE_ENV, final_config.defaults.environment, origin); + + // DD_VERSION + std::tie(origin, final_config.defaults.version) = + pick(env->version, user_config.version, ""); + final_config.metadata[ConfigName::SERVICE_VERSION] = ConfigMetadata( + ConfigName::SERVICE_VERSION, final_config.defaults.version, origin); + final_config.defaults.name = value_or(env->name, user_config.name, ""); - final_config.defaults.tags = - value_or(env->tags, user_config.tags, - std::unordered_map{}); - final_config.extraction_styles = - value_or(env->extraction_styles, user_config.extraction_styles, - std::vector{PropagationStyle::DATADOG, - PropagationStyle::W3C}); - final_config.injection_styles = - value_or(env->injection_styles, user_config.injection_styles, - std::vector{PropagationStyle::DATADOG, - PropagationStyle::W3C}); - final_config.log_on_startup = - value_or(env->log_on_startup, user_config.log_on_startup, true); - final_config.report_traces = - value_or(env->report_traces, user_config.report_traces, true); - final_config.report_telemetry = - value_or(env->report_telemetry, user_config.report_telemetry, true); + + // DD_TAGS + std::tie(origin, final_config.defaults.tags) = + pick(env->tags, user_config.tags, + std::unordered_map{}); + final_config.metadata[ConfigName::TAGS] = ConfigMetadata( + ConfigName::TAGS, join_tags(final_config.defaults.tags), origin); + + // Extraction Styles + const std::vector default_propagation_styles{ + PropagationStyle::DATADOG, PropagationStyle::W3C}; + + std::tie(origin, final_config.extraction_styles) = + pick(env->extraction_styles, user_config.extraction_styles, + default_propagation_styles); + if (final_config.extraction_styles.empty()) { + return Error{Error::MISSING_SPAN_EXTRACTION_STYLE, + "At least one extraction style must be specified."}; + } + final_config.metadata[ConfigName::EXTRACTION_STYLES] = ConfigMetadata( + ConfigName::EXTRACTION_STYLES, + join_propagation_styles(final_config.extraction_styles), origin); + + // Injection Styles + std::tie(origin, final_config.injection_styles) = + pick(env->injection_styles, user_config.injection_styles, + default_propagation_styles); + if (final_config.injection_styles.empty()) { + return Error{Error::MISSING_SPAN_INJECTION_STYLE, + "At least one injection style must be specified."}; + } + final_config.metadata[ConfigName::INJECTION_STYLES] = ConfigMetadata( + ConfigName::INJECTION_STYLES, + join_propagation_styles(final_config.injection_styles), origin); + + // Startup Logs + std::tie(origin, final_config.log_on_startup) = + pick(env->log_on_startup, user_config.log_on_startup, true); + final_config.metadata[ConfigName::STARTUP_LOGS] = ConfigMetadata( + ConfigName::STARTUP_LOGS, to_string(final_config.log_on_startup), origin); + + // Report traces + std::tie(origin, final_config.report_traces) = + pick(env->report_traces, user_config.report_traces, true); + final_config.metadata[ConfigName::REPORT_TRACES] = ConfigMetadata( + ConfigName::REPORT_TRACES, to_string(final_config.report_traces), origin); + + // Report telemetry + std::tie(origin, final_config.report_telemetry) = + pick(env->report_telemetry, user_config.report_telemetry, true); + final_config.metadata[ConfigName::REPORT_TELEMETRY] = + ConfigMetadata(ConfigName::REPORT_TELEMETRY, + to_string(final_config.report_traces), origin); + + // Report hostname final_config.report_hostname = value_or(env->report_hostname, user_config.report_hostname, false); - final_config.delegate_trace_sampling = value_or( + + // Delegate Sampling + std::tie(origin, final_config.delegate_trace_sampling) = pick( env->delegate_trace_sampling, user_config.delegate_trace_sampling, false); + final_config.metadata[ConfigName::DELEGATE_SAMPLING] = + ConfigMetadata(ConfigName::DELEGATE_SAMPLING, + to_string(final_config.delegate_trace_sampling), origin); + + // Tags Header Size final_config.tags_header_size = value_or( env->max_tags_header_size, user_config.max_tags_header_size, 512); - final_config.generate_128bit_trace_ids = - value_or(env->generate_128bit_trace_ids, - user_config.generate_128bit_trace_ids, true); + + // 128b Trace Ids + std::tie(origin, final_config.generate_128bit_trace_ids) = + pick(env->generate_128bit_trace_ids, + user_config.generate_128bit_trace_ids, true); + final_config.metadata[ConfigName::GENEREATE_128BIT_TRACE_IDS] = + ConfigMetadata(ConfigName::DELEGATE_SAMPLING, + to_string(final_config.delegate_trace_sampling), origin); + final_config.integration_name = value_or(env->integration_name, user_config.integration_name, ""); final_config.integration_version = @@ -371,19 +464,6 @@ Expected finalize_config(const TracerConfig &user_config, final_config.runtime_id = user_config.runtime_id; } - if (final_config.defaults.service.empty()) { - return Error{Error::SERVICE_NAME_REQUIRED, "Service name is required."}; - } - - if (final_config.extraction_styles.empty()) { - return Error{Error::MISSING_SPAN_EXTRACTION_STYLE, - "At least one extraction style must be specified."}; - } - if (final_config.injection_styles.empty()) { - return Error{Error::MISSING_SPAN_INJECTION_STYLE, - "At least one injection style must be specified."}; - } - if (!user_config.collector) { auto finalized = finalize_config(user_config.agent, final_config.logger, clock); diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 8c448361..5d6bbfa2 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -13,6 +13,7 @@ #include "datadog_agent_config.h" #include "error.h" #include "expected.h" +#include "json.hpp" #include "propagation_style.h" #include "runtime_id.h" #include "span_defaults.h" @@ -42,7 +43,7 @@ struct TracerConfig { // Example: `prod`, `pre-prod` or `staging`. Optional environment; - // Set the application version. + // Set the service version. // // Overriden by the `DD_VERSION` environment variable. // Example values: `1.2.3`, `6c44da20`, `2020.02.13`. @@ -164,6 +165,79 @@ struct TracerConfig { Optional report_service_as_default; }; +enum class ConfigName : char { + SERVICE_NAME, + SERVICE_ENV, + SERVICE_VERSION, + REPORT_TRACES, + TAGS, + TRACE_SAMPLING_RATE, + EXTRACTION_STYLES, + INJECTION_STYLES, + STARTUP_LOGS, + REPORT_TELEMETRY, + DELEGATE_SAMPLING, + GENEREATE_128BIT_TRACE_IDS, + AGENT_URL, + RC_POLL_INTERVAL, + TRACE_SAMPLING_LIMIT, + TRACE_SAMPLING_RULES, + SPAN_SAMPLING_RULES, +}; + +inline std::string to_string(ConfigName name) { + switch (name) { + case ConfigName::SERVICE_NAME: + return "DD_SERVICE"; + case ConfigName::SERVICE_ENV: + return "DD_ENV"; + case ConfigName::REPORT_TRACES: + return "trace_enabled"; + case ConfigName::TAGS: + return "trace_tags"; + case ConfigName::TRACE_SAMPLING_RATE: + return "trace_sample_rate"; + case ConfigName::SERVICE_VERSION: + return "DD_VERSION"; + case ConfigName::EXTRACTION_STYLES: + return "DD_TRACE_PROPAGATION_STYLE_EXTRACT"; + case ConfigName::INJECTION_STYLES: + return "DD_TRACE_PROPAGATION_STYLE_INJECT"; + case ConfigName::STARTUP_LOGS: + return "DD_TRACE_STARTUP_LOGS"; + case ConfigName::REPORT_TELEMETRY: + return "DD_INSTRUMENTATION_TELEMETRY_ENABLED"; + case ConfigName::DELEGATE_SAMPLING: + return "DD_TRACE_DELEGATE_SAMPLING"; + case ConfigName::GENEREATE_128BIT_TRACE_IDS: + return "DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED"; + case ConfigName::AGENT_URL: + return "DD_TRACE_AGENT_URL"; + case ConfigName::RC_POLL_INTERVAL: + return "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"; + case ConfigName::TRACE_SAMPLING_LIMIT: + return "DD_TRACE_RATE_LIMIT"; + case ConfigName::SPAN_SAMPLING_RULES: + return "DD_SPAN_SAMPLING_RULES"; + case ConfigName::TRACE_SAMPLING_RULES: + return "DD_TRACE_SAMPLING_RULES"; + } + + std::abort(); +} + +struct ConfigMetadata { + enum class Origin { ENVIRONMENT_VARIABLE, CODE, REMOTE_CONFIG, DEFAULT }; + ConfigName name; + std::string value; + Origin origin; + Optional error; + + ConfigMetadata() = default; + ConfigMetadata(ConfigName n, std::string v, Origin orig) + : name(n), value(std::move(v)), origin(orig), error(nullopt) {} +}; + // `FinalizedTracerConfig` contains `Tracer` implementation details derived from // a valid `TracerConfig` and accompanying environment. // `FinalizedTracerConfig` must be obtained by calling `finalize_config`. @@ -197,6 +271,7 @@ class FinalizedTracerConfig final { std::string integration_version; bool delegate_trace_sampling; bool report_traces; + std::unordered_map metadata; }; // Return a `FinalizedTracerConfig` from the specified `config` and from any diff --git a/src/datadog/tracer_telemetry.cpp b/src/datadog/tracer_telemetry.cpp index 9ebda3c5..96bb5fbd 100644 --- a/src/datadog/tracer_telemetry.cpp +++ b/src/datadog/tracer_telemetry.cpp @@ -83,12 +83,59 @@ nlohmann::json TracerTelemetry::generate_telemetry_body( }); } -std::string TracerTelemetry::app_started() { +nlohmann::json TracerTelemetry::generate_configuration_field( + const ConfigMetadata& config_metadata) { + // NOTE(@dmehala): `seq_id` should start at 1 so that the go backend can + // detect between non set fields. + config_seq_ids[config_metadata.name] += 1; + auto seq_id = config_seq_ids[config_metadata.name]; + + auto j = nlohmann::json{{"name", to_string(config_metadata.name)}, + {"value", config_metadata.value}, + {"seq_id", seq_id}}; + + switch (config_metadata.origin) { + case ConfigMetadata::Origin::ENVIRONMENT_VARIABLE: + j["origin"] = "env_var"; + break; + case ConfigMetadata::Origin::CODE: + j["origin"] = "code"; + break; + case ConfigMetadata::Origin::REMOTE_CONFIG: + j["origin"] = "remote_config"; + break; + case ConfigMetadata::Origin::DEFAULT: + j["origin"] = "default"; + break; + } + + if (config_metadata.error) { + // clang-format off + j["error"] = { + {"code", config_metadata.error->code}, + {"message", config_metadata.error->message} + }; + // clang-format on + } + + return j; +} + +std::string TracerTelemetry::app_started( + const std::unordered_map& configurations) { + auto configuration_json = nlohmann::json::array(); + for (const auto& [_, config_metadata] : configurations) { + // if (config_metadata.value.empty()) continue; + + configuration_json.emplace_back( + generate_configuration_field(config_metadata)); + } + // clang-format off auto app_started_msg = nlohmann::json{ {"request_type", "app-started"}, {"payload", nlohmann::json{ - {"configuration", nlohmann::json::array()} + {"configuration", configuration_json} }} }; @@ -241,5 +288,22 @@ std::string TracerTelemetry::app_closing() { return message_batch_payload; } +std::string TracerTelemetry::configuration_change( + const std::vector& new_configuration) { + auto configuration_json = nlohmann::json::array(); + for (const auto& config_metadata : new_configuration) { + // if (config_metadata.value.empty()) continue; + configuration_json.emplace_back( + generate_configuration_field(config_metadata)); + } + + auto configuration_change = + generate_telemetry_body("app-client-configuration-change"); + configuration_change["payload"] = + nlohmann::json{{"configuration", configuration_json}}; + + return configuration_change.dump(); +} + } // namespace tracing } // namespace datadog diff --git a/src/datadog/tracer_telemetry.h b/src/datadog/tracer_telemetry.h index f29231a6..042ac33c 100644 --- a/src/datadog/tracer_telemetry.h +++ b/src/datadog/tracer_telemetry.h @@ -31,6 +31,7 @@ #include "json.hpp" #include "metrics.h" #include "runtime_id.h" +#include "tracer_config.h" #include "tracer_signature.h" namespace datadog { @@ -49,6 +50,8 @@ class TracerTelemetry { std::string integration_name_; std::string integration_version_; uint64_t seq_id_ = 0; + // Track sequence id per configuration field. + std::unordered_map config_seq_ids; // This structure contains all the metrics that are exposed by tracer // telemetry. struct { @@ -99,6 +102,9 @@ class TracerTelemetry { nlohmann::json generate_telemetry_body(std::string request_type); + nlohmann::json generate_configuration_field( + const ConfigMetadata& config_metadata); + public: TracerTelemetry(bool enabled, const Clock& clock, const std::shared_ptr& logger, @@ -111,17 +117,21 @@ class TracerTelemetry { auto& metrics() { return metrics_; }; // Constructs an `app-started` message using information provided when // constructed and the tracer_config value passed in. - std::string app_started(); - // This is used to take a snapshot of the current state of metrics and collect - // timestamped "points" of values. These values are later submitted in - // `generate-metrics` messages. + std::string app_started( + const std::unordered_map& configurations); + // This is used to take a snapshot of the current state of metrics and + // collect timestamped "points" of values. These values are later submitted + // in `generate-metrics` messages. void capture_metrics(); - // Constructs a messsage-batch containing `app-heartbeat`, and if metrics have - // been modified, a `generate-metrics` message. + // Constructs a messsage-batch containing `app-heartbeat`, and if metrics + // have been modified, a `generate-metrics` message. std::string heartbeat_and_telemetry(); // Constructs a message-batch containing `app-closing`, and if metrics have // been modified, a `generate-metrics` message. std::string app_closing(); + // Construct an `app-client-configuration-change` message. + std::string configuration_change( + const std::vector& new_configuration); }; } // namespace tracing diff --git a/test/test_smoke.cpp b/test/test_smoke.cpp index c71fb7bf..924a9312 100644 --- a/test/test_smoke.cpp +++ b/test/test_smoke.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -12,6 +13,7 @@ TEST_CASE("smoke") { TracerConfig config; config.service = "testsvc"; config.logger = std::make_shared(); + config.collector = std::make_shared(); auto maybe_config = finalize_config(config); REQUIRE(maybe_config); diff --git a/test/test_trace_segment.cpp b/test/test_trace_segment.cpp index 50c949da..eddaffd9 100644 --- a/test/test_trace_segment.cpp +++ b/test/test_trace_segment.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -467,6 +468,7 @@ TEST_CASE("independent of Tracer") { TracerConfig config; config.service = "testsvc"; config.name = "do.thing"; + config.collector = std::make_shared(); config.logger = std::make_shared(); auto maybe_tracer = finalize_config(config); diff --git a/test/test_tracer_telemetry.cpp b/test/test_tracer_telemetry.cpp index 5fe1b144..ab80d95e 100644 --- a/test/test_tracer_telemetry.cpp +++ b/test/test_tracer_telemetry.cpp @@ -35,7 +35,7 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { SECTION("generates app-started message") { SECTION("Without a defined integration") { - auto app_started_message = tracer_telemetry.app_started(); + auto app_started_message = tracer_telemetry.app_started({}); auto app_started = nlohmann::json::parse(app_started_message); REQUIRE(app_started["request_type"] == "message-batch"); REQUIRE(app_started["payload"].size() == 1); @@ -45,7 +45,7 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { SECTION("With an integration") { TracerTelemetry tracer_telemetry{ true, clock, logger, tracer_signature, "nginx", "1.25.2"}; - auto app_started_message = tracer_telemetry.app_started(); + auto app_started_message = tracer_telemetry.app_started({}); auto app_started = nlohmann::json::parse(app_started_message); REQUIRE(app_started["request_type"] == "message-batch"); REQUIRE(app_started["payload"].size() == 2); From 09482ca11a22d9e29ed26ad9141174121bbdfda2 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Mon, 12 Feb 2024 14:49:22 +0100 Subject: [PATCH 2/9] add tests --- src/datadog/config_manager.cpp | 28 ++++++----- src/datadog/config_manager.h | 9 ++-- src/datadog/datadog_agent.cpp | 43 +++++----------- src/datadog/datadog_agent.h | 1 + src/datadog/tracer_config.h | 5 +- test/test_datadog_agent.cpp | 65 ++++++++++++++++++++++++- test/test_remote_config.cpp | 22 ++++++--- test/test_tracer_telemetry.cpp | 89 +++++++++++++++++++++++++++++++++- 8 files changed, 203 insertions(+), 59 deletions(-) diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index 9807432c..b59a0917 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -72,12 +72,15 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { auto finalized_trace_sampler_cfg = finalize_config(trace_sampler_cfg); if (auto error = finalized_trace_sampler_cfg.if_error()) { trace_sampling_metadata.error = *error; - } else { - trace_sampler_ = - std::make_shared(*finalized_trace_sampler_cfg, clock_); } - telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); + auto trace_sampler = + std::make_shared(*finalized_trace_sampler_cfg, clock_); + + if (trace_sampler != trace_sampler_.get()) { + trace_sampler_ = std::move(trace_sampler); + telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); + } } else { if (!trace_sampler_.is_default()) { trace_sampler_.reset(); @@ -93,15 +96,16 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { auto parsed_tags = parse_tags(*conf.tags); if (auto error = parsed_tags.if_error()) { tags_metadata.error = *error; - } else { + } + + if (*parsed_tags != span_defaults_.get()->tags) { auto new_span_defaults = std::make_shared(*span_defaults_.get()); new_span_defaults->tags = std::move(*parsed_tags); span_defaults_ = new_span_defaults; + telemetry_conf.emplace_back(std::move(tags_metadata)); } - - telemetry_conf.emplace_back(std::move(tags_metadata)); } else { if (!span_defaults_.is_default()) { span_defaults_.reset(); @@ -110,10 +114,12 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { } if (conf.report_traces) { - ConfigMetadata tags_metadata(ConfigName::REPORT_TRACES, - *conf.report_traces ? "true" : "false", - ConfigMetadata::Origin::REMOTE_CONFIG); - report_traces_ = *conf.report_traces; + if (conf.report_traces != report_traces_) { + report_traces_ = *conf.report_traces; + telemetry_conf.emplace_back(ConfigName::REPORT_TRACES, + *conf.report_traces ? "true" : "false", + ConfigMetadata::Origin::REMOTE_CONFIG); + } } else { if (!report_traces_.is_default()) { report_traces_.reset(); diff --git a/src/datadog/config_manager.h b/src/datadog/config_manager.h index 522b591d..b2925591 100644 --- a/src/datadog/config_manager.h +++ b/src/datadog/config_manager.h @@ -33,15 +33,14 @@ class ConfigManager { } void reset() { - if (!is_default_value_) { - current_value_ = default_value_; - is_default_value_ = true; - } + current_value_ = default_value_; + is_default_value_ = true; } bool is_default() const { return is_default_value_; } - T get() const { return current_value_; } + T get() { return current_value_; } + const T& get() const { return current_value_; } operator T() const { return current_value_; } diff --git a/src/datadog/datadog_agent.cpp b/src/datadog/datadog_agent.cpp index 3b6146fd..484a2b81 100644 --- a/src/datadog/datadog_agent.cpp +++ b/src/datadog/datadog_agent.cpp @@ -360,54 +360,33 @@ void DatadogAgent::flush() { } } -void DatadogAgent::send_app_started( - const std::unordered_map& config_metadata) { - auto payload = tracer_telemetry_->app_started(config_metadata); +void DatadogAgent::send_telemetry(std::string payload) { auto post_result = http_client_->post(telemetry_endpoint_, set_content_type_json, std::move(payload), telemetry_on_response_, telemetry_on_error_, clock_().tick + request_timeout_); if (auto* error = post_result.if_error()) { - logger_->log_error(error->with_prefix( - "Unexpected error submitting telemetry app-started event: ")); + logger_->log_error( + error->with_prefix("Unexpected error submitting telemetry event: ")); } } +void DatadogAgent::send_app_started( + const std::unordered_map& config_metadata) { + send_telemetry(tracer_telemetry_->app_started(config_metadata)); +} + void DatadogAgent::send_heartbeat_and_telemetry() { - auto payload = tracer_telemetry_->heartbeat_and_telemetry(); - auto post_result = - http_client_->post(telemetry_endpoint_, set_content_type_json, - std::move(payload), telemetry_on_response_, - telemetry_on_error_, clock_().tick + request_timeout_); - if (auto* error = post_result.if_error()) { - logger_->log_error(error->with_prefix( - "Unexpected error submitting telemetry app-heartbeat event: ")); - } + send_telemetry(tracer_telemetry_->heartbeat_and_telemetry()); } void DatadogAgent::send_app_closing() { - auto payload = tracer_telemetry_->app_closing(); - auto post_result = - http_client_->post(telemetry_endpoint_, set_content_type_json, - std::move(payload), telemetry_on_response_, - telemetry_on_error_, clock_().tick + request_timeout_); - if (auto* error = post_result.if_error()) { - logger_->log_error(error->with_prefix( - "Unexpected error submitting telemetry app-closing event: ")); - } + send_telemetry(tracer_telemetry_->app_closing()); } void DatadogAgent::send_configuration_change( const std::vector& config) { - auto payload = tracer_telemetry_->configuration_change(config); - auto post_result = - http_client_->post(telemetry_endpoint_, set_content_type_json, - std::move(payload), telemetry_on_response_, - telemetry_on_error_, clock_().tick + request_timeout_); - if (auto* error = post_result.if_error()) { - logger_->log_error(error->with_prefix( - "Unexpected error submitting telemetry configuration-change event: ")); - } + send_telemetry(tracer_telemetry_->configuration_change(config)); } void DatadogAgent::get_and_apply_remote_configuration_updates() { diff --git a/src/datadog/datadog_agent.h b/src/datadog/datadog_agent.h index 497a372b..2a23aaff 100644 --- a/src/datadog/datadog_agent.h +++ b/src/datadog/datadog_agent.h @@ -59,6 +59,7 @@ class DatadogAgent : public Collector { RemoteConfigurationManager remote_config_; void flush(); + void send_telemetry(std::string); void send_heartbeat_and_telemetry(); void send_app_closing(); diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 5d6bbfa2..9df500b3 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -234,8 +234,9 @@ struct ConfigMetadata { Optional error; ConfigMetadata() = default; - ConfigMetadata(ConfigName n, std::string v, Origin orig) - : name(n), value(std::move(v)), origin(orig), error(nullopt) {} + ConfigMetadata(ConfigName n, std::string v, Origin orig, + Optional err = nullopt) + : name(n), value(std::move(v)), origin(orig), error(err) {} }; // `FinalizedTracerConfig` contains `Tracer` implementation details derived from diff --git a/test/test_datadog_agent.cpp b/test/test_datadog_agent.cpp index d45bae12..cc97acb3 100644 --- a/test/test_datadog_agent.cpp +++ b/test/test_datadog_agent.cpp @@ -1,8 +1,10 @@ #include +#include #include #include #include +#include #include #include "mocks/event_schedulers.h" @@ -11,8 +13,9 @@ #include "test.h" using namespace datadog::tracing; +using namespace std::chrono_literals; -TEST_CASE("CollectorResponse") { +TEST_CASE("CollectorResponse", "[datadog_agent]") { TracerConfig config; config.service = "testsvc"; const auto logger = @@ -166,3 +169,63 @@ TEST_CASE("CollectorResponse") { REQUIRE(logger->first_error().code == error.code); } } + +// NOTE: `report_telemetry` is too vague for now. +// Does it mean no telemetry at all or just metrics are not generated? +// +// TODO: Use cases to implement: +// - telemetry is disabled, no event scheduled. +// - telemetry is enabled, after x sec generate metrics is called. +// - send_app_started? +TEST_CASE("Remote Configuration", "[datadog_agent]") { + const auto logger = + std::make_shared(std::cerr, MockLogger::ERRORS_ONLY); + logger->echo = nullptr; + const auto event_scheduler = std::make_shared(); + const auto http_client = std::make_shared(); + + TracerConfig config; + config.service = "testsvc"; + config.logger = logger; + config.agent.event_scheduler = event_scheduler; + config.agent.http_client = http_client; + config.report_telemetry = false; + + auto finalized = finalize_config(config); + REQUIRE(finalized); + + const TracerSignature signature(RuntimeID::generate(), "testsvc", "test"); + auto config_manager = std::make_shared(*finalized); + + auto telemetry = std::make_shared( + finalized->report_telemetry, finalized->clock, finalized->logger, + signature, "", ""); + + const auto& agent_config = + std::get(finalized->collector); + DatadogAgent agent(agent_config, telemetry, config.logger, signature, + config_manager); + + SECTION("404 do not log an error") { + http_client->response_status = 404; + agent.get_and_apply_remote_configuration_updates(); + http_client->drain(std::chrono::steady_clock::now()); + CHECK(logger->error_count() == 0); + } + + SECTION("5xx log an error") { + http_client->response_status = 500; + agent.get_and_apply_remote_configuration_updates(); + http_client->drain(std::chrono::steady_clock::now()); + CHECK(logger->error_count() == 1); + } + + SECTION("non json input") { + http_client->response_status = 200; + http_client->response_body << "hello, mars!"; + + agent.get_and_apply_remote_configuration_updates(); + http_client->drain(std::chrono::steady_clock::now()); + CHECK(logger->error_count() == 1); + } +} diff --git a/test/test_remote_config.cpp b/test/test_remote_config.cpp index 1a8fde74..8b55d1cf 100644 --- a/test/test_remote_config.cpp +++ b/test/test_remote_config.cpp @@ -151,7 +151,8 @@ REMOTE_CONFIG_TEST("response processing") { /* allow_exceptions = */ false); REQUIRE(!response_json.is_discarded()); - rc.process_response(response_json); + const auto config_updated = rc.process_response(response_json); + CHECK(config_updated.empty()); // Next payload should contains an error. const auto payload = rc.make_request_payload(); @@ -201,7 +202,8 @@ REMOTE_CONFIG_TEST("response processing") { const auto old_trace_sampler = config_manager->trace_sampler(); const auto old_span_defaults = config_manager->span_defaults(); const auto old_report_traces = config_manager->report_traces(); - rc.process_response(response_json); + const auto config_updated = rc.process_response(response_json); + REQUIRE(config_updated.size() == 3); const auto new_trace_sampler = config_manager->trace_sampler(); const auto new_span_defaults = config_manager->span_defaults(); const auto new_report_traces = config_manager->report_traces(); @@ -227,7 +229,9 @@ REMOTE_CONFIG_TEST("response processing") { REQUIRE(!response_json.is_discarded()); - rc.process_response(response_json); + const auto config_updated = rc.process_response(response_json); + REQUIRE(config_updated.size() == 3); + const auto current_trace_sampler = config_manager->trace_sampler(); const auto current_span_defaults = config_manager->span_defaults(); const auto current_report_traces = config_manager->report_traces(); @@ -237,7 +241,9 @@ REMOTE_CONFIG_TEST("response processing") { CHECK(old_report_traces == current_report_traces); } - SECTION("missing configuration field -> field should be reset") { + SECTION( + "missing the trace_sampling_rate field -> only this field should be " + "reset") { // clang-format off const std::string json_input = R"({ "targets": "ewogICAgInNpZ25lZCI6IHsKICAgICAgICAiY3VzdG9tIjogewogICAgICAgICAgICAiYWdlbnRfcmVmcmVzaF9pbnRlcnZhbCI6IDUsCiAgICAgICAgICAgICJvcGFxdWVfYmFja2VuZF9zdGF0ZSI6ICJleUoyWlhKemFXOXVJam95TENKemRHRjBaU0k2ZXlKbWFXeGxYMmhoYzJobGN5STZleUprWVhSaFpHOW5MekV3TURBeE1qVTROREF2UVZCTlgxUlNRVU5KVGtjdk9ESTNaV0ZqWmpoa1ltTXpZV0l4TkRNMFpETXlNV05pT0RGa1ptSm1OMkZtWlRZMU5HRTBZall4TVRGalpqRTJOakJpTnpGalkyWTRPVGM0TVRrek9DOHlPVEE0Tm1Ka1ltVTFNRFpsTmpoaU5UQm1NekExTlRneU0yRXpaR0UxWTJVd05USTRaakUyTkRCa05USmpaamc0TmpFNE1UWmhZV0U1Wm1ObFlXWTBJanBiSW05WVpESnBlVU16ZUM5b1JXc3hlWFZoWTFoR04xbHFjWEpwVGs5QldVdHVaekZ0V0UwMU5WWktUSGM5SWwxOWZYMD0iCiAgICAgICAgfSwKICAgICAgICAic3BlY192ZXJzaW9uIjogIjEuMC4wIiwKICAgICAgICAidGFyZ2V0cyI6IHsKICAgICAgICAgICAgImZvby9BUE1fVFJBQ0lORy8zMCI6IHsKICAgICAgICAgICAgICAgICJoYXNoZXMiOiB7CiAgICAgICAgICAgICAgICAgICAgInNoYTI1NiI6ICI2OWUzNDZiNWZmY2U4NDVlMjk5ODRlNzU5YjcxZDdiMDdjNTYxOTc5ZmFlOWU4MmVlZDA4MmMwMzhkODZlNmIwIgogICAgICAgICAgICAgICAgfSwKICAgICAgICAgICAgICAgICJsZW5ndGgiOiAzNzQKICAgICAgICAgICAgfQogICAgICAgIH0sCiAgICAgICAgInZlcnNpb24iOiA2NjIwNDMyMAogICAgfQp9", @@ -245,7 +251,7 @@ REMOTE_CONFIG_TEST("response processing") { "target_files": [ { "path": "foo/APM_TRACING/30", - "raw": "eyAiaWQiOiAiODI3ZWFjZjhkYmMzYWIxNDM0ZDMyMWNiODFkZmJmN2FmZTY1NGE0YjYxMTFjZjE2NjBiNzFjY2Y4OTc4MTkzOCIsICJyZXZpc2lvbiI6IDE2OTgxNjcxMjYwNjQsICJzY2hlbWFfdmVyc2lvbiI6ICJ2MS4wLjAiLCAiYWN0aW9uIjogImVuYWJsZSIsICJsaWJfY29uZmlnIjogeyAibGlicmFyeV9sYW5ndWFnZSI6ICJhbGwiLCAibGlicmFyeV92ZXJzaW9uIjogImxhdGVzdCIsICJzZXJ2aWNlX25hbWUiOiAidGVzdHN2YyIsICJlbnYiOiAidGVzdCIsICJ0cmFjaW5nX2VuYWJsZWQiOiB0cnVlIH0sICJzZXJ2aWNlX3RhcmdldCI6IHsgInNlcnZpY2UiOiAidGVzdHN2YyIsICJlbnYiOiAidGVzdCIgfSB9" + "raw": "eyAiaWQiOiAiODI3ZWFjZjhkYmMzYWIxNDM0ZDMyMWNiODFkZmJmN2FmZTY1NGE0YjYxMTFjZjE2NjBiNzFjY2Y4OTc4MTkzOCIsICJyZXZpc2lvbiI6IDE2OTgxNjcxMjYwNjQsICJzY2hlbWFfdmVyc2lvbiI6ICJ2MS4wLjAiLCAiYWN0aW9uIjogImVuYWJsZSIsICJsaWJfY29uZmlnIjogeyAibGlicmFyeV9sYW5ndWFnZSI6ICJhbGwiLCAibGlicmFyeV92ZXJzaW9uIjogImxhdGVzdCIsICJzZXJ2aWNlX25hbWUiOiAidGVzdHN2YyIsICJlbnYiOiAidGVzdCIsICJ0cmFjaW5nX2VuYWJsZWQiOiBmYWxzZSwgInRyYWNpbmdfdGFncyI6IFsiaGVsbG86d29ybGQiLCAiZm9vOmJhciJdIH0sICJzZXJ2aWNlX3RhcmdldCI6IHsgInNlcnZpY2UiOiAidGVzdHN2YyIsICJlbnYiOiAidGVzdCIgfSB9" } ] })"; @@ -258,7 +264,8 @@ REMOTE_CONFIG_TEST("response processing") { REQUIRE(!response_json.is_discarded()); - rc.process_response(response_json); + const auto config_updated = rc.process_response(response_json); + REQUIRE(config_updated.size() == 1); const auto current_trace_sampler = config_manager->trace_sampler(); CHECK(old_trace_sampler == current_trace_sampler); } @@ -303,9 +310,10 @@ REMOTE_CONFIG_TEST("response processing") { REQUIRE(!response_json.is_discarded()); const auto old_sampling_rate = config_manager->trace_sampler(); - rc.process_response(response_json); + const auto config_updated = rc.process_response(response_json); const auto new_sampling_rate = config_manager->trace_sampler(); + CHECK(config_updated.empty()); CHECK(new_sampling_rate == old_sampling_rate); } } diff --git a/test/test_tracer_telemetry.cpp b/test/test_tracer_telemetry.cpp index ab80d95e..95c359ba 100644 --- a/test/test_tracer_telemetry.cpp +++ b/test/test_tracer_telemetry.cpp @@ -39,7 +39,10 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { auto app_started = nlohmann::json::parse(app_started_message); REQUIRE(app_started["request_type"] == "message-batch"); REQUIRE(app_started["payload"].size() == 1); - CHECK(app_started["payload"][0]["request_type"] == "app-started"); + + auto& app_started_payload = app_started["payload"][0]; + CHECK(app_started_payload["request_type"] == "app-started"); + CHECK(app_started_payload["payload"]["configuration"].empty()); } SECTION("With an integration") { @@ -57,6 +60,90 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { CHECK(expected.find(payload["request_type"]) != expected.cend()); } } + + SECTION("With configuration") { + std::unordered_map configuration{ + {ConfigName::SERVICE_NAME, + ConfigMetadata(ConfigName::SERVICE_NAME, "foo", + ConfigMetadata::Origin::CODE)}}; + + auto app_started_message = tracer_telemetry.app_started(configuration); + + auto app_started = nlohmann::json::parse(app_started_message); + REQUIRE(app_started["request_type"] == "message-batch"); + REQUIRE(app_started["payload"].is_array()); + REQUIRE(app_started["payload"].size() == 1); + + auto& app_started_payload = app_started["payload"][0]; + CHECK(app_started_payload["request_type"] == "app-started"); + + auto cfg_payload = app_started_payload["payload"]["configuration"]; + REQUIRE(cfg_payload.is_array()); + REQUIRE(cfg_payload.size() == 1); + + // clang-format off + const auto expected_conf = nlohmann::json({ + {"name", "DD_SERVICE"}, + {"value", "foo"}, + {"seq_id", 1}, + {"origin", "code"}, + }); + // clang-format on + + CHECK(cfg_payload[0] == expected_conf); + + SECTION("generates a configuration change event") { + SECTION("empty configuration generate a valid payload") { + auto config_change_message = nlohmann::json::parse( + tracer_telemetry.configuration_change({}), nullptr, false); + REQUIRE(config_change_message.is_discarded() == false); + + CHECK(config_change_message["request_type"] == + "app-client-configuration-change"); + CHECK(config_change_message["payload"]["configuration"].is_array()); + CHECK(config_change_message["payload"]["configuration"].empty()); + } + + SECTION("valid configurations update") { + const std::vector new_config{ + {ConfigName::SERVICE_NAME, "increase seq_id", + ConfigMetadata::Origin::ENVIRONMENT_VARIABLE}, + {ConfigName::REPORT_TRACES, "", ConfigMetadata::Origin::DEFAULT, + Error{Error::Code::OTHER, "empty field"}}}; + + auto config_change_message = nlohmann::json::parse( + tracer_telemetry.configuration_change(new_config), nullptr, + false); + REQUIRE(config_change_message.is_discarded() == false); + + CHECK(config_change_message["request_type"] == + "app-client-configuration-change"); + CHECK(config_change_message["payload"]["configuration"].is_array()); + CHECK(config_change_message["payload"]["configuration"].size() == 2); + + const std::unordered_map expected_json{ + {"DD_SERVICE", nlohmann::json{{"name", "DD_SERVICE"}, + {"value", "increase seq_id"}, + {"seq_id", 2}, + {"origin", "env_var"}}}, + {"trace_enabled", + nlohmann::json{{"name", "trace_enabled"}, + {"value", ""}, + {"seq_id", 1}, + {"origin", "default"}, + {"error", + {{"code", Error::Code::OTHER}, + {"message", "empty field"}}}}}}; + + for (const auto& conf : + config_change_message["payload"]["configuration"]) { + auto expected_conf = expected_json.find(conf["name"]); + REQUIRE(expected_conf != expected_json.cend()); + CHECK(expected_conf->second == conf); + } + } + } + } } SECTION("generates a heartbeat message") { From 868a63985ff78eee462e71862137cec70e2358c3 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Tue, 13 Feb 2024 10:14:05 +0100 Subject: [PATCH 3/9] code review --- src/datadog/config_manager.cpp | 1 - src/datadog/tracer_config.h | 41 ----------------------------- src/datadog/tracer_telemetry.cpp | 44 ++++++++++++++++++++++++++++++++ src/datadog/tracer_telemetry.h | 3 ++- 4 files changed, 46 insertions(+), 43 deletions(-) diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index b59a0917..e6275405 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -67,7 +67,6 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { TraceSamplerConfig trace_sampler_cfg; trace_sampler_cfg.sample_rate = *conf.trace_sampling_rate; - trace_sampler_cfg.max_per_second = 200; auto finalized_trace_sampler_cfg = finalize_config(trace_sampler_cfg); if (auto error = finalized_trace_sampler_cfg.if_error()) { diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 9df500b3..56886c8e 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -185,47 +185,6 @@ enum class ConfigName : char { SPAN_SAMPLING_RULES, }; -inline std::string to_string(ConfigName name) { - switch (name) { - case ConfigName::SERVICE_NAME: - return "DD_SERVICE"; - case ConfigName::SERVICE_ENV: - return "DD_ENV"; - case ConfigName::REPORT_TRACES: - return "trace_enabled"; - case ConfigName::TAGS: - return "trace_tags"; - case ConfigName::TRACE_SAMPLING_RATE: - return "trace_sample_rate"; - case ConfigName::SERVICE_VERSION: - return "DD_VERSION"; - case ConfigName::EXTRACTION_STYLES: - return "DD_TRACE_PROPAGATION_STYLE_EXTRACT"; - case ConfigName::INJECTION_STYLES: - return "DD_TRACE_PROPAGATION_STYLE_INJECT"; - case ConfigName::STARTUP_LOGS: - return "DD_TRACE_STARTUP_LOGS"; - case ConfigName::REPORT_TELEMETRY: - return "DD_INSTRUMENTATION_TELEMETRY_ENABLED"; - case ConfigName::DELEGATE_SAMPLING: - return "DD_TRACE_DELEGATE_SAMPLING"; - case ConfigName::GENEREATE_128BIT_TRACE_IDS: - return "DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED"; - case ConfigName::AGENT_URL: - return "DD_TRACE_AGENT_URL"; - case ConfigName::RC_POLL_INTERVAL: - return "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"; - case ConfigName::TRACE_SAMPLING_LIMIT: - return "DD_TRACE_RATE_LIMIT"; - case ConfigName::SPAN_SAMPLING_RULES: - return "DD_SPAN_SAMPLING_RULES"; - case ConfigName::TRACE_SAMPLING_RULES: - return "DD_TRACE_SAMPLING_RULES"; - } - - std::abort(); -} - struct ConfigMetadata { enum class Origin { ENVIRONMENT_VARIABLE, CODE, REMOTE_CONFIG, DEFAULT }; ConfigName name; diff --git a/src/datadog/tracer_telemetry.cpp b/src/datadog/tracer_telemetry.cpp index 96bb5fbd..017fb0cd 100644 --- a/src/datadog/tracer_telemetry.cpp +++ b/src/datadog/tracer_telemetry.cpp @@ -7,6 +7,50 @@ namespace datadog { namespace tracing { +namespace { + +std::string to_string(ConfigName name) { + switch (name) { + case ConfigName::SERVICE_NAME: + return "DD_SERVICE"; + case ConfigName::SERVICE_ENV: + return "DD_ENV"; + case ConfigName::REPORT_TRACES: + return "trace_enabled"; + case ConfigName::TAGS: + return "trace_tags"; + case ConfigName::TRACE_SAMPLING_RATE: + return "trace_sample_rate"; + case ConfigName::SERVICE_VERSION: + return "DD_VERSION"; + case ConfigName::EXTRACTION_STYLES: + return "DD_TRACE_PROPAGATION_STYLE_EXTRACT"; + case ConfigName::INJECTION_STYLES: + return "DD_TRACE_PROPAGATION_STYLE_INJECT"; + case ConfigName::STARTUP_LOGS: + return "DD_TRACE_STARTUP_LOGS"; + case ConfigName::REPORT_TELEMETRY: + return "DD_INSTRUMENTATION_TELEMETRY_ENABLED"; + case ConfigName::DELEGATE_SAMPLING: + return "DD_TRACE_DELEGATE_SAMPLING"; + case ConfigName::GENEREATE_128BIT_TRACE_IDS: + return "DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED"; + case ConfigName::AGENT_URL: + return "DD_TRACE_AGENT_URL"; + case ConfigName::RC_POLL_INTERVAL: + return "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"; + case ConfigName::TRACE_SAMPLING_LIMIT: + return "DD_TRACE_RATE_LIMIT"; + case ConfigName::SPAN_SAMPLING_RULES: + return "DD_SPAN_SAMPLING_RULES"; + case ConfigName::TRACE_SAMPLING_RULES: + return "DD_TRACE_SAMPLING_RULES"; + } + + std::abort(); +} + +} // namespace TracerTelemetry::TracerTelemetry(bool enabled, const Clock& clock, const std::shared_ptr& logger, diff --git a/src/datadog/tracer_telemetry.h b/src/datadog/tracer_telemetry.h index 042ac33c..29ab1c49 100644 --- a/src/datadog/tracer_telemetry.h +++ b/src/datadog/tracer_telemetry.h @@ -49,8 +49,9 @@ class TracerTelemetry { std::string hostname_; std::string integration_name_; std::string integration_version_; + // Track sequence id per payload generated uint64_t seq_id_ = 0; - // Track sequence id per configuration field. + // Track sequence id per configuration field std::unordered_map config_seq_ids; // This structure contains all the metrics that are exposed by tracer // telemetry. From ec6ad2217b4710ab33f9bbac6dc9b7b7753ff461 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Thu, 29 Feb 2024 15:07:26 +0100 Subject: [PATCH 4/9] code review --- src/datadog/datadog_agent_config.cpp | 8 ++- src/datadog/datadog_agent_config.h | 2 + src/datadog/span_sampler_config.cpp | 17 +++++- src/datadog/span_sampler_config.h | 3 + src/datadog/trace_sampler_config.cpp | 27 ++++++++- src/datadog/trace_sampler_config.h | 4 ++ src/datadog/tracer_config.cpp | 86 +++++++++------------------- src/datadog/tracer_config.h | 34 +---------- src/datadog/tracer_telemetry.cpp | 10 ++-- src/datadog/tracer_telemetry.h | 2 +- 10 files changed, 89 insertions(+), 104 deletions(-) diff --git a/src/datadog/datadog_agent_config.cpp b/src/datadog/datadog_agent_config.cpp index 593f8a80..0ee41b05 100644 --- a/src/datadog/datadog_agent_config.cpp +++ b/src/datadog/datadog_agent_config.cpp @@ -46,7 +46,7 @@ Expected load_datadog_agent_env_config() { Expected finalize_config( const DatadogAgentConfig& user_config, const std::shared_ptr& logger, const Clock& clock) { - auto env_config = load_datadog_agent_env_config(); + Expected env_config = load_datadog_agent_env_config(); if (auto error = env_config.if_error()) { return *error; } @@ -122,13 +122,15 @@ Expected finalize_config( "positive number of seconds."}; } - auto url = - value_or(env_config->url, user_config.url, "http://localhost:8126"); + const auto [origin, url] = + pick(env_config->url, user_config.url, "http://localhost:8126"); auto parsed_url = HTTPClient::URL::parse(url); if (auto* error = parsed_url.if_error()) { return std::move(*error); } result.url = *parsed_url; + result.metadata[ConfigName::AGENT_URL] = + ConfigMetadata(ConfigName::AGENT_URL, url, origin); return result; } diff --git a/src/datadog/datadog_agent_config.h b/src/datadog/datadog_agent_config.h index a95bd454..6a90fc77 100644 --- a/src/datadog/datadog_agent_config.h +++ b/src/datadog/datadog_agent_config.h @@ -17,6 +17,7 @@ #include #include "clock.h" +#include "config.h" #include "expected.h" #include "http_client.h" #include "string_view.h" @@ -76,6 +77,7 @@ class FinalizedDatadogAgentConfig { std::chrono::steady_clock::duration request_timeout; std::chrono::steady_clock::duration shutdown_timeout; std::chrono::steady_clock::duration remote_configuration_poll_interval; + std::unordered_map metadata; }; Expected finalize_config( diff --git a/src/datadog/span_sampler_config.cpp b/src/datadog/span_sampler_config.cpp index f0dc295c..bde23eb8 100644 --- a/src/datadog/span_sampler_config.cpp +++ b/src/datadog/span_sampler_config.cpp @@ -13,6 +13,15 @@ namespace datadog { namespace tracing { namespace { +std::string to_string(const std::vector &rules) { + nlohmann::json res; + for (const auto &r : rules) { + res.emplace_back(r.to_json()); + } + + return res.dump(); +} + // `env_var` is the name of the environment variable from which `rules_raw` was // obtained. It's used for error messages. Expected> parse_rules(StringView rules_raw, @@ -207,7 +216,7 @@ SpanSamplerConfig::Rule::Rule(const SpanMatcher &base) : SpanMatcher(base) {} Expected finalize_config( const SpanSamplerConfig &user_config, Logger &logger) { - auto env_config = load_span_sampler_env_config(logger); + Expected env_config = load_span_sampler_env_config(logger); if (auto error = env_config.if_error()) { return *error; } @@ -217,8 +226,14 @@ Expected finalize_config( std::vector rules; if (!env_config->rules.empty()) { rules = env_config->rules; + result.metadata[ConfigName::SPAN_SAMPLING_RULES] = + ConfigMetadata(ConfigName::SPAN_SAMPLING_RULES, to_string(rules), + ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); } else if (!user_config.rules.empty()) { rules = user_config.rules; + result.metadata[ConfigName::SPAN_SAMPLING_RULES] = + ConfigMetadata(ConfigName::SPAN_SAMPLING_RULES, to_string(rules), + ConfigMetadata::Origin::CODE); } for (const auto &rule : rules) { diff --git a/src/datadog/span_sampler_config.h b/src/datadog/span_sampler_config.h index 065049ac..a89e25f8 100644 --- a/src/datadog/span_sampler_config.h +++ b/src/datadog/span_sampler_config.h @@ -7,8 +7,10 @@ // `SpanSamplerConfig` is specified as the `span_sampler` property of // `TracerConfig`. +#include #include +#include "config.h" #include "expected.h" #include "json_fwd.hpp" #include "logger.h" @@ -48,6 +50,7 @@ class FinalizedSpanSamplerConfig { }; std::vector rules; + std::unordered_map metadata; }; Expected finalize_config(const SpanSamplerConfig&, diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp index 97a65e64..df02270e 100644 --- a/src/datadog/trace_sampler_config.cpp +++ b/src/datadog/trace_sampler_config.cpp @@ -130,6 +130,15 @@ Expected load_trace_sampler_env_config() { return env_config; } +std::string to_string(const std::vector &rules) { + nlohmann::json res; + for (const auto &r : rules) { + res.emplace_back(r.to_json()); + } + + return res.dump(); +} + } // namespace TraceSamplerConfig::Rule::Rule(const SpanMatcher &base) : SpanMatcher(base) {} @@ -147,8 +156,14 @@ Expected finalize_config( if (!env_config->rules.empty()) { rules = std::move(env_config->rules); + result.metadata[ConfigName::TRACE_SAMPLING_RULES] = + ConfigMetadata(ConfigName::TRACE_SAMPLING_RULES, to_string(rules), + ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); } else if (!config.rules.empty()) { rules = std::move(config.rules); + result.metadata[ConfigName::TRACE_SAMPLING_RULES] = + ConfigMetadata(ConfigName::TRACE_SAMPLING_RULES, to_string(rules), + ConfigMetadata::Origin::CODE); } for (const auto &rule : rules) { @@ -172,8 +187,14 @@ Expected finalize_config( Optional sample_rate; if (env_config->sample_rate) { sample_rate = env_config->sample_rate; + result.metadata[ConfigName::TRACE_SAMPLING_RATE] = + ConfigMetadata(ConfigName::TRACE_SAMPLING_RATE, to_string(rules), + ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); } else if (config.sample_rate) { sample_rate = config.sample_rate; + result.metadata[ConfigName::TRACE_SAMPLING_RATE] = + ConfigMetadata(ConfigName::TRACE_SAMPLING_RATE, to_string(rules), + ConfigMetadata::Origin::CODE); } // If `sample_rate` was specified, then it translates to a "catch-all" rule @@ -191,8 +212,10 @@ Expected finalize_config( result.rules.push_back(std::move(catch_all)); } - auto max_per_second = - value_or(env_config->max_per_second, config.max_per_second, 200); + const auto [origin, max_per_second] = + pick(env_config->max_per_second, config.max_per_second, 200); + result.metadata[ConfigName::TRACE_SAMPLING_LIMIT] = ConfigMetadata( + ConfigName::TRACE_SAMPLING_LIMIT, std::to_string(max_per_second), origin); const auto allowed_types = {FP_NORMAL, FP_SUBNORMAL}; if (!(max_per_second > 0) || diff --git a/src/datadog/trace_sampler_config.h b/src/datadog/trace_sampler_config.h index 5fab1845..bfe93a50 100644 --- a/src/datadog/trace_sampler_config.h +++ b/src/datadog/trace_sampler_config.h @@ -7,8 +7,10 @@ // `TraceSamplerConfig` is specified as the `trace_sampler` property of // `TracerConfig`. +#include #include +#include "config.h" #include "expected.h" #include "json_fwd.hpp" #include "optional.h" @@ -45,6 +47,8 @@ class FinalizedTraceSamplerConfig { std::vector rules; double max_per_second; + + std::unordered_map metadata; }; Expected finalize_config( diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp index 5dc0da7d..f8b8ac27 100644 --- a/src/datadog/tracer_config.cpp +++ b/src/datadog/tracer_config.cpp @@ -22,31 +22,6 @@ namespace { std::string to_string(bool b) { return b ? "true" : "false"; } -// TODO: use `to_chars` -// std::string to_string(double d) { -// std::stringstream stream; -// stream << std::fixed << std::setprecision(1) << d; -// return stream.str(); -// } -// -// std::string to_string(const std::vector &rules) { -// nlohmann::json res; -// for (const auto &r : rules) { -// res.emplace_back(r.to_json()); -// } -// -// return res.dump(); -// } -// -// std::string to_string(const std::vector &rules) { -// nlohmann::json res; -// for (const auto &r : rules) { -// res.emplace_back(r.to_json()); -// } -// -// return res.dump(); -// } - std::string join_propagation_styles( const std::vector &values) { auto to_string = [](PropagationStyle style) { @@ -318,19 +293,6 @@ Expected load_tracer_env_config(Logger &logger) { return env_cfg; } -// TODO: Use SFINAE w/ is_trivially_constructible? -template -std::pair pick(const Optional &env, - const Optional &user, - U default_v) { - if (env) { - return {ConfigMetadata::Origin::ENVIRONMENT_VARIABLE, *env}; - } else if (user) { - return {ConfigMetadata::Origin::CODE, *user}; - } - return {ConfigMetadata::Origin::DEFAULT, default_v}; -} - } // namespace Expected finalize_config(const TracerConfig &config) { @@ -342,18 +304,19 @@ Expected finalize_config(const TracerConfig &user_config, auto logger = user_config.logger ? user_config.logger : std::make_shared(); - auto env = load_tracer_env_config(*logger); - if (auto error = env.if_error()) { + Expected env_config = load_tracer_env_config(*logger); + if (auto error = env_config.if_error()) { return *error; } FinalizedTracerConfig final_config; final_config.clock = clock; + final_config.logger = logger; ConfigMetadata::Origin origin; std::tie(origin, final_config.defaults.service) = - pick(env->service, user_config.service, ""); + pick(env_config->service, user_config.service, ""); if (final_config.defaults.service.empty()) { return Error{Error::SERVICE_NAME_REQUIRED, "Service name is required."}; @@ -363,25 +326,25 @@ Expected finalize_config(const TracerConfig &user_config, ConfigName::SERVICE_NAME, final_config.defaults.service, origin); final_config.defaults.service_type = - value_or(env->service_type, user_config.service_type, "web"); + value_or(env_config->service_type, user_config.service_type, "web"); // DD_ENV std::tie(origin, final_config.defaults.environment) = - pick(env->environment, user_config.environment, ""); + pick(env_config->environment, user_config.environment, ""); final_config.metadata[ConfigName::SERVICE_ENV] = ConfigMetadata( ConfigName::SERVICE_ENV, final_config.defaults.environment, origin); // DD_VERSION std::tie(origin, final_config.defaults.version) = - pick(env->version, user_config.version, ""); + pick(env_config->version, user_config.version, ""); final_config.metadata[ConfigName::SERVICE_VERSION] = ConfigMetadata( ConfigName::SERVICE_VERSION, final_config.defaults.version, origin); - final_config.defaults.name = value_or(env->name, user_config.name, ""); + final_config.defaults.name = value_or(env_config->name, user_config.name, ""); // DD_TAGS std::tie(origin, final_config.defaults.tags) = - pick(env->tags, user_config.tags, + pick(env_config->tags, user_config.tags, std::unordered_map{}); final_config.metadata[ConfigName::TAGS] = ConfigMetadata( ConfigName::TAGS, join_tags(final_config.defaults.tags), origin); @@ -391,7 +354,7 @@ Expected finalize_config(const TracerConfig &user_config, PropagationStyle::DATADOG, PropagationStyle::W3C}; std::tie(origin, final_config.extraction_styles) = - pick(env->extraction_styles, user_config.extraction_styles, + pick(env_config->extraction_styles, user_config.extraction_styles, default_propagation_styles); if (final_config.extraction_styles.empty()) { return Error{Error::MISSING_SPAN_EXTRACTION_STYLE, @@ -403,7 +366,7 @@ Expected finalize_config(const TracerConfig &user_config, // Injection Styles std::tie(origin, final_config.injection_styles) = - pick(env->injection_styles, user_config.injection_styles, + pick(env_config->injection_styles, user_config.injection_styles, default_propagation_styles); if (final_config.injection_styles.empty()) { return Error{Error::MISSING_SPAN_INJECTION_STYLE, @@ -415,50 +378,52 @@ Expected finalize_config(const TracerConfig &user_config, // Startup Logs std::tie(origin, final_config.log_on_startup) = - pick(env->log_on_startup, user_config.log_on_startup, true); + pick(env_config->log_on_startup, user_config.log_on_startup, true); final_config.metadata[ConfigName::STARTUP_LOGS] = ConfigMetadata( ConfigName::STARTUP_LOGS, to_string(final_config.log_on_startup), origin); // Report traces std::tie(origin, final_config.report_traces) = - pick(env->report_traces, user_config.report_traces, true); + pick(env_config->report_traces, user_config.report_traces, true); final_config.metadata[ConfigName::REPORT_TRACES] = ConfigMetadata( ConfigName::REPORT_TRACES, to_string(final_config.report_traces), origin); // Report telemetry std::tie(origin, final_config.report_telemetry) = - pick(env->report_telemetry, user_config.report_telemetry, true); + pick(env_config->report_telemetry, user_config.report_telemetry, true); final_config.metadata[ConfigName::REPORT_TELEMETRY] = ConfigMetadata(ConfigName::REPORT_TELEMETRY, to_string(final_config.report_traces), origin); // Report hostname final_config.report_hostname = - value_or(env->report_hostname, user_config.report_hostname, false); + value_or(env_config->report_hostname, user_config.report_hostname, false); // Delegate Sampling - std::tie(origin, final_config.delegate_trace_sampling) = pick( - env->delegate_trace_sampling, user_config.delegate_trace_sampling, false); + std::tie(origin, final_config.delegate_trace_sampling) = + pick(env_config->delegate_trace_sampling, + user_config.delegate_trace_sampling, false); final_config.metadata[ConfigName::DELEGATE_SAMPLING] = ConfigMetadata(ConfigName::DELEGATE_SAMPLING, to_string(final_config.delegate_trace_sampling), origin); // Tags Header Size final_config.tags_header_size = value_or( - env->max_tags_header_size, user_config.max_tags_header_size, 512); + env_config->max_tags_header_size, user_config.max_tags_header_size, 512); // 128b Trace Ids std::tie(origin, final_config.generate_128bit_trace_ids) = - pick(env->generate_128bit_trace_ids, + pick(env_config->generate_128bit_trace_ids, user_config.generate_128bit_trace_ids, true); final_config.metadata[ConfigName::GENEREATE_128BIT_TRACE_IDS] = ConfigMetadata(ConfigName::DELEGATE_SAMPLING, to_string(final_config.delegate_trace_sampling), origin); + // Integration name & version final_config.integration_name = - value_or(env->integration_name, user_config.integration_name, ""); - final_config.integration_version = - value_or(env->integration_version, user_config.integration_version, ""); + value_or(env_config->integration_name, user_config.integration_name, ""); + final_config.integration_version = value_or( + env_config->integration_version, user_config.integration_version, ""); if (user_config.runtime_id) { final_config.runtime_id = user_config.runtime_id; @@ -471,12 +436,14 @@ Expected finalize_config(const TracerConfig &user_config, return std::move(*error); } final_config.collector = *finalized; + final_config.metadata.merge(finalized->metadata); } else { final_config.collector = user_config.collector; } if (auto trace_sampler_config = finalize_config(user_config.trace_sampler)) { final_config.trace_sampler = std::move(*trace_sampler_config); + final_config.metadata.merge(trace_sampler_config->metadata); } else { return std::move(trace_sampler_config.error()); } @@ -484,6 +451,7 @@ Expected finalize_config(const TracerConfig &user_config, if (auto span_sampler_config = finalize_config(user_config.span_sampler, *logger)) { final_config.span_sampler = std::move(*span_sampler_config); + final_config.metadata.merge(span_sampler_config->metadata); } else { return std::move(span_sampler_config.error()); } diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 56886c8e..37c2dcc1 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -10,6 +10,7 @@ #include #include "clock.h" +#include "config.h" #include "datadog_agent_config.h" #include "error.h" #include "expected.h" @@ -165,39 +166,6 @@ struct TracerConfig { Optional report_service_as_default; }; -enum class ConfigName : char { - SERVICE_NAME, - SERVICE_ENV, - SERVICE_VERSION, - REPORT_TRACES, - TAGS, - TRACE_SAMPLING_RATE, - EXTRACTION_STYLES, - INJECTION_STYLES, - STARTUP_LOGS, - REPORT_TELEMETRY, - DELEGATE_SAMPLING, - GENEREATE_128BIT_TRACE_IDS, - AGENT_URL, - RC_POLL_INTERVAL, - TRACE_SAMPLING_LIMIT, - TRACE_SAMPLING_RULES, - SPAN_SAMPLING_RULES, -}; - -struct ConfigMetadata { - enum class Origin { ENVIRONMENT_VARIABLE, CODE, REMOTE_CONFIG, DEFAULT }; - ConfigName name; - std::string value; - Origin origin; - Optional error; - - ConfigMetadata() = default; - ConfigMetadata(ConfigName n, std::string v, Origin orig, - Optional err = nullopt) - : name(n), value(std::move(v)), origin(orig), error(err) {} -}; - // `FinalizedTracerConfig` contains `Tracer` implementation details derived from // a valid `TracerConfig` and accompanying environment. // `FinalizedTracerConfig` must be obtained by calling `finalize_config`. diff --git a/src/datadog/tracer_telemetry.cpp b/src/datadog/tracer_telemetry.cpp index 017fb0cd..3e7d18ac 100644 --- a/src/datadog/tracer_telemetry.cpp +++ b/src/datadog/tracer_telemetry.cpp @@ -9,20 +9,18 @@ namespace datadog { namespace tracing { namespace { -std::string to_string(ConfigName name) { +std::string to_string(datadog::tracing::ConfigName name) { switch (name) { case ConfigName::SERVICE_NAME: return "DD_SERVICE"; case ConfigName::SERVICE_ENV: return "DD_ENV"; + case ConfigName::SERVICE_VERSION: + return "DD_VERSION"; case ConfigName::REPORT_TRACES: return "trace_enabled"; case ConfigName::TAGS: return "trace_tags"; - case ConfigName::TRACE_SAMPLING_RATE: - return "trace_sample_rate"; - case ConfigName::SERVICE_VERSION: - return "DD_VERSION"; case ConfigName::EXTRACTION_STYLES: return "DD_TRACE_PROPAGATION_STYLE_EXTRACT"; case ConfigName::INJECTION_STYLES: @@ -39,6 +37,8 @@ std::string to_string(ConfigName name) { return "DD_TRACE_AGENT_URL"; case ConfigName::RC_POLL_INTERVAL: return "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"; + case ConfigName::TRACE_SAMPLING_RATE: + return "trace_sample_rate"; case ConfigName::TRACE_SAMPLING_LIMIT: return "DD_TRACE_RATE_LIMIT"; case ConfigName::SPAN_SAMPLING_RULES: diff --git a/src/datadog/tracer_telemetry.h b/src/datadog/tracer_telemetry.h index 29ab1c49..eab9b6df 100644 --- a/src/datadog/tracer_telemetry.h +++ b/src/datadog/tracer_telemetry.h @@ -28,10 +28,10 @@ #include #include "clock.h" +#include "config.h" #include "json.hpp" #include "metrics.h" #include "runtime_id.h" -#include "tracer_config.h" #include "tracer_signature.h" namespace datadog { From e2bb8f465b2245055cb150d6d5bc8bcf69211c65 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Fri, 1 Mar 2024 11:22:16 +0100 Subject: [PATCH 5/9] code review --- BUILD.bazel | 3 ++ CMakeLists.txt | 3 ++ src/datadog/config.h | 72 ++++++++++++++++++++++++++++ src/datadog/config_manager.cpp | 70 ++++++++------------------- src/datadog/config_manager.h | 14 +++--- src/datadog/datadog_agent_config.cpp | 6 +-- src/datadog/trace_sampler_config.cpp | 14 +++--- src/datadog/tracer_config.cpp | 60 ++--------------------- src/datadog/tracer_config.h | 1 - src/datadog/tracer_telemetry.cpp | 26 +++++----- src/datadog/tracer_telemetry.h | 3 ++ test/test_tracer_telemetry.cpp | 10 ++-- 12 files changed, 142 insertions(+), 140 deletions(-) create mode 100644 src/datadog/config.h diff --git a/BUILD.bazel b/BUILD.bazel index 1a0523a4..ae4a9631 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -35,6 +35,7 @@ cc_library( "src/datadog/span_matcher.cpp", "src/datadog/span_sampler_config.cpp", "src/datadog/span_sampler.cpp", + "src/datadog/string_util.cpp", "src/datadog/tag_propagation.cpp", "src/datadog/tags.cpp", "src/datadog/threaded_event_scheduler.cpp", @@ -51,6 +52,7 @@ cc_library( hdrs = [ "src/datadog/base64.h", "src/datadog/cerr_logger.h", + "src/datadog/config.h", "src/datadog/clock.h", "src/datadog/config_manager.h", "src/datadog/config_update.h", @@ -99,6 +101,7 @@ cc_library( "src/datadog/span_matcher.h", "src/datadog/span_sampler_config.h", "src/datadog/span_sampler.h", + "src/datadog/string_util.h", "src/datadog/string_view.h", "src/datadog/tag_propagation.h", "src/datadog/tags.h", diff --git a/CMakeLists.txt b/CMakeLists.txt index b36d3399..c1838193 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -121,6 +121,7 @@ target_sources(dd_trace_cpp-objects PRIVATE src/datadog/span_matcher.cpp src/datadog/span_sampler_config.cpp src/datadog/span_sampler.cpp + src/datadog/string_util.cpp src/datadog/tags.cpp src/datadog/tag_propagation.cpp src/datadog/threaded_event_scheduler.cpp @@ -142,6 +143,7 @@ target_sources(dd_trace_cpp-objects PUBLIC BASE_DIRS src/ FILES src/datadog/base64.h + src/datadog/config.h src/datadog/cerr_logger.h src/datadog/clock.h src/datadog/config_manager.h @@ -191,6 +193,7 @@ target_sources(dd_trace_cpp-objects PUBLIC src/datadog/span_matcher.h src/datadog/span_sampler_config.h src/datadog/span_sampler.h + src/datadog/string_util.h src/datadog/string_view.h src/datadog/tag_propagation.h src/datadog/tags.h diff --git a/src/datadog/config.h b/src/datadog/config.h new file mode 100644 index 00000000..aa987865 --- /dev/null +++ b/src/datadog/config.h @@ -0,0 +1,72 @@ +#pragma once + +#include "error.h" +#include "optional.h" + +namespace datadog { +namespace tracing { + +// Enumerates available configuration names for the tracing library +enum class ConfigName : char { + SERVICE_NAME, + SERVICE_ENV, + SERVICE_VERSION, + REPORT_TRACES, + TAGS, + EXTRACTION_STYLES, + INJECTION_STYLES, + STARTUP_LOGS, + REPORT_TELEMETRY, + DELEGATE_SAMPLING, + GENEREATE_128BIT_TRACE_IDS, + AGENT_URL, + RC_POLL_INTERVAL, + TRACE_SAMPLING_RATE, + TRACE_SAMPLING_LIMIT, + TRACE_SAMPLING_RULES, + SPAN_SAMPLING_RULES, +}; + +// Represents metadata for configuration parameters +struct ConfigMetadata { + enum class Origin : char { + ENVIRONMENT_VARIABLE, // Originating from environment variables + CODE, // Defined in code + REMOTE_CONFIG, // Retrieeved from remote configuration + DEFAULT // Default value + }; + + // Name of the configuration parameter + ConfigName name; + // Value of the configuration parameter + std::string value; + // Origin of the configuration parameter + Origin origin; + // Optional error associated with the configuration parameter + Optional error; + + ConfigMetadata() = default; + ConfigMetadata(ConfigName n, std::string v, Origin orig, + Optional err = nullopt) + : name(n), value(std::move(v)), origin(orig), error(err) {} +}; + +// Return a pair containing the configuration origin and value of a +// configuration value chosen from one of the specified `from_env`, +// `from_config`, and `fallback`. This function defines the relative precedence +// among configuration values originating from the environment, programmatic +// configuration, and default configuration. +template +std::pair pick(const Optional &from_env, + const Optional &from_user, + DefaultValue fallback) { + if (from_env) { + return {ConfigMetadata::Origin::ENVIRONMENT_VARIABLE, *from_env}; + } else if (from_user) { + return {ConfigMetadata::Origin::CODE, *from_user}; + } + return {ConfigMetadata::Origin::DEFAULT, fallback}; +} + +} // namespace tracing +} // namespace datadog diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index e6275405..f36ab3c8 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -3,34 +3,11 @@ #include #include "parse_util.h" +#include "string_util.h" #include "trace_sampler.h" namespace datadog { namespace tracing { -namespace { - -std::string join(const std::vector& values, - const char* const separator) { - if (values.empty()) return {}; - auto it = values.cbegin(); - - std::string res{*it}; - for (++it; it != values.cend(); ++it) { - res += separator; - append(res, *it); - } - - return res; -} - -// TODO: use `to_chars` -std::string to_string(double d) { - std::stringstream stream; - stream << std::fixed << std::setprecision(1) << d; - return stream.str(); -} - -} // namespace ConfigManager::ConfigManager(const FinalizedTracerConfig& config) : clock_(config.clock), @@ -76,16 +53,15 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { auto trace_sampler = std::make_shared(*finalized_trace_sampler_cfg, clock_); - if (trace_sampler != trace_sampler_.get()) { - trace_sampler_ = std::move(trace_sampler); - telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); - } - } else { - if (!trace_sampler_.is_default()) { - trace_sampler_.reset(); - telemetry_conf.emplace_back( - default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); - } + // This reset rate limiting and `TraceSampler` has no `operator==`. + // TODO: Instead of creating another `TraceSampler`, we should + // update the default sampling rate + trace_sampler_ = std::move(trace_sampler); + telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); + } else if (!trace_sampler_.is_original_value()) { + trace_sampler_.reset(); + telemetry_conf.emplace_back( + default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); } if (conf.tags) { @@ -105,26 +81,22 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { span_defaults_ = new_span_defaults; telemetry_conf.emplace_back(std::move(tags_metadata)); } - } else { - if (!span_defaults_.is_default()) { - span_defaults_.reset(); - telemetry_conf.emplace_back(default_config_metadata_[ConfigName::TAGS]); - } + } else if (!span_defaults_.is_original_value()) { + span_defaults_.reset(); + telemetry_conf.emplace_back(default_config_metadata_[ConfigName::TAGS]); } if (conf.report_traces) { if (conf.report_traces != report_traces_) { report_traces_ = *conf.report_traces; telemetry_conf.emplace_back(ConfigName::REPORT_TRACES, - *conf.report_traces ? "true" : "false", + to_string(*conf.report_traces), ConfigMetadata::Origin::REMOTE_CONFIG); } - } else { - if (!report_traces_.is_default()) { - report_traces_.reset(); - telemetry_conf.emplace_back( - default_config_metadata_[ConfigName::REPORT_TRACES]); - } + } else if (!report_traces_.is_original_value()) { + report_traces_.reset(); + telemetry_conf.emplace_back( + default_config_metadata_[ConfigName::REPORT_TRACES]); } return telemetry_conf; @@ -134,18 +106,18 @@ std::vector ConfigManager::reset() { std::vector config_metadata; std::lock_guard lock(mutex_); - if (!trace_sampler_.is_default()) { + if (!trace_sampler_.is_original_value()) { trace_sampler_.reset(); config_metadata.emplace_back( default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); } - if (!span_defaults_.is_default()) { + if (!span_defaults_.is_original_value()) { span_defaults_.reset(); config_metadata.emplace_back(default_config_metadata_[ConfigName::TAGS]); } - if (!report_traces_.is_default()) { + if (!report_traces_.is_original_value()) { report_traces_.reset(); config_metadata.emplace_back( default_config_metadata_[ConfigName::REPORT_TRACES]); diff --git a/src/datadog/config_manager.h b/src/datadog/config_manager.h index b2925591..c46966a3 100644 --- a/src/datadog/config_manager.h +++ b/src/datadog/config_manager.h @@ -19,25 +19,25 @@ namespace tracing { class ConfigManager { template class DynamicConf { - T default_value_; + T original_value_; T current_value_; - bool is_default_value_; + bool is_original_value_; public: explicit DynamicConf(T v) - : default_value_(v), current_value_(v), is_default_value_(true) {} + : original_value_(v), current_value_(v), is_original_value_(true) {} void update(T new_v) { current_value_ = new_v; - is_default_value_ = false; + is_original_value_ = false; } void reset() { - current_value_ = default_value_; - is_default_value_ = true; + current_value_ = original_value_; + is_original_value_ = true; } - bool is_default() const { return is_default_value_; } + bool is_original_value() const { return is_original_value_; } T get() { return current_value_; } const T& get() const { return current_value_; } diff --git a/src/datadog/datadog_agent_config.cpp b/src/datadog/datadog_agent_config.cpp index 0ee41b05..c42f306d 100644 --- a/src/datadog/datadog_agent_config.cpp +++ b/src/datadog/datadog_agent_config.cpp @@ -76,7 +76,7 @@ Expected finalize_config( if (auto flush_interval_milliseconds = value_or(env_config->flush_interval_milliseconds, - user_config.flush_interval_milliseconds, 200); + user_config.flush_interval_milliseconds, 2000); flush_interval_milliseconds > 0) { result.flush_interval = std::chrono::milliseconds(flush_interval_milliseconds); @@ -88,7 +88,7 @@ Expected finalize_config( if (auto request_timeout_milliseconds = value_or(env_config->request_timeout_milliseconds, - user_config.request_timeout_milliseconds, 200); + user_config.request_timeout_milliseconds, 2000); request_timeout_milliseconds > 0) { result.request_timeout = std::chrono::milliseconds(request_timeout_milliseconds); @@ -100,7 +100,7 @@ Expected finalize_config( if (auto shutdown_timeout_milliseconds = value_or(env_config->shutdown_timeout_milliseconds, - user_config.shutdown_timeout_milliseconds, 200); + user_config.shutdown_timeout_milliseconds, 2000); shutdown_timeout_milliseconds > 0) { result.shutdown_timeout = std::chrono::milliseconds(shutdown_timeout_milliseconds); diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp index df02270e..ebaad72c 100644 --- a/src/datadog/trace_sampler_config.cpp +++ b/src/datadog/trace_sampler_config.cpp @@ -145,7 +145,7 @@ TraceSamplerConfig::Rule::Rule(const SpanMatcher &base) : SpanMatcher(base) {} Expected finalize_config( const TraceSamplerConfig &config) { - auto env_config = load_trace_sampler_env_config(); + Expected env_config = load_trace_sampler_env_config(); if (auto error = env_config.if_error()) { return *error; } @@ -187,14 +187,14 @@ Expected finalize_config( Optional sample_rate; if (env_config->sample_rate) { sample_rate = env_config->sample_rate; - result.metadata[ConfigName::TRACE_SAMPLING_RATE] = - ConfigMetadata(ConfigName::TRACE_SAMPLING_RATE, to_string(rules), - ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); + result.metadata[ConfigName::TRACE_SAMPLING_RATE] = ConfigMetadata( + ConfigName::TRACE_SAMPLING_RATE, std::to_string(*sample_rate), + ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); } else if (config.sample_rate) { sample_rate = config.sample_rate; - result.metadata[ConfigName::TRACE_SAMPLING_RATE] = - ConfigMetadata(ConfigName::TRACE_SAMPLING_RATE, to_string(rules), - ConfigMetadata::Origin::CODE); + result.metadata[ConfigName::TRACE_SAMPLING_RATE] = ConfigMetadata( + ConfigName::TRACE_SAMPLING_RATE, std::to_string(*sample_rate), + ConfigMetadata::Origin::CODE); } // If `sample_rate` was specified, then it translates to a "catch-all" rule diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp index f8b8ac27..5df8ee3e 100644 --- a/src/datadog/tracer_config.cpp +++ b/src/datadog/tracer_config.cpp @@ -14,63 +14,13 @@ #include "environment.h" #include "json.hpp" #include "parse_util.h" +#include "string_util.h" #include "string_view.h" namespace datadog { namespace tracing { namespace { -std::string to_string(bool b) { return b ? "true" : "false"; } - -std::string join_propagation_styles( - const std::vector &values) { - auto to_string = [](PropagationStyle style) { - switch (style) { - case PropagationStyle::B3: - return "B3"; - case PropagationStyle::DATADOG: - return "Datadog"; - case PropagationStyle::W3C: - return "W3C"; - case PropagationStyle::NONE: - return "None"; - } - return ""; ///< unlikely - }; - - if (values.empty()) return {}; - auto it = values.cbegin(); - - std::string res{to_string(*it)}; - for (++it; it != values.cend(); ++it) { - res += ','; - res += to_string(*it); - } - - return res; -} - -std::string join_tags( - const std::unordered_map &values) { - if (values.empty()) return {}; - - auto it = values.cbegin(); - - std::string res; - res += it->first; - res += ":"; - res += it->second; - - for (++it; it != values.cend(); ++it) { - res += ","; - res += it->first; - res += ":"; - res += it->second; - } - - return res; -} - bool falsy(StringView text) { auto lower = std::string{text}; to_lower(lower); @@ -411,12 +361,12 @@ Expected finalize_config(const TracerConfig &user_config, final_config.tags_header_size = value_or( env_config->max_tags_header_size, user_config.max_tags_header_size, 512); - // 128b Trace Ids + // 128b Trace IDs std::tie(origin, final_config.generate_128bit_trace_ids) = pick(env_config->generate_128bit_trace_ids, user_config.generate_128bit_trace_ids, true); final_config.metadata[ConfigName::GENEREATE_128BIT_TRACE_IDS] = - ConfigMetadata(ConfigName::DELEGATE_SAMPLING, + ConfigMetadata(ConfigName::GENEREATE_128BIT_TRACE_IDS, to_string(final_config.delegate_trace_sampling), origin); // Integration name & version @@ -442,16 +392,16 @@ Expected finalize_config(const TracerConfig &user_config, } if (auto trace_sampler_config = finalize_config(user_config.trace_sampler)) { - final_config.trace_sampler = std::move(*trace_sampler_config); final_config.metadata.merge(trace_sampler_config->metadata); + final_config.trace_sampler = std::move(*trace_sampler_config); } else { return std::move(trace_sampler_config.error()); } if (auto span_sampler_config = finalize_config(user_config.span_sampler, *logger)) { - final_config.span_sampler = std::move(*span_sampler_config); final_config.metadata.merge(span_sampler_config->metadata); + final_config.span_sampler = std::move(*span_sampler_config); } else { return std::move(span_sampler_config.error()); } diff --git a/src/datadog/tracer_config.h b/src/datadog/tracer_config.h index 37c2dcc1..49cf6ab2 100644 --- a/src/datadog/tracer_config.h +++ b/src/datadog/tracer_config.h @@ -14,7 +14,6 @@ #include "datadog_agent_config.h" #include "error.h" #include "expected.h" -#include "json.hpp" #include "propagation_style.h" #include "runtime_id.h" #include "span_defaults.h" diff --git a/src/datadog/tracer_telemetry.cpp b/src/datadog/tracer_telemetry.cpp index 3e7d18ac..d58ab82e 100644 --- a/src/datadog/tracer_telemetry.cpp +++ b/src/datadog/tracer_telemetry.cpp @@ -12,39 +12,39 @@ namespace { std::string to_string(datadog::tracing::ConfigName name) { switch (name) { case ConfigName::SERVICE_NAME: - return "DD_SERVICE"; + return "service"; case ConfigName::SERVICE_ENV: - return "DD_ENV"; + return "env"; case ConfigName::SERVICE_VERSION: - return "DD_VERSION"; + return "application_version"; case ConfigName::REPORT_TRACES: return "trace_enabled"; case ConfigName::TAGS: return "trace_tags"; case ConfigName::EXTRACTION_STYLES: - return "DD_TRACE_PROPAGATION_STYLE_EXTRACT"; + return "trace_propagation_style_extract"; case ConfigName::INJECTION_STYLES: - return "DD_TRACE_PROPAGATION_STYLE_INJECT"; + return "trace_propagation_style_inject"; case ConfigName::STARTUP_LOGS: - return "DD_TRACE_STARTUP_LOGS"; + return "trace_startup_logs_enabled"; case ConfigName::REPORT_TELEMETRY: - return "DD_INSTRUMENTATION_TELEMETRY_ENABLED"; + return "instrumentation_telemetry_enabled"; case ConfigName::DELEGATE_SAMPLING: return "DD_TRACE_DELEGATE_SAMPLING"; case ConfigName::GENEREATE_128BIT_TRACE_IDS: - return "DD_TRACE_128_BIT_TRACEID_GENERATION_ENABLED"; + return "trace_128_bits_id_enabled"; case ConfigName::AGENT_URL: - return "DD_TRACE_AGENT_URL"; + return "trace_agent_url"; case ConfigName::RC_POLL_INTERVAL: - return "DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"; + return "remote_config_poll_interval"; case ConfigName::TRACE_SAMPLING_RATE: return "trace_sample_rate"; case ConfigName::TRACE_SAMPLING_LIMIT: - return "DD_TRACE_RATE_LIMIT"; + return "trace_rate_limit"; case ConfigName::SPAN_SAMPLING_RULES: - return "DD_SPAN_SAMPLING_RULES"; + return "span_sample_rules"; case ConfigName::TRACE_SAMPLING_RULES: - return "DD_TRACE_SAMPLING_RULES"; + return "trace_sample_rules"; } std::abort(); diff --git a/src/datadog/tracer_telemetry.h b/src/datadog/tracer_telemetry.h index eab9b6df..6a6ffb33 100644 --- a/src/datadog/tracer_telemetry.h +++ b/src/datadog/tracer_telemetry.h @@ -13,6 +13,7 @@ // - `app-heartbeat` // - `generate-metrics` // - `app-closing` +// - `app-client-configuration-change` // // `app-started` messages are sent as part of initializing the tracer. // @@ -25,6 +26,8 @@ // last `app-heartbeat` event, a `generate-metrics` message is also included in // the batch. // +// `app-client-configuration-change` messages are sent as soon as the tracer +// configuration has been updated by a Remote Configuration event. #include #include "clock.h" diff --git a/test/test_tracer_telemetry.cpp b/test/test_tracer_telemetry.cpp index 95c359ba..534a17d4 100644 --- a/test/test_tracer_telemetry.cpp +++ b/test/test_tracer_telemetry.cpp @@ -83,7 +83,7 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { // clang-format off const auto expected_conf = nlohmann::json({ - {"name", "DD_SERVICE"}, + {"name", "service"}, {"value", "foo"}, {"seq_id", 1}, {"origin", "code"}, @@ -122,10 +122,10 @@ TEST_CASE("Tracer telemetry", "[telemetry]") { CHECK(config_change_message["payload"]["configuration"].size() == 2); const std::unordered_map expected_json{ - {"DD_SERVICE", nlohmann::json{{"name", "DD_SERVICE"}, - {"value", "increase seq_id"}, - {"seq_id", 2}, - {"origin", "env_var"}}}, + {"service", nlohmann::json{{"name", "service"}, + {"value", "increase seq_id"}, + {"seq_id", 2}, + {"origin", "env_var"}}}, {"trace_enabled", nlohmann::json{{"name", "trace_enabled"}, {"value", ""}, From 93b846814270ee334e589e016dacd328153f5d11 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Wed, 6 Mar 2024 11:40:08 +0100 Subject: [PATCH 6/9] code review #3 --- src/datadog/config_manager.cpp | 95 +++++++++++++--------------- src/datadog/config_manager.h | 56 +++++++++------- src/datadog/string_util.cpp | 81 ++++++++++++++++++++++++ src/datadog/string_util.h | 29 +++++++++ src/datadog/trace_sampler_config.cpp | 5 +- test/test_remote_config.cpp | 2 +- 6 files changed, 190 insertions(+), 78 deletions(-) create mode 100644 src/datadog/string_util.cpp create mode 100644 src/datadog/string_util.h diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index f36ab3c8..452887eb 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -11,7 +11,7 @@ namespace tracing { ConfigManager::ConfigManager(const FinalizedTracerConfig& config) : clock_(config.clock), - default_config_metadata_(config.metadata), + default_metadata_(config.metadata), trace_sampler_( std::make_shared(config.trace_sampler, clock_)), span_defaults_(std::make_shared(config.defaults)), @@ -19,27 +19,30 @@ ConfigManager::ConfigManager(const FinalizedTracerConfig& config) std::shared_ptr ConfigManager::trace_sampler() { std::lock_guard lock(mutex_); - return trace_sampler_; + return trace_sampler_.value(); } std::shared_ptr ConfigManager::span_defaults() { std::lock_guard lock(mutex_); - return span_defaults_; + return span_defaults_.value(); } bool ConfigManager::report_traces() { std::lock_guard lock(mutex_); - return report_traces_; + return report_traces_.value(); } std::vector ConfigManager::update(const ConfigUpdate& conf) { - std::vector telemetry_conf; + std::vector metadata; std::lock_guard lock(mutex_); - if (conf.trace_sampling_rate) { + if (!conf.trace_sampling_rate) { + reset_config(ConfigName::TRACE_SAMPLING_RATE, trace_sampler_, metadata); + } else { ConfigMetadata trace_sampling_metadata( - ConfigName::TRACE_SAMPLING_RATE, to_string(*conf.trace_sampling_rate), + ConfigName::TRACE_SAMPLING_RATE, + to_string(*conf.trace_sampling_rate, 1), ConfigMetadata::Origin::REMOTE_CONFIG); TraceSamplerConfig trace_sampler_cfg; @@ -55,16 +58,14 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { // This reset rate limiting and `TraceSampler` has no `operator==`. // TODO: Instead of creating another `TraceSampler`, we should - // update the default sampling rate + // update the default sampling rate. trace_sampler_ = std::move(trace_sampler); - telemetry_conf.emplace_back(std::move(trace_sampling_metadata)); - } else if (!trace_sampler_.is_original_value()) { - trace_sampler_.reset(); - telemetry_conf.emplace_back( - default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); + metadata.emplace_back(std::move(trace_sampling_metadata)); } - if (conf.tags) { + if (!conf.tags) { + reset_config(ConfigName::TAGS, span_defaults_, metadata); + } else { ConfigMetadata tags_metadata(ConfigName::TAGS, join(*conf.tags, ","), ConfigMetadata::Origin::REMOTE_CONFIG); @@ -73,64 +74,56 @@ std::vector ConfigManager::update(const ConfigUpdate& conf) { tags_metadata.error = *error; } - if (*parsed_tags != span_defaults_.get()->tags) { + if (*parsed_tags != span_defaults_.value()->tags) { auto new_span_defaults = - std::make_shared(*span_defaults_.get()); + std::make_shared(*span_defaults_.value()); new_span_defaults->tags = std::move(*parsed_tags); span_defaults_ = new_span_defaults; - telemetry_conf.emplace_back(std::move(tags_metadata)); + metadata.emplace_back(std::move(tags_metadata)); } - } else if (!span_defaults_.is_original_value()) { - span_defaults_.reset(); - telemetry_conf.emplace_back(default_config_metadata_[ConfigName::TAGS]); } - if (conf.report_traces) { - if (conf.report_traces != report_traces_) { + if (!conf.report_traces) { + reset_config(ConfigName::REPORT_TRACES, report_traces_, metadata); + } else { + if (conf.report_traces != report_traces_.value()) { report_traces_ = *conf.report_traces; - telemetry_conf.emplace_back(ConfigName::REPORT_TRACES, - to_string(*conf.report_traces), - ConfigMetadata::Origin::REMOTE_CONFIG); + metadata.emplace_back(ConfigName::REPORT_TRACES, + to_string(*conf.report_traces), + ConfigMetadata::Origin::REMOTE_CONFIG); } - } else if (!report_traces_.is_original_value()) { - report_traces_.reset(); - telemetry_conf.emplace_back( - default_config_metadata_[ConfigName::REPORT_TRACES]); } - return telemetry_conf; + return metadata; } -std::vector ConfigManager::reset() { - std::vector config_metadata; +template +void ConfigManager::reset_config(ConfigName name, T& conf, + std::vector& metadata) { + if (conf.is_original_value()) return; - std::lock_guard lock(mutex_); - if (!trace_sampler_.is_original_value()) { - trace_sampler_.reset(); - config_metadata.emplace_back( - default_config_metadata_[ConfigName::TRACE_SAMPLING_RATE]); - } + conf.reset(); + metadata.emplace_back(default_metadata_[name]); +} - if (!span_defaults_.is_original_value()) { - span_defaults_.reset(); - config_metadata.emplace_back(default_config_metadata_[ConfigName::TAGS]); - } +std::vector ConfigManager::reset() { + std::vector metadata; - if (!report_traces_.is_original_value()) { - report_traces_.reset(); - config_metadata.emplace_back( - default_config_metadata_[ConfigName::REPORT_TRACES]); - } + std::lock_guard lock(mutex_); + reset_config(ConfigName::TRACE_SAMPLING_RATE, trace_sampler_, metadata); + reset_config(ConfigName::TAGS, span_defaults_, metadata); + reset_config(ConfigName::REPORT_TRACES, report_traces_, metadata); - return config_metadata; + return metadata; } nlohmann::json ConfigManager::config_json() const { std::lock_guard lock(mutex_); - return nlohmann::json{{"default", to_json(*span_defaults_.get())}, - {"trace_sampler", trace_sampler_.get()->config_json()}, - {"report_traces", report_traces_.get()}}; + return nlohmann::json{ + {"default", to_json(*span_defaults_.value())}, + {"trace_sampler", trace_sampler_.value()->config_json()}, + {"report_traces", report_traces_.value()}}; } } // namespace tracing diff --git a/src/datadog/config_manager.h b/src/datadog/config_manager.h index c46966a3..39f1afca 100644 --- a/src/datadog/config_manager.h +++ b/src/datadog/config_manager.h @@ -10,6 +10,7 @@ #include "clock.h" #include "config_update.h" #include "json.hpp" +#include "optional.h" #include "span_defaults.h" #include "tracer_config.h" @@ -17,43 +18,50 @@ namespace datadog { namespace tracing { class ConfigManager { + // A template class for managing dynamic configuration values. + // + // This class allows storing and managing dynamic configuration values. It + // maintains an original value and a current value, allowing for updates and + // resets. + // + // Additionally, it provides methods for accessing the current value and + // checking whether it has been modified from its original state. template - class DynamicConf { + class DynamicConfig { T original_value_; - T current_value_; - bool is_original_value_; + Optional current_value_ = nullopt; public: - explicit DynamicConf(T v) - : original_value_(v), current_value_(v), is_original_value_(true) {} + // Constructs a DynamicConf object with the given initial value + explicit DynamicConfig(T original_value) + : original_value_(original_value) {} - void update(T new_v) { - current_value_ = new_v; - is_original_value_ = false; - } - - void reset() { - current_value_ = original_value_; - is_original_value_ = true; - } - - bool is_original_value() const { return is_original_value_; } + // Resets the current value of the configuration to the original value + void reset() { current_value_ = nullopt; } - T get() { return current_value_; } - const T& get() const { return current_value_; } + // Returns whether the current value is the original value + bool is_original_value() const { return !current_value_.has_value(); } - operator T() const { return current_value_; } + const T& value() const { + return current_value_.has_value() ? *current_value_ : original_value_; + } - void operator=(const T& rhs) { update(rhs); } + // Updates the current value of the configuration + void operator=(const T& rhs) { current_value_ = rhs; } }; mutable std::mutex mutex_; Clock clock_; - std::unordered_map default_config_metadata_; + std::unordered_map default_metadata_; + + DynamicConfig> trace_sampler_; + DynamicConfig> span_defaults_; + DynamicConfig report_traces_; - DynamicConf> trace_sampler_; - DynamicConf> span_defaults_; - DynamicConf report_traces_; + private: + template + void reset_config(ConfigName name, T& conf, + std::vector& metadata); public: ConfigManager(const FinalizedTracerConfig& config); diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp new file mode 100644 index 00000000..5c46481e --- /dev/null +++ b/src/datadog/string_util.cpp @@ -0,0 +1,81 @@ +#include "string_util.h" + +#include +#include + +namespace datadog { +namespace tracing { + +std::string to_string(bool b) { return b ? "true" : "false"; } + +std::string to_string(double d, size_t precision) { + std::stringstream stream; + stream << std::fixed << std::setprecision(precision) << d; + return stream.str(); +} + +std::string join(const std::vector& values, + const char* const separator) { + if (values.empty()) return {}; + auto it = values.cbegin(); + + std::string res{*it}; + for (++it; it != values.cend(); ++it) { + res += separator; + append(res, *it); + } + + return res; +} + +std::string join_propagation_styles( + const std::vector& values) { + auto to_string = [](PropagationStyle style) { + switch (style) { + case PropagationStyle::B3: + return "b3"; + case PropagationStyle::DATADOG: + return "datadog"; + case PropagationStyle::W3C: + return "tracecontext"; + case PropagationStyle::NONE: + return "none"; + } + return ""; ///< unlikely + }; + + if (values.empty()) return ""; + auto it = values.cbegin(); + + std::string res{to_string(*it)}; + for (++it; it != values.cend(); ++it) { + res += ','; + res += to_string(*it); + } + + return res; +} + +std::string join_tags( + const std::unordered_map& values) { + if (values.empty()) return {}; + + auto it = values.cbegin(); + + std::string res; + res += it->first; + res += ":"; + res += it->second; + + for (++it; it != values.cend(); ++it) { + res += ","; + res += it->first; + res += ":"; + res += it->second; + } + + return res; +} + +} // namespace tracing +} // namespace datadog diff --git a/src/datadog/string_util.h b/src/datadog/string_util.h new file mode 100644 index 00000000..66a6842b --- /dev/null +++ b/src/datadog/string_util.h @@ -0,0 +1,29 @@ +#pragma once + +#include + +#include "propagation_style.h" + +namespace datadog { +namespace tracing { + +// Converts a boolean value to a string +// The resulted string is either `true` or `false` +std::string to_string(bool b); + +// Converts a double value to a string +std::string to_string(double d, size_t precision); + +// Joins elements of a vector into a single string with a specified separator +std::string join(const std::vector& values, + const char* const separator); + +// Joins propagation styles into a single comma-separated string +std::string join_propagation_styles(const std::vector&); + +// Joins key-value pairs into a single comma-separated string +std::string join_tags( + const std::unordered_map& values); + +} // namespace tracing +} // namespace datadog diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp index ebaad72c..ac4afcd6 100644 --- a/src/datadog/trace_sampler_config.cpp +++ b/src/datadog/trace_sampler_config.cpp @@ -6,6 +6,7 @@ #include "environment.h" #include "json.hpp" #include "parse_util.h" +#include "string_util.h" namespace datadog { namespace tracing { @@ -188,12 +189,12 @@ Expected finalize_config( if (env_config->sample_rate) { sample_rate = env_config->sample_rate; result.metadata[ConfigName::TRACE_SAMPLING_RATE] = ConfigMetadata( - ConfigName::TRACE_SAMPLING_RATE, std::to_string(*sample_rate), + ConfigName::TRACE_SAMPLING_RATE, to_string(*sample_rate, 1), ConfigMetadata::Origin::ENVIRONMENT_VARIABLE); } else if (config.sample_rate) { sample_rate = config.sample_rate; result.metadata[ConfigName::TRACE_SAMPLING_RATE] = ConfigMetadata( - ConfigName::TRACE_SAMPLING_RATE, std::to_string(*sample_rate), + ConfigName::TRACE_SAMPLING_RATE, to_string(*sample_rate, 1), ConfigMetadata::Origin::CODE); } diff --git a/test/test_remote_config.cpp b/test/test_remote_config.cpp index 8b55d1cf..2778f4f6 100644 --- a/test/test_remote_config.cpp +++ b/test/test_remote_config.cpp @@ -212,7 +212,7 @@ REMOTE_CONFIG_TEST("response processing") { CHECK(new_span_defaults != old_span_defaults); CHECK(new_report_traces != old_report_traces); - SECTION("reset confguration") { + SECTION("reset configuration") { SECTION( "missing from client_configs -> all configurations should be reset") { // clang-format off From d8bb3161cd69fe62284cf8e9f1d34b8a118754bc Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Wed, 6 Mar 2024 13:20:44 +0100 Subject: [PATCH 7/9] fix: compilation --- src/datadog/string_util.cpp | 8 ++++---- src/datadog/string_util.h | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp index 5c46481e..b59c5115 100644 --- a/src/datadog/string_util.cpp +++ b/src/datadog/string_util.cpp @@ -57,17 +57,17 @@ std::string join_propagation_styles( } std::string join_tags( - const std::unordered_map& values) { - if (values.empty()) return {}; + const std::unordered_map& tagset) { + if (tagset.empty()) return {}; - auto it = values.cbegin(); + auto it = tagset.cbegin(); std::string res; res += it->first; res += ":"; res += it->second; - for (++it; it != values.cend(); ++it) { + for (++it; it != tagset.cend(); ++it) { res += ","; res += it->first; res += ":"; diff --git a/src/datadog/string_util.h b/src/datadog/string_util.h index 66a6842b..6754f739 100644 --- a/src/datadog/string_util.h +++ b/src/datadog/string_util.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include "propagation_style.h" From 2454fae50ccf5c837c3d56ce31cfd51eddd65292 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Wed, 6 Mar 2024 14:35:08 +0100 Subject: [PATCH 8/9] fix: compilation #2 --- src/datadog/datadog_agent_config.h | 1 + src/datadog/string_util.cpp | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/datadog/datadog_agent_config.h b/src/datadog/datadog_agent_config.h index 6a90fc77..ef4e13f4 100644 --- a/src/datadog/datadog_agent_config.h +++ b/src/datadog/datadog_agent_config.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include "clock.h" diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp index b59c5115..e110ad0c 100644 --- a/src/datadog/string_util.cpp +++ b/src/datadog/string_util.cpp @@ -62,8 +62,7 @@ std::string join_tags( auto it = tagset.cbegin(); - std::string res; - res += it->first; + std::string res{it->first}; res += ":"; res += it->second; From 8861eb69ace0dbcfcc0c8093ca3f160fbee184ab Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Tue, 12 Mar 2024 11:05:53 +0100 Subject: [PATCH 9/9] code review --- src/datadog/config_manager.cpp | 11 +---- src/datadog/config_manager.h | 14 +++--- src/datadog/string_util.cpp | 88 ++++++++++++++++------------------ src/datadog/string_util.h | 7 ++- 4 files changed, 52 insertions(+), 68 deletions(-) diff --git a/src/datadog/config_manager.cpp b/src/datadog/config_manager.cpp index 452887eb..00b11b4f 100644 --- a/src/datadog/config_manager.cpp +++ b/src/datadog/config_manager.cpp @@ -107,16 +107,7 @@ void ConfigManager::reset_config(ConfigName name, T& conf, metadata.emplace_back(default_metadata_[name]); } -std::vector ConfigManager::reset() { - std::vector metadata; - - std::lock_guard lock(mutex_); - reset_config(ConfigName::TRACE_SAMPLING_RATE, trace_sampler_, metadata); - reset_config(ConfigName::TAGS, span_defaults_, metadata); - reset_config(ConfigName::REPORT_TRACES, report_traces_, metadata); - - return metadata; -} +std::vector ConfigManager::reset() { return update({}); } nlohmann::json ConfigManager::config_json() const { std::lock_guard lock(mutex_); diff --git a/src/datadog/config_manager.h b/src/datadog/config_manager.h index 39f1afca..190cb9cb 100644 --- a/src/datadog/config_manager.h +++ b/src/datadog/config_manager.h @@ -18,7 +18,7 @@ namespace datadog { namespace tracing { class ConfigManager { - // A template class for managing dynamic configuration values. + // A class template for managing dynamic configuration values. // // This class allows storing and managing dynamic configuration values. It // maintains an original value and a current value, allowing for updates and @@ -26,14 +26,14 @@ class ConfigManager { // // Additionally, it provides methods for accessing the current value and // checking whether it has been modified from its original state. - template + template class DynamicConfig { - T original_value_; - Optional current_value_ = nullopt; + Value original_value_; + Optional current_value_; public: // Constructs a DynamicConf object with the given initial value - explicit DynamicConfig(T original_value) + explicit DynamicConfig(Value original_value) : original_value_(original_value) {} // Resets the current value of the configuration to the original value @@ -42,12 +42,12 @@ class ConfigManager { // Returns whether the current value is the original value bool is_original_value() const { return !current_value_.has_value(); } - const T& value() const { + const Value& value() const { return current_value_.has_value() ? *current_value_ : original_value_; } // Updates the current value of the configuration - void operator=(const T& rhs) { current_value_ = rhs; } + void operator=(const Value& rhs) { current_value_ = rhs; } }; mutable std::mutex mutex_; diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp index e110ad0c..9384c81f 100644 --- a/src/datadog/string_util.cpp +++ b/src/datadog/string_util.cpp @@ -5,6 +5,26 @@ namespace datadog { namespace tracing { +namespace { + +template +std::string join(const Sequence& elements, StringView separator, + Func&& append_element) { + auto iter = std::begin(elements); + const auto end = std::end(elements); + std::string result; + if (iter == end) { + return result; + } + append_element(result, *iter); + for (++iter; iter != end; ++iter) { + append(result, separator); + append_element(result, *iter); + } + return result; +} + +} // namespace std::string to_string(bool b) { return b ? "true" : "false"; } @@ -14,66 +34,40 @@ std::string to_string(double d, size_t precision) { return stream.str(); } -std::string join(const std::vector& values, - const char* const separator) { - if (values.empty()) return {}; - auto it = values.cbegin(); - - std::string res{*it}; - for (++it; it != values.cend(); ++it) { - res += separator; - append(res, *it); - } - - return res; +std::string join(const std::vector& values, StringView separator) { + return join(values, separator, [](std::string& result, StringView value) { + append(result, value); + }); } std::string join_propagation_styles( const std::vector& values) { - auto to_string = [](PropagationStyle style) { + return join(values, ",", [](std::string& result, PropagationStyle style) { switch (style) { case PropagationStyle::B3: - return "b3"; + result += "b3"; + break; case PropagationStyle::DATADOG: - return "datadog"; + result += "datadog"; + break; case PropagationStyle::W3C: - return "tracecontext"; + result += "tracecontext"; + break; case PropagationStyle::NONE: - return "none"; + result += "none"; + break; } - return ""; ///< unlikely - }; - - if (values.empty()) return ""; - auto it = values.cbegin(); - - std::string res{to_string(*it)}; - for (++it; it != values.cend(); ++it) { - res += ','; - res += to_string(*it); - } - - return res; + }); } std::string join_tags( - const std::unordered_map& tagset) { - if (tagset.empty()) return {}; - - auto it = tagset.cbegin(); - - std::string res{it->first}; - res += ":"; - res += it->second; - - for (++it; it != tagset.cend(); ++it) { - res += ","; - res += it->first; - res += ":"; - res += it->second; - } - - return res; + const std::unordered_map& values) { + return join(values, ",", [](std::string& result, const auto& entry) { + const auto& [key, value] = entry; + result += key; + result += ':'; + result += value; + }); } } // namespace tracing diff --git a/src/datadog/string_util.h b/src/datadog/string_util.h index 6754f739..255a4b3c 100644 --- a/src/datadog/string_util.h +++ b/src/datadog/string_util.h @@ -8,16 +8,15 @@ namespace datadog { namespace tracing { -// Converts a boolean value to a string -// The resulted string is either `true` or `false` +// Return a string representation of the specified boolean `value`. +// The result is "true" for `true` and "false" for `false`. std::string to_string(bool b); // Converts a double value to a string std::string to_string(double d, size_t precision); // Joins elements of a vector into a single string with a specified separator -std::string join(const std::vector& values, - const char* const separator); +std::string join(const std::vector& values, StringView separator); // Joins propagation styles into a single comma-separated string std::string join_propagation_styles(const std::vector&);