diff --git a/appsec/src/helper/client.cpp b/appsec/src/helper/client.cpp index 217ed193f8..c7a62090e8 100644 --- a/appsec/src/helper/client.cpp +++ b/appsec/src/helper/client.cpp @@ -506,12 +506,13 @@ struct request_metrics_submitter : public metrics::telemetry_submitter { request_metrics_submitter(request_metrics_submitter &&) = delete; request_metrics_submitter &operator=(request_metrics_submitter &&) = delete; - void submit_metric( - std::string_view name, double value, std::string tags) override + void submit_metric(std::string_view name, double value, + metrics::telemetry_tags tags) override { + std::string tags_s = tags.consume(); SPDLOG_TRACE("submit_metric [req]: name={}, value={}, tags={}", name, - value, tags); - tel_metrics[name].emplace_back(value, tags); + value, tags_s); + tel_metrics[name].emplace_back(value, std::move(tags_s)); }; void submit_span_metric(std::string_view name, double value) override { @@ -547,7 +548,7 @@ void collect_metrics_impl(Response &response, service &service, context->get_metrics(msubmitter); } service.drain_metrics( - [&msubmitter](std::string_view name, double value, std::string tags) { + [&msubmitter](std::string_view name, double value, auto tags) { msubmitter.submit_metric(name, value, std::move(tags)); }); msubmitter.metrics.merge(service.drain_legacy_metrics()); diff --git a/appsec/src/helper/metrics.hpp b/appsec/src/helper/metrics.hpp index 662720b448..31585edeb0 100644 --- a/appsec/src/helper/metrics.hpp +++ b/appsec/src/helper/metrics.hpp @@ -6,11 +6,53 @@ #pragma once +#include #include #include namespace dds::metrics { +class telemetry_tags { +public: + telemetry_tags &add(std::string_view key, std::string_view value) + { + data_.reserve(data_.size() + key.size() + value.size() + 2); + if (!data_.empty()) { + data_ += ','; + } + data_ += key; + data_ += ':'; + data_ += value; + return *this; + } + std::string consume() { return std::move(data_); } + + // the rest of the methods are for testing + static telemetry_tags from_string(std::string str) + { + telemetry_tags tags; + tags.data_ = std::move(str); + return tags; + } + + bool operator==(const telemetry_tags &other) const + { + return data_ == other.data_; + } + + friend std::ostream &operator<<( + std::ostream &os, const telemetry_tags &tags) + { + os << tags.data_; + return os; + } + +private: + std::string data_; + + friend struct fmt::formatter; +}; + struct telemetry_submitter { telemetry_submitter() = default; telemetry_submitter(const telemetry_submitter &) = delete; @@ -20,7 +62,7 @@ struct telemetry_submitter { virtual ~telemetry_submitter() = 0; // first arguments of type string_view should have static storage - virtual void submit_metric(std::string_view, double, std::string) = 0; + virtual void submit_metric(std::string_view, double, telemetry_tags) = 0; virtual void submit_span_metric(std::string_view, double) = 0; virtual void submit_span_meta(std::string_view, std::string) = 0; void submit_span_meta(std::string, std::string) = delete; @@ -57,3 +99,15 @@ constexpr std::string_view waf_version = "_dd.appsec.waf.version"; constexpr std::string_view waf_duration = "_dd.appsec.waf.duration"; } // namespace dds::metrics + +template <> +struct fmt::formatter + : fmt::formatter { + + auto format( + const dds::metrics::telemetry_tags tags, format_context &ctx) const + { + return formatter::format( + std::string_view{tags.data_}, ctx); + } +}; diff --git a/appsec/src/helper/service.hpp b/appsec/src/helper/service.hpp index 48c53c38ea..198ad6c752 100644 --- a/appsec/src/helper/service.hpp +++ b/appsec/src/helper/service.hpp @@ -27,12 +27,13 @@ class service { protected: class metrics_impl : public metrics::telemetry_submitter { struct tel_metric { - tel_metric(std::string_view name, double value, std::string tags) + tel_metric(std::string_view name, double value, + metrics::telemetry_tags tags) : name{name}, value{value}, tags{std::move(tags)} {} std::string_view name; double value; - std::string tags; + metrics::telemetry_tags tags; }; public: @@ -45,7 +46,7 @@ class service { ~metrics_impl() override = default; void submit_metric(std::string_view metric_name, double value, - std::string tags) override + metrics::telemetry_tags tags) override { SPDLOG_TRACE("submit_metric: {} {} {}", metric_name, value, tags); const std::lock_guard lock{pending_metrics_mutex_}; @@ -184,7 +185,7 @@ class service { { if (client_handler_ && !client_handler_->has_applied_rc()) { msubmitter_->submit_metric( - "remote_config.requests_before_running"sv, 1, ""); + "remote_config.requests_before_running"sv, 1, {}); } } diff --git a/appsec/src/helper/subscriber/waf.cpp b/appsec/src/helper/subscriber/waf.cpp index 13ec23466e..c8526f341b 100644 --- a/appsec/src/helper/subscriber/waf.cpp +++ b/appsec/src/helper/subscriber/waf.cpp @@ -124,21 +124,19 @@ void log_cb(DDWAF_LOG_LEVEL level, const char *function, const char *file, std::string_view(message, message_len)); } -std::string waf_update_init_report_tags( +metrics::telemetry_tags waf_update_init_report_tags( bool success, std::optional rules_version) { - std::string tags{"success"sv}; + metrics::telemetry_tags tags; if (success) { - tags += ":true"sv; + tags.add("success", "true"); } else { - tags += ":false"sv; + tags.add("success", "false"); } if (rules_version) { - tags += ",event_rules_version:"sv; - tags += rules_version.value(); + tags.add("event_rules_version", rules_version.value()); } - tags += ",waf_version:"sv; - tags += ddwaf_get_version(); + tags.add("waf_version", ddwaf_get_version()); return tags; } @@ -191,18 +189,16 @@ void load_result_report( // map has either "error" key or "loaded", "failed", "errors" keys const parameter_view::map map = static_cast(value); - auto tags_prefix = [&]() { - std::string tags{"waf_version:"}; - tags += ddwaf_get_version(); - tags += ",event_rules_version:"; - tags += ruleset_version; - tags += ",config_key:"; - tags += k; + auto tags_common = [&]() { + metrics::telemetry_tags tags; + tags.add("waf_version", ddwaf_get_version()) + .add("event_rules_version", ruleset_version) + .add("config_key", k); return tags; }; if (map.contains("error")) { - std::string tags{tags_prefix()}; - tags += ",scope:top-level"; + metrics::telemetry_tags tags{tags_common()}; + tags.add("scope", "top-level"); msubmitter.submit_metric( metrics::waf_config_errors, 1.0, std::move(tags)); @@ -218,8 +214,8 @@ void load_result_report( error_count += err.size(); } - std::string tags{tags_prefix()}; - tags += ",scope:item"; + metrics::telemetry_tags tags{tags_common()}; + tags.add("scope", "item"); msubmitter.submit_metric(metrics::waf_config_errors, static_cast(error_count), std::move(tags)); @@ -279,10 +275,8 @@ instance::listener::listener(ddwaf_context ctx, : handle_{ctx}, waf_timeout_{waf_timeout}, ruleset_version_{std::move(ruleset_version)} { - base_tags_ += "event_rules_version:"sv; - base_tags_ += ruleset_version_; - base_tags_ += ",waf_version:"sv; - base_tags_ += ddwaf_get_version(); + base_tags_.add("event_rules_version", ruleset_version_); + base_tags_.add("waf_version", ddwaf_get_version()); } instance::listener::listener(instance::listener &&other) noexcept @@ -384,18 +378,18 @@ void instance::listener::call(dds::parameter_view &data, event &event) void instance::listener::submit_metrics( metrics::telemetry_submitter &msubmitter) { - std::string tags = base_tags_; + metrics::telemetry_tags tags = base_tags_; if (rule_triggered_) { - tags += ",rule_triggered:true"sv; + tags.add("rule_triggered", "true"); } if (request_blocked_) { - tags += ",request_blocked:true"sv; + tags.add("request_blocked", "true"); } if (waf_hit_timeout_) { - tags += ",waf_timeout:true"sv; + tags.add("waf_timeout", "true"); } if (waf_run_error_) { - tags += ",waf_run_error:true"sv; + tags.add("waf_run_error", "true"); } msubmitter.submit_metric(metrics::waf_requests, 1.0, std::move(tags)); diff --git a/appsec/src/helper/subscriber/waf.hpp b/appsec/src/helper/subscriber/waf.hpp index ff952c20e2..45890a995e 100644 --- a/appsec/src/helper/subscriber/waf.hpp +++ b/appsec/src/helper/subscriber/waf.hpp @@ -47,7 +47,7 @@ class instance : public dds::subscriber { double total_runtime_{0.0}; std::string ruleset_version_; std::map schemas_; - std::string base_tags_; + metrics::telemetry_tags base_tags_; bool rule_triggered_{}; bool request_blocked_{}; bool waf_hit_timeout_{}; diff --git a/appsec/tests/helper/engine_test.cpp b/appsec/tests/helper/engine_test.cpp index 4caef43ca6..395637a960 100644 --- a/appsec/tests/helper/engine_test.cpp +++ b/appsec/tests/helper/engine_test.cpp @@ -39,8 +39,8 @@ class subscriber : public dds::subscriber { class tel_submitter : public metrics::telemetry_submitter { public: - MOCK_METHOD(void, submit_metric, (std::string_view, double, std::string), - (override)); + MOCK_METHOD(void, submit_metric, + (std::string_view, double, metrics::telemetry_tags), (override)); MOCK_METHOD( void, submit_span_metric, (std::string_view, double), (override)); MOCK_METHOD( @@ -538,8 +538,10 @@ TEST(EngineTest, WafSubscriptorUpdateRuleData) msubmitter, submit_span_meta("_dd.appsec.waf.version"sv, _)); EXPECT_CALL(msubmitter, submit_metric("waf.updates"sv, 1, - std::string{"success:true,event_rules_version:,waf_version:"} + - ddwaf_get_version())); + metrics::telemetry_tags::from_string( + std::string{ + "success:true,event_rules_version:,waf_version:"} + + ddwaf_get_version()))); engine_ruleset rule_data( R"({"rules_data":[{"id":"blocked_ips","type":"data_with_expiration","data":[{"value":"192.168.1.1","expiration":"9999999999"}]}]})"); @@ -565,8 +567,10 @@ TEST(EngineTest, WafSubscriptorUpdateRuleData) msubmitter, submit_span_meta("_dd.appsec.waf.version"sv, _)); EXPECT_CALL(msubmitter, submit_metric("waf.updates"sv, 1, - std::string{"success:true,event_rules_version:,waf_version:"} + - ddwaf_get_version())); + metrics::telemetry_tags::from_string( + std::string{ + "success:true,event_rules_version:,waf_version:"} + + ddwaf_get_version()))); engine_ruleset rule_data( R"({"rules_data":[{"id":"blocked_ips","type":"data_with_expiration","data":[{"value":"192.168.1.2","expiration":"9999999999"}]}]})"); @@ -606,8 +610,10 @@ TEST(EngineTest, WafSubscriptorInvalidRuleData) { EXPECT_CALL(submitter, submit_metric("waf.updates"sv, 1, - std::string{"success:false,event_rules_version:,waf_version:"} + - ddwaf_get_version())); + metrics::telemetry_tags::from_string( + std::string{ + "success:false,event_rules_version:,waf_version:"} + + ddwaf_get_version()))); engine_ruleset rule_data( R"({"id":"blocked_ips","type":"data_with_expiration","data":[{"value":"192.168.1.1","expiration":"9999999999"}]})"); diff --git a/appsec/tests/helper/tel_subm_mock.hpp b/appsec/tests/helper/tel_subm_mock.hpp index b34e180d0b..712a56bc99 100644 --- a/appsec/tests/helper/tel_subm_mock.hpp +++ b/appsec/tests/helper/tel_subm_mock.hpp @@ -4,8 +4,8 @@ namespace dds::mock { class tel_submitter : public dds::metrics::telemetry_submitter { public: - MOCK_METHOD(void, submit_metric, (std::string_view, double, std::string), - (override)); + MOCK_METHOD(void, submit_metric, + (std::string_view, double, dds::metrics::telemetry_tags), (override)); MOCK_METHOD( void, submit_span_metric, (std::string_view, double), (override)); MOCK_METHOD( diff --git a/appsec/tests/helper/waf_test.cpp b/appsec/tests/helper/waf_test.cpp index 787bceb2ac..92dae805ea 100644 --- a/appsec/tests/helper/waf_test.cpp +++ b/appsec/tests/helper/waf_test.cpp @@ -56,10 +56,12 @@ TEST(WafTest, InitWithInvalidRules) EXPECT_CALL(submitm, submit_span_metric(metrics::event_rules_failed, 4.0)); EXPECT_CALL(submitm, submit_metric("waf.init"sv, 1, _)); - EXPECT_CALL(submitm, submit_metric("waf.config_errors", 4., - std::string{"waf_version:"} + ddwaf_get_version() + - ",event_rules_version:1.2.3," - "config_key:rules,scope:item")); + EXPECT_CALL( + submitm, submit_metric("waf.config_errors", 4., + metrics::telemetry_tags::from_string( + std::string{"waf_version:"} + ddwaf_get_version() + + ",event_rules_version:1.2.3," + "config_key:rules,scope:item"))); std::shared_ptr wi{ waf::instance::from_settings(cs, ruleset, submitm)}; @@ -124,8 +126,9 @@ TEST(WafTest, ValidRunGood) .WillOnce(SaveArg<1>(&duration)); EXPECT_CALL( submitm, submit_metric("waf.requests"sv, 1, - std::string{"event_rules_version:1.2.3,waf_version:"} + - ddwaf_get_version())); + metrics::telemetry_tags::from_string( + std::string{"event_rules_version:1.2.3,waf_version:"} + + ddwaf_get_version()))); ctx->submit_metrics(submitm); EXPECT_GT(duration, 0.0); Mock::VerifyAndClearExpectations(&submitm); @@ -160,8 +163,9 @@ TEST(WafTest, ValidRunMonitor) EXPECT_CALL(submitm, submit_span_metric(metrics::waf_duration, _)); EXPECT_CALL( submitm, submit_metric("waf.requests"sv, 1, - std::string{"event_rules_version:1.2.3,waf_version:"} + - ddwaf_get_version() + ",rule_triggered:true")); + metrics::telemetry_tags::from_string( + std::string{"event_rules_version:1.2.3,waf_version:"} + + ddwaf_get_version() + ",rule_triggered:true"))); EXPECT_CALL( submitm, submit_span_meta_copy_key( std::string{"_dd.appsec.s.arg2"}, std::string{"[8]"})); @@ -314,8 +318,9 @@ TEST(WafTest, UpdateInvalid) EXPECT_CALL(submitm, submit_metric("waf.updates"sv, 1, - std::string{"success:false,event_rules_version:,waf_version:"} + - ddwaf_get_version())); + metrics::telemetry_tags::from_string( + std::string{"success:false,event_rules_version:,waf_version:"} + + ddwaf_get_version()))); ASSERT_THROW(wi->update(param, submitm), invalid_object); } @@ -344,8 +349,9 @@ TEST(WafTest, SchemasAreAdded) EXPECT_CALL( submitm, submit_metric("waf.requests"sv, 1, - std::string{"event_rules_version:1.2.3,waf_version:"} + - ddwaf_get_version() + ",rule_triggered:true")); + metrics::telemetry_tags::from_string( + std::string{"event_rules_version:1.2.3,waf_version:"} + + ddwaf_get_version() + ",rule_triggered:true"))); EXPECT_CALL( submitm, submit_span_meta("_dd.appsec.event_rules.version", "1.2.3")); EXPECT_CALL(submitm, submit_span_metric("_dd.appsec.waf.duration"sv, _));