Address PR comments

Author: cataphract

appsec/src/extension/commands_helpers.c

@@ -749,10 +749,9 @@ void _handle_telemetry_metric(const char *nonnull key_str, size_t key_len,
 {
 #define HANDLE_METRIC(name, type)                                              \
     do {                                                                       \
-        if (key_len == sizeof(name "") - 1 &&                                  \
-            memcmp(key_str, name, key_len) == 0) {                             \
+        if (key_len == LSTRLEN(name) && memcmp(key_str, name, key_len) == 0) { \
             static zend_string *_Atomic key_zstr;                              \
-            _init_zstr(&key_zstr, name, sizeof(name) - 1);                     \
+            _init_zstr(&key_zstr, name, LSTRLEN(name));                        \
             zend_string *tags_zstr = zend_string_init(tags_str, tags_len, 1);  \
             ddtrace_metric_register_buffer(                                    \
                 key_zstr, type, DDTRACE_METRIC_NAMESPACE_APPSEC);              \

appsec/src/extension/request_abort.c

@@ -12,7 +12,6 @@
 #include "compatibility.h"
 #include "configuration.h"
 #include "ddappsec.h"
-#include "dddefs.h"
 #include "ddtrace.h"
 #include "logging.h"
 #include "php_compat.h"

appsec/src/helper/.clang-tidy

@@ -0,0 +1,13 @@
+Checks: 'readability-identifier-naming'
+
+CheckOptions:
+  - key: readability-identifier-naming.StructCase
+    value: lower_case
+  - key: readability-identifier-naming.StructPrefix
+    value: ''
+  - key: readability-identifier-naming.ClassCase
+    value: lower_case
+  - key: readability-identifier-naming.ClassPrefix
+    value: ''
+
+InheritParentConfig: true

appsec/src/helper/client.cpp

@@ -497,14 +497,14 @@ void client::run(worker::queue_consumer &q)
 
 namespace {
 
-struct RequestMetricsSubmitter : public metrics::TelemetrySubmitter {
-    RequestMetricsSubmitter() = default;
-    ~RequestMetricsSubmitter() override = default;
-    RequestMetricsSubmitter(const RequestMetricsSubmitter &) = delete;
-    RequestMetricsSubmitter &operator=(
-        const RequestMetricsSubmitter &) = delete;
-    RequestMetricsSubmitter(RequestMetricsSubmitter &&) = delete;
-    RequestMetricsSubmitter &operator=(RequestMetricsSubmitter &&) = delete;
+struct request_metrics_submitter : public metrics::telemetry_submitter {
+    request_metrics_submitter() = default;
+    ~request_metrics_submitter() override = default;
+    request_metrics_submitter(const request_metrics_submitter &) = delete;
+    request_metrics_submitter &operator=(
+        const request_metrics_submitter &) = delete;
+    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
@@ -513,37 +513,36 @@ struct RequestMetricsSubmitter : public metrics::TelemetrySubmitter {
             value, tags);
         tel_metrics[name].emplace_back(value, tags);
     };
-    void submit_legacy_metric(std::string_view name, double value) override
+    void submit_span_metric(std::string_view name, double value) override
     {
         SPDLOG_TRACE(
-            "submit_legacy_metric [req]: name={}, value={}", name, value);
+            "submit_span_metric [req]: name={}, value={}", name, value);
         metrics[name] = value;
     };
-    void submit_legacy_meta(std::string_view name, std::string value) override
+    void submit_span_meta(std::string_view name, std::string value) override
     {
-        SPDLOG_TRACE(
-            "submit_legacy_meta [req]: name={}, value={}", name, value);
+        SPDLOG_TRACE("submit_span_meta [req]: name={}, value={}", name, value);
         meta[std::string{name}] = value;
     };
-    void submit_legacy_meta_copy_key(
-        std::string name, std::string value) override
+    void submit_span_meta_copy_key(std::string name, std::string value) override
     {
-        SPDLOG_TRACE("submit_legacy_meta_copy_key [req]: name={}, value={}",
-            name, value);
+        SPDLOG_TRACE(
+            "submit_span_meta_copy_key [req]: name={}, value={}", name, value);
         meta[name] = value;
     }
 
     std::map<std::string, std::string> meta;
     std::map<std::string_view, double> metrics;
-    std::map<std::string_view, std::vector<std::pair<double, std::string>>>
+    std::unordered_map<std::string_view,
+        std::vector<std::pair<double, std::string>>>
         tel_metrics;
 };
 
 template <typename Response>
 void collect_metrics_impl(Response &response, service &service,
     std::optional<engine::context> &context)
 {
-    RequestMetricsSubmitter msubmitter{};
+    request_metrics_submitter msubmitter{};
     if (context) {
         context->get_metrics(msubmitter);
     }

appsec/src/helper/engine.cpp

@@ -25,7 +25,7 @@ void engine::subscribe(std::unique_ptr<subscriber> sub)
 }
 
 void engine::update(
-    engine_ruleset &ruleset, metrics::TelemetrySubmitter &submit_metric)
+    engine_ruleset &ruleset, metrics::telemetry_submitter &submit_metric)
 {
     std::vector<std::unique_ptr<subscriber>> new_subscribers;
     auto old_common =
@@ -114,7 +114,7 @@ std::optional<engine::result> engine::context::publish(parameter &&param)
     return res;
 }
 
-void engine::context::get_metrics(metrics::TelemetrySubmitter &msubmitter)
+void engine::context::get_metrics(metrics::telemetry_submitter &msubmitter)
 {
     for (const auto &[subscriber, listener] : listeners_) {
         listener->submit_metrics(msubmitter);
@@ -123,7 +123,7 @@ void engine::context::get_metrics(metrics::TelemetrySubmitter &msubmitter)
 
 std::unique_ptr<engine> engine::from_settings(
     const dds::engine_settings &eng_settings,
-    metrics::TelemetrySubmitter &msubmitter)
+    metrics::telemetry_submitter &msubmitter)
 {
     auto &&rules_path = eng_settings.rules_file_or_default();
     auto ruleset = engine_ruleset::from_path(rules_path);

appsec/src/helper/engine.hpp

@@ -68,7 +68,7 @@ class engine {
 
         std::optional<result> publish(parameter &&param);
         // NOLINTNEXTLINE(google-runtime-references)
-        void get_metrics(metrics::TelemetrySubmitter &msubmitter);
+        void get_metrics(metrics::telemetry_submitter &msubmitter);
 
     protected:
         std::shared_ptr<shared_state> common_;
@@ -87,7 +87,7 @@ class engine {
 
     static std::unique_ptr<engine> from_settings(
         const dds::engine_settings &eng_settings,
-        metrics::TelemetrySubmitter &msubmitter);
+        metrics::telemetry_submitter &msubmitter);
 
     static auto create(
         uint32_t trace_rate_limit = engine_settings::default_trace_rate_limit)
@@ -103,7 +103,7 @@ class engine {
     // Should not be called concurrently but safely publishes changes to common_
     // the rc client has a lock that ensures this
     virtual void update(
-        engine_ruleset &ruleset, metrics::TelemetrySubmitter &submit_metric);
+        engine_ruleset &ruleset, metrics::telemetry_submitter &submit_metric);
 
 protected:
     explicit engine(uint32_t trace_rate_limit)
@@ -114,7 +114,7 @@ class engine {
     // should use only atomic operations (pre-c++20
     // std::atomic<std::shared_ptr>)
     std::shared_ptr<shared_state> common_;
-    std::shared_ptr<metrics::TelemetrySubmitter> msubmitter_;
+    std::shared_ptr<metrics::telemetry_submitter> msubmitter_;
     rate_limiter<dds::timer> limiter_;
 };
 

appsec/src/helper/metrics.cpp

@@ -6,25 +6,28 @@
 
 #pragma once
 
-#include <functional>
 #include <string>
 #include <string_view>
 
 namespace dds::metrics {
 
-struct TelemetrySubmitter {
-    TelemetrySubmitter() = default;
-    TelemetrySubmitter(const TelemetrySubmitter &) = delete;
-    TelemetrySubmitter &operator=(const TelemetrySubmitter &) = delete;
-    TelemetrySubmitter(TelemetrySubmitter &&) = delete;
-    TelemetrySubmitter &operator=(TelemetrySubmitter &&) = delete;
+struct telemetry_submitter {
+    telemetry_submitter() = default;
+    telemetry_submitter(const telemetry_submitter &) = delete;
+    telemetry_submitter &operator=(const telemetry_submitter &) = delete;
+    telemetry_submitter(telemetry_submitter &&) = delete;
+    telemetry_submitter &operator=(telemetry_submitter &&) = delete;
 
-    virtual ~TelemetrySubmitter() = 0;
+    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_legacy_metric(std::string_view, double) = 0;
-    virtual void submit_legacy_meta(std::string_view, std::string) = 0;
-    virtual void submit_legacy_meta_copy_key(std::string, std::string) = 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;
+    virtual void submit_span_meta_copy_key(std::string, std::string) = 0;
+    void submit_span_meta_copy_key(std::string_view, std::string) = delete;
 };
+inline telemetry_submitter::~telemetry_submitter() = default;
 
 constexpr std::string_view waf_init = "waf.init";
 constexpr std::string_view waf_updates = "waf.updates";
@@ -38,9 +41,6 @@ constexpr std::string_view waf_truncated_value_size =
 constexpr std::string_view waf_duration_tel = "waf.duration";
 constexpr std::string_view waf_duration_ext = "waf.duration_ext";
 
-constexpr std::string_view rc_first_pull = "remote_config.first_pull";
-constexpr std::string_view rc_last_success = "remote_config.last_success";
-
 // not implemented (difficult to count requests on the helper)
 constexpr std::string_view rc_requests_before_running =
     "remote_config.requests_before_running";

appsec/src/helper/metrics.hpp

@@ -4,7 +4,6 @@
 // This product includes software developed at Datadog
 // (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
 #include "acceptor.hpp"
-#include "../exception.hpp"
 #include "socket.hpp"
 #include <cerrno>
 #include <chrono>

appsec/src/helper/network/acceptor.cpp

@@ -7,7 +7,6 @@
 #include "../exception.hpp"
 #include "proto.hpp"
 #include <chrono>
-#include <iostream>
 #include <msgpack.hpp>
 #include <spdlog/spdlog.h>
 #include <sstream>

appsec/src/helper/network/broker.cpp

@@ -130,7 +130,8 @@ struct client_init {
 
         std::map<std::string, std::string> meta;
         std::map<std::string_view, double> metrics;
-        std::map<std::string_view, std::vector<std::pair<double, std::string>>>
+        std::unordered_map<std::string_view,
+            std::vector<std::pair<double, std::string>>>
             tel_metrics;
 
         MSGPACK_DEFINE(status, version, errors, meta, metrics, tel_metrics);
@@ -291,7 +292,8 @@ struct request_shutdown {
 
         std::map<std::string, std::string> meta;
         std::map<std::string_view, double> metrics;
-        std::map<std::string_view, std::vector<std::pair<double, std::string>>>
+        std::unordered_map<std::string_view,
+            std::vector<std::pair<double, std::string>>>
             tel_metrics;
 
         MSGPACK_DEFINE(

appsec/src/helper/network/proto.hpp

@@ -4,10 +4,7 @@
 // This product includes software developed at Datadog
 // (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
 #include "client.hpp"
-#include "exception.hpp"
 #include "product.hpp"
-#include <algorithm>
-#include <regex>
 #include <set>
 #include <spdlog/spdlog.h>
 #include <stdexcept>
@@ -18,7 +15,7 @@ extern "C" {
 }
 
 namespace {
-struct ddog_CharSlice {
+struct ddog_CharSlice { // NOLINT(readability-identifier-naming)
     const char *ptr;
     uintptr_t len;
 };

appsec/src/helper/remote_config/client.cpp

@@ -19,7 +19,7 @@ static constexpr std::chrono::milliseconds default_max_interval = 5min;
 
 client_handler::client_handler(std::unique_ptr<client> &&rc_client,
     std::shared_ptr<service_config> service_config,
-    std::shared_ptr<metrics::TelemetrySubmitter> msubmitter)
+    std::shared_ptr<metrics::telemetry_submitter> msubmitter)
     : rc_client_{std::move(rc_client)},
       service_config_{std::move(service_config)},
       msubmitter_{std::move(msubmitter)}
@@ -30,7 +30,7 @@ std::unique_ptr<client_handler> client_handler::from_settings(
     std::shared_ptr<dds::service_config> service_config,
     const remote_config::settings &rc_settings,
     const std::shared_ptr<engine> &engine_ptr,
-    std::shared_ptr<metrics::TelemetrySubmitter> msubmitter,
+    std::shared_ptr<metrics::telemetry_submitter> msubmitter,
     bool dynamic_enablement)
 {
     if (!rc_settings.enabled) {
@@ -73,28 +73,7 @@ void client_handler::poll()
     const std::lock_guard lock{mutex_};
 
     try {
-        if (last_success_ != empty_time) {
-            auto now = std::chrono::steady_clock::now();
-            auto elapsed =
-                std::chrono::duration_cast<std::chrono::milliseconds>(
-                    now - last_success_);
-            msubmitter_->submit_metric(metrics::rc_last_success,
-                static_cast<double>(elapsed.count()), {});
-        }
-
-        const bool result = rc_client_->poll();
-
-        auto now = std::chrono::steady_clock::now();
-        last_success_ = now;
-
-        if (result && creation_time_ != empty_time) {
-            auto elapsed =
-                std::chrono::duration_cast<std::chrono::milliseconds>(
-                    now - creation_time_);
-            msubmitter_->submit_metric(metrics::rc_first_pull,
-                static_cast<double>(elapsed.count()), {});
-            creation_time_ = empty_time;
-        }
+        rc_client_->poll();
     } catch (const std::exception &e) {
         SPDLOG_WARN("Error polling remote config: {}", e.what());
     }

appsec/src/helper/remote_config/client_handler.cpp

@@ -21,7 +21,7 @@ class client_handler {
 public:
     client_handler(std::unique_ptr<remote_config::client> &&rc_client,
         std::shared_ptr<service_config> service_config,
-        std::shared_ptr<metrics::TelemetrySubmitter> msubmitter);
+        std::shared_ptr<metrics::telemetry_submitter> msubmitter);
     ~client_handler() = default;
 
     client_handler(const client_handler &) = delete;
@@ -35,7 +35,7 @@ class client_handler {
         std::shared_ptr<dds::service_config> service_config,
         const remote_config::settings &rc_settings,
         const std::shared_ptr<engine> &engine_ptr,
-        std::shared_ptr<metrics::TelemetrySubmitter> msubmitter,
+        std::shared_ptr<metrics::telemetry_submitter> msubmitter,
         bool dynamic_enablement);
 
     void poll();
@@ -51,12 +51,11 @@ class client_handler {
 
     std::shared_ptr<service_config> service_config_;
     std::unique_ptr<remote_config::client> rc_client_;
-    std::shared_ptr<metrics::TelemetrySubmitter> msubmitter_;
+    std::shared_ptr<metrics::telemetry_submitter> msubmitter_;
 
     std::mutex mutex_{};
     std::chrono::steady_clock::time_point creation_time_{
         std::chrono::steady_clock::now()}; // def value after first poll() done
-    std::chrono::steady_clock::time_point last_success_{};
 };
 
 } // namespace dds::remote_config

appsec/src/helper/remote_config/client_handler.hpp

@@ -4,23 +4,20 @@
 // This product includes software developed at Datadog
 // (https://www.datadoghq.com/). Copyright 2021 Datadog, Inc.
 #include "engine_listener.hpp"
-#include "../../json_helper.hpp"
 #include "../exception.hpp"
 #include "../product.hpp"
 #include "config_aggregators/asm_aggregator.hpp"
 #include "config_aggregators/asm_data_aggregator.hpp"
 #include "config_aggregators/asm_dd_aggregator.hpp"
-#include <optional>
 #include <rapidjson/document.h>
 #include <rapidjson/rapidjson.h>
 #include <spdlog/spdlog.h>
-#include <type_traits>
 #include <utility>
 
 namespace dds::remote_config {
 
 engine_listener::engine_listener(std::shared_ptr<engine> engine,
-    std::shared_ptr<dds::metrics::TelemetrySubmitter> msubmitter,
+    std::shared_ptr<dds::metrics::telemetry_submitter> msubmitter,
     const std::string &rules_file)
     : engine_{std::move(engine)}, msubmitter_{std::move(msubmitter)}
 {