From c4958c791982ad5411f2a12627b6f9da32f34fb6 Mon Sep 17 00:00:00 2001 From: Damien Mehala Date: Fri, 29 Mar 2024 15:31:18 -0400 Subject: [PATCH] feat: report Remote Configuration status (#108) - Fix Remote Configuration payload. - Report configuration status. - Update Remote Configuration tests. - Strengthen Remote Configuration parsing for increased resilience. - Implement scheduling of telemetry and remote configuration requests by the system-tests server. - Fix `trim` implementation. --- src/datadog/remote_config.cpp | 76 +++++++++++++++++----------- src/datadog/remote_config.h | 8 +++ src/datadog/string_util.cpp | 7 ++- test/system-tests/manual_scheduler.h | 14 ++--- test/test_remote_config.cpp | 29 ++++++++++- 5 files changed, 95 insertions(+), 39 deletions(-) diff --git a/src/datadog/remote_config.cpp b/src/datadog/remote_config.cpp index 08ce8c6a..e26084d2 100644 --- a/src/datadog/remote_config.cpp +++ b/src/datadog/remote_config.cpp @@ -55,19 +55,18 @@ 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()) { + sampling_rate_it != j.cend() && sampling_rate_it->is_number()) { config_update.trace_sampling_rate = *sampling_rate_it; } - if (auto tags_it = j.find("tracing_tags"); tags_it != j.cend()) { + if (auto tags_it = j.find("tracing_tags"); + tags_it != j.cend() && tags_it->is_array()) { 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(); - } + tracing_enabled_it != j.cend() && tracing_enabled_it->is_boolean()) { + config_update.report_traces = tracing_enabled_it->get(); } return config_update; @@ -120,17 +119,26 @@ nlohmann::json RemoteConfigurationManager::make_request_payload() { if (!applied_config_.empty()) { auto config_states = nlohmann::json::array(); for (const auto& [_, config] : applied_config_) { - config_states.emplace_back(nlohmann::json{{"id", config.id}, - {"version", config.version}, - {"product", k_apm_product}}); + nlohmann::json config_state = { + {"id", config.id}, + {"version", config.version}, + {"product", k_apm_product}, + {"apply_state", config.state}, + }; + + if (config.error_message) { + config_state["apply_error"] = *config.error_message; + } + + config_states.emplace_back(std::move(config_state)); } - j["config_states"] = config_states; + j["client"]["state"]["config_states"] = config_states; } if (state_.error_message) { - j["has_error"] = true; - j["error"] = *state_.error_message; + j["client"]["state"]["has_error"] = true; + j["client"]["state"]["error"] = *state_.error_message; } return j; @@ -138,8 +146,8 @@ nlohmann::json RemoteConfigurationManager::make_request_payload() { std::vector RemoteConfigurationManager::process_response( const nlohmann::json& json) { - std::vector config_update; - config_update.reserve(8); + std::vector config_updates; + config_updates.reserve(8); state_.error_message = nullopt; @@ -158,12 +166,14 @@ std::vector RemoteConfigurationManager::process_response( if (client_configs_it == json.cend()) { if (!applied_config_.empty()) { std::for_each(applied_config_.cbegin(), applied_config_.cend(), - [this, &config_update](const auto it) { - config_update = revert_config(it.second); + [this, &config_updates](const auto it) { + auto updated = revert_config(it.second); + config_updates.insert(config_updates.end(), + updated.begin(), updated.end()); }); applied_config_.clear(); } - return config_update; + return config_updates; } // Keep track of config path received to know which ones to revert. @@ -194,34 +204,42 @@ std::vector RemoteConfigurationManager::process_response( "target file having path \""; append(*state_.error_message, config_path); *state_.error_message += '\"'; - return config_update; + return config_updates; } const auto config_json = nlohmann::json::parse( base64_decode(target_it.value().at("raw").get())); + Configuration new_config; + new_config.id = config_json.at("id"); + new_config.hash = config_metadata.at("/hashes/sha256"_json_pointer); + new_config.version = config_json.at("revision"); + const auto& targeted_service = config_json.at("service_target"); if (targeted_service.at("service").get() != tracer_signature_.default_service || targeted_service.at("env").get() != tracer_signature_.default_environment) { - continue; - } + new_config.state = Configuration::State::error; + new_config.error_message = "Wrong service targeted"; + } else { + new_config.state = Configuration::State::acknowledged; + new_config.content = parse_dynamic_config(config_json.at("lib_config")); - Configuration new_config; - new_config.hash = config_metadata.at("/hashes/sha256"_json_pointer); - new_config.id = config_json.at("id"); - new_config.version = config_json.at("revision"); - new_config.content = parse_dynamic_config(config_json.at("lib_config")); + auto updated = apply_config(new_config); + config_updates.insert(config_updates.end(), updated.begin(), + updated.end()); + } - 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)) { - config_update = revert_config(it->second); + auto updated = revert_config(it->second); + config_updates.insert(config_updates.end(), updated.begin(), + updated.end()); it = applied_config_.erase(it); } else { it++; @@ -232,10 +250,10 @@ std::vector RemoteConfigurationManager::process_response( error_message += e.what(); state_.error_message = std::move(error_message); - return config_update; + return config_updates; } - return config_update; + return config_updates; } std::vector RemoteConfigurationManager::apply_config( diff --git a/src/datadog/remote_config.h b/src/datadog/remote_config.h index 6d8310e1..d822fe58 100644 --- a/src/datadog/remote_config.h +++ b/src/datadog/remote_config.h @@ -42,6 +42,14 @@ class RemoteConfigurationManager { std::string hash; std::size_t version; ConfigUpdate content; + + enum State : char { + unacknowledged = 1, + acknowledged = 2, + error = 3 + } state = State::unacknowledged; + + Optional error_message; }; TracerSignature tracer_signature_; diff --git a/src/datadog/string_util.cpp b/src/datadog/string_util.cpp index ac93c504..8ff173d5 100644 --- a/src/datadog/string_util.cpp +++ b/src/datadog/string_util.cpp @@ -8,6 +8,8 @@ namespace datadog { namespace tracing { namespace { +constexpr StringView k_spaces_characters = " \f\n\r\t\v"; + template std::string join(const Sequence& elements, StringView separator, Func&& append_element) { @@ -86,8 +88,9 @@ bool starts_with(StringView subject, StringView prefix) { } StringView trim(StringView str) { - str.remove_prefix(std::min(str.find_first_not_of(' '), str.size())); - const auto pos = str.find_last_not_of(' '); + str.remove_prefix( + std::min(str.find_first_not_of(k_spaces_characters), str.size())); + const auto pos = str.find_last_not_of(k_spaces_characters); if (pos != str.npos) str.remove_suffix(str.size() - pos - 1); return str; } diff --git a/test/system-tests/manual_scheduler.h b/test/system-tests/manual_scheduler.h index 811f484f..a01bfabc 100644 --- a/test/system-tests/manual_scheduler.h +++ b/test/system-tests/manual_scheduler.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -8,23 +9,24 @@ struct ManualScheduler : public datadog::tracing::EventScheduler { std::function flush_traces = nullptr; std::function flush_telemetry = nullptr; + datadog::tracing::ThreadedEventScheduler scheduler_; - Cancel schedule_recurring_event( - std::chrono::steady_clock::duration /* interval */, - std::function callback) override { + Cancel schedule_recurring_event(std::chrono::steady_clock::duration interval, + std::function callback) override { assert(callback != nullptr); // NOTE: This depends on the precise order that dd-trace-cpp sets up the // `schedule_recurring_event`s for traces and telemetry. if (flush_traces == nullptr) { flush_traces = callback; - return {}; + return []() {}; } + if (flush_telemetry == nullptr) { flush_telemetry = callback; - return {}; } - return []() {}; + + return scheduler_.schedule_recurring_event(interval, callback); } nlohmann::json config_json() const override { diff --git a/test/test_remote_config.cpp b/test/test_remote_config.cpp index 2778f4f6..6c59fc59 100644 --- a/test/test_remote_config.cpp +++ b/test/test_remote_config.cpp @@ -156,8 +156,8 @@ REMOTE_CONFIG_TEST("response processing") { // Next payload should contains an error. const auto payload = rc.make_request_payload(); - CHECK(payload.contains("error") == true); - CHECK(payload.contains("has_error") == true); + CHECK(payload.contains("/client/state/has_error"_json_pointer) == true); + CHECK(payload.contains("/client/state/error"_json_pointer) == true); } SECTION("valid remote configuration") { @@ -212,6 +212,19 @@ REMOTE_CONFIG_TEST("response processing") { CHECK(new_span_defaults != old_span_defaults); CHECK(new_report_traces != old_report_traces); + SECTION("config status is correctly applied") { + const auto payload = rc.make_request_payload(); + const auto s = payload.dump(2); + REQUIRE(payload.contains("/client/state/config_states"_json_pointer) == + true); + + const auto& config_states = + payload.at("/client/state/config_states"_json_pointer); + REQUIRE(config_states.size() == 1); + CHECK(config_states[0]["product"] == "APM_TRACING"); + CHECK(config_states[0]["apply_state"] == 2); + } + SECTION("reset configuration") { SECTION( "missing from client_configs -> all configurations should be reset") { @@ -315,5 +328,17 @@ REMOTE_CONFIG_TEST("response processing") { CHECK(config_updated.empty()); CHECK(new_sampling_rate == old_sampling_rate); + + // Verify next request set the config status + const auto payload = rc.make_request_payload(); + REQUIRE(payload.contains("/client/state/config_states"_json_pointer) == + true); + + const auto& config_states = + payload.at("/client/state/config_states"_json_pointer); + REQUIRE(config_states.size() == 1); + CHECK(config_states[0]["product"] == "APM_TRACING"); + CHECK(config_states[0]["apply_state"] == 3); + CHECK(config_states[0].contains("apply_state")); } }