appsec helper: abstract tags format

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());

appsec/src/helper/metrics.hpp

@@ -6,11 +6,53 @@
 
 #pragma once
 
+#include <spdlog/spdlog.h>
 #include <string>
 #include <string_view>
 
 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<telemetry_tags>;
+};
+
 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<dds::metrics::telemetry_tags>
+    : fmt::formatter<std::string_view> {
+
+    auto format(
+        const dds::metrics::telemetry_tags tags, format_context &ctx) const
+    {
+        return formatter<std::string_view>::format(
+            std::string_view{tags.data_}, ctx);
+    }
+};

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<std::mutex> 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, {});
         }
     }
 

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<std::string> 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<parameter_view::map>(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<double>(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));
 

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<std::string, std::string> schemas_;
-        std::string base_tags_;
+        metrics::telemetry_tags base_tags_;
         bool rule_triggered_{};
         bool request_blocked_{};
         bool waf_hit_timeout_{};

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"}]})");

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(

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<subscriber> 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, _));