Fix a bunch of lints

cwaldren-ld · cwaldren-ld · commit 0b7da036f7af · 2023-04-12T17:46:20.000-07:00
diff --git a/.clang-tidy b/.clang-tidy
@@ -1,4 +1,5 @@
 ---
 CheckOptions:
   - { key: readability-identifier-length.IgnoredParameterNames,   value: 'i|j|k|c|os|it' }
+  - { key: readability-identifier-length.IgnoredVariableNames, value: 'ec' }
   - { key: readability-identifier-length.IgnoredLoopCounterNames,   value: 'i|j|k|c|os|it' }
diff --git a/libs/common/include/events/detail/asio_event_processor.hpp b/libs/common/include/events/detail/asio_event_processor.hpp
@@ -43,7 +43,7 @@ class AsioEventProcessor : public IEventProcessor {
 
     boost::asio::any_io_executor io_;
     Outbox outbox_;
-    Summarizer summary_state_;
+    Summarizer summarizer_;
 
     std::chrono::milliseconds flush_interval_;
     boost::asio::steady_timer timer_;
diff --git a/libs/common/include/events/detail/summarizer.hpp b/libs/common/include/events/detail/summarizer.hpp
@@ -9,24 +9,54 @@
 
 namespace launchdarkly::events::detail {
 
+/**
+ * Summarizer is responsible for accepting FeatureEventParams (the context
+ * related to a feature evaluation) and outputting summary events (which
+ * essentially condenses the various evaluation results into a single
+ * structure).
+ */
 class Summarizer {
    public:
-    using Date = std::chrono::system_clock::time_point;
+    using Time = std::chrono::system_clock::time_point;
     using FlagKey = std::string;
 
-    explicit Summarizer(Date start);
+    /**
+     * Construct a Summarizer starting at the given time.
+     * @param start Start time of the summary.
+     */
+    explicit Summarizer(Time start_time);
+
+    /**
+     * Construct a Summarizer at time zero.
+     */
     Summarizer();
-    void update(client::FeatureEventParams const& event);
 
-    bool empty() const;
+    /**
+     * Updates the summary with a feature event.
+     * @param event Feature event.
+     */
+    void Update(client::FeatureEventParams const& event);
+
+    /**
+     * Returns true if the summary is empty.
+     */
+    bool Empty() const;
 
-    Date start_time() const;
+    /**
+     * Returns the summary's start time as given in the constructor.
+     */
+    [[nodiscard]] Time start_time() const;
 
     struct VariationSummary {
-        std::size_t count;
-        Value value;
+       public:
         explicit VariationSummary(Value value);
         void Increment();
+        std::size_t count() const;
+        Value const& value() const;
+
+       private:
+        std::size_t count_;
+        Value value_;
     };
 
     struct VariationKey {
@@ -58,19 +88,19 @@ class Summarizer {
                            Summarizer::VariationKey::Hash>
             counters;
 
-        State(Value defaultVal);
+        explicit State(Value defaultVal);
     };
 
-    std::unordered_map<FlagKey, State> const& features() const;
+    [[nodiscard]] std::unordered_map<FlagKey, State> const& features() const;
 
    private:
-    Date start_time_;
+    Time start_time_;
     std::unordered_map<FlagKey, State> features_;
 };
 
 struct Summary {
     Summarizer const& summarizer;
-    Summarizer::Date end_time;
+    Summarizer::Time end_time;
 };
 
 }  // namespace launchdarkly::events::detail
diff --git a/libs/common/src/events/asio_event_processor.cpp b/libs/common/src/events/asio_event_processor.cpp
@@ -23,7 +23,7 @@ AsioEventProcessor::AsioEventProcessor(
     Logger& logger)
     : io_(boost::asio::make_strand(io)),
       outbox_(config.capacity()),
-      summary_state_(std::chrono::system_clock::now()),
+      summarizer_(std::chrono::system_clock::now()),
       flush_interval_(config.flush_interval()),
       timer_(io_),
       host_(endpoints.events_base_url()),  // TODO: parse and use host
@@ -68,14 +68,14 @@ void AsioEventProcessor::AsyncSend(InputEvent input_event) {
     if (!InboxIncrement()) {
         return;
     }
-    boost::asio::post(io_, [this, e = std::move(input_event)]() mutable {
+    boost::asio::post(io_, [this, event = std::move(input_event)]() mutable {
         InboxDecrement();
-        HandleSend(std::move(e));
+        HandleSend(std::move(event));
     });
 }
 
-void AsioEventProcessor::HandleSend(InputEvent e) {
-    std::vector<OutputEvent> output_events = Process(std::move(e));
+void AsioEventProcessor::HandleSend(InputEvent event) {
+    std::vector<OutputEvent> output_events = Process(std::move(event));
 
     bool inserted = outbox_.PushDiscardingOverflow(std::move(output_events));
     if (!inserted && !full_outbox_encountered_) {
@@ -93,7 +93,7 @@ void AsioEventProcessor::Flush(FlushTrigger flush_type) {
         LD_LOG(logger_, LogLevel::kDebug)
             << "event-processor: nothing to flush";
     }
-    summary_state_ = Summarizer(std::chrono::system_clock::now());
+    summarizer_ = Summarizer(std::chrono::system_clock::now());
     if (flush_type == FlushTrigger::Automatic) {
         ScheduleFlush();
     }
@@ -132,14 +132,16 @@ AsioEventProcessor::MakeRequest() {
         return std::nullopt;
     }
 
-    boost::json::value summary_event;
-    if (!summary_state_.empty()) {
-        summary_event = boost::json::value_from(
-            Summary{summary_state_, std::chrono::system_clock::now()});
+    auto events = boost::json::value_from(outbox_.Consume());
+
+    if (!summarizer_.Empty()) {
+        events.as_array().push_back(boost::json::value_from(
+            Summary{summarizer_, std::chrono::system_clock::now()}));
     }
 
     LD_LOG(logger_, LogLevel::kDebug)
         << "event-processor: generating http request";
+
     RequestType req;
 
     req.set(http::field::host, host_);
@@ -150,11 +152,6 @@ AsioEventProcessor::MakeRequest() {
     req.set(kPayloadIdHeader, boost::lexical_cast<std::string>(uuids_()));
     req.target(host_ + path_);
 
-    auto events = boost::json::value_from(outbox_.Consume());
-    if (!summary_event.is_null()) {
-        events.as_array().push_back(summary_event);
-    }
-
     req.body() = boost::json::serialize(events);
     req.prepare_payload();
     return req;
@@ -169,55 +166,57 @@ struct overloaded : Ts... {
 template <class... Ts>
 overloaded(Ts...) -> overloaded<Ts...>;
 
-std::vector<OutputEvent> AsioEventProcessor::Process(InputEvent event) {
+std::vector<OutputEvent> AsioEventProcessor::Process(InputEvent input_event) {
     std::vector<OutputEvent> out;
     std::visit(
-        overloaded{
-            [&](client::FeatureEventParams&& e) {
-                summary_state_.update(e);
-
-                if (!e.eval_result.track_events()) {
-                    return;
-                }
-                std::optional<Reason> reason;
-
-                // TODO(cwaldren): should also add the reason if the variation
-                // method was VariationDetail().
-                if (e.eval_result.track_reason()) {
-                    reason = e.eval_result.detail().reason();
-                }
-
-                client::FeatureEventBase b = {
-                    e.creation_date, std::move(e.key), e.eval_result.version(),
-                    e.eval_result.detail().variation_index(),
-                    e.eval_result.detail().value(), reason,
-                    // TODO(cwaldren): change to actual default; figure out
-                    // where this should be plumbed through.
-                    Value::null()};
-
-                auto debug_until_date = e.eval_result.debug_events_until_date();
-                bool emit_debug_event =
-                    debug_until_date &&
-                    debug_until_date.value() > std::chrono::system_clock::now();
-
-                if (emit_debug_event) {
-                    out.emplace_back(
-                        client::DebugEvent{b, filter_.filter(e.context)});
-                }
-                // TODO(cwaldren): see about not copying the keys / having the
-                // getter return a value.
-                out.emplace_back(client::FeatureEvent{
-                    std::move(b), e.context.kinds_to_keys()});
-            },
-            [&](client::IdentifyEventParams&& e) {
-                // Contexts should already have been checked for
-                // validity by this point.
-                assert(e.context.valid());
-                out.emplace_back(client::IdentifyEvent{
-                    e.creation_date, filter_.filter(e.context)});
-            },
-            [&](TrackEventParams&& e) { out.emplace_back(std::move(e)); }},
-        std::move(event));
+        overloaded{[&](client::FeatureEventParams&& event) {
+                       summarizer_.Update(event);
+
+                       if (!event.eval_result.track_events()) {
+                           return;
+                       }
+                       std::optional<Reason> reason;
+
+                       // TODO(cwaldren): should also add the reason if the
+                       // variation method was VariationDetail().
+                       if (event.eval_result.track_reason()) {
+                           reason = event.eval_result.detail().reason();
+                       }
+
+                       client::FeatureEventBase base = {
+                           event.creation_date, std::move(event.key),
+                           event.eval_result.version(),
+                           event.eval_result.detail().variation_index(),
+                           event.eval_result.detail().value(), reason,
+                           // TODO(cwaldren): change to actual default; figure
+                           // out where this should be plumbed through.
+                           Value::null()};
+
+                       auto debug_until_date =
+                           event.eval_result.debug_events_until_date();
+                       bool emit_debug_event =
+                           debug_until_date &&
+                           debug_until_date.value() >
+                               std::chrono::system_clock::now();
+
+                       if (emit_debug_event) {
+                           out.emplace_back(client::DebugEvent{
+                               base, filter_.filter(event.context)});
+                       }
+                       out.emplace_back(client::FeatureEvent{
+                           std::move(base), event.context.kinds_to_keys()});
+                   },
+                   [&](client::IdentifyEventParams&& event) {
+                       // Contexts should already have been checked for
+                       // validity by this point.
+                       assert(event.context.valid());
+                       out.emplace_back(client::IdentifyEvent{
+                           event.creation_date, filter_.filter(event.context)});
+                   },
+                   [&](TrackEventParams&& event) {
+                       out.emplace_back(std::move(event));
+                   }},
+        std::move(input_event));
 
     return out;
 }
diff --git a/libs/common/src/events/summarizer.cpp b/libs/common/src/events/summarizer.cpp
@@ -7,7 +7,7 @@ Summarizer::Summarizer(std::chrono::system_clock::time_point start)
 
 Summarizer::Summarizer() : start_time_(), features_() {}
 
-bool Summarizer::empty() const {
+bool Summarizer::Empty() const {
     return features_.empty();
 }
 
@@ -24,12 +24,9 @@ static bool FlagNotFound(client::FeatureEventParams const& event) {
     return false;
 }
 
-void Summarizer::update(client::FeatureEventParams const& event) {
+void Summarizer::Update(client::FeatureEventParams const& event) {
     auto const& kinds = event.context.kinds();
 
-    // TODO(cwaldren): Value::null() should be replaced with the default value
-    // from the evaluation.
-
     auto feature_state_iterator =
         features_.try_emplace(event.key, event.default_).first;
 
@@ -57,9 +54,11 @@ void Summarizer::update(client::FeatureEventParams const& event) {
 
     summary_counter->second.Increment();
 }
-Summarizer::Date Summarizer::start_time() const {
+
+Summarizer::Time Summarizer::start_time() const {
     return start_time_;
 }
+
 Summarizer::VariationKey::VariationKey(Version version,
                                        std::optional<VariationIndex> variation)
     : version(version), variation(variation) {}
@@ -68,13 +67,21 @@ Summarizer::VariationKey::VariationKey()
     : version(std::nullopt), variation(std::nullopt) {}
 
 Summarizer::VariationSummary::VariationSummary(Value value)
-    : count(0), value(std::move(value)) {}
+    : count_(0), value_(std::move(value)) {}
 
 void Summarizer::VariationSummary::Increment() {
     // todo(cwaldren): prevent overflow?
-    count++;
+    count_++;
+}
+
+Value const& Summarizer::VariationSummary::value() const {
+    return value_;
+}
+
+std::size_t Summarizer::VariationSummary::count() const {
+    return count_;
 }
 
-Summarizer::State::State(Value defaultVal)
-    : default_(std::move(defaultVal)), context_kinds() {}
+Summarizer::State::State(Value default_value)
+    : default_(std::move(default_value)), context_kinds() {}
 }  // namespace launchdarkly::events::detail
diff --git a/libs/common/src/serialization/events/json_events.cpp b/libs/common/src/serialization/events/json_events.cpp
@@ -124,8 +124,8 @@ void tag_invoke(boost::json::value_from_tag const&,
         if (kvp.first.variation) {
             counter.emplace("variation", *kvp.first.variation);
         }
-        counter.emplace("value", boost::json::value_from(kvp.second.value));
-        counter.emplace("count", kvp.second.count);
+        counter.emplace("value", boost::json::value_from(kvp.second.value()));
+        counter.emplace("count", kvp.second.count());
         counters.push_back(std::move(counter));
     }
     obj.emplace("counters", std::move(counters));
diff --git a/libs/common/tests/event_processor_test.cpp b/libs/common/tests/event_processor_test.cpp
@@ -18,7 +18,7 @@ static std::chrono::system_clock::time_point ZeroTime() {
 
 TEST(SummarizerTests, IsEmptyOnConstruction) {
     Summarizer summarizer;
-    ASSERT_TRUE(summarizer.empty());
+    ASSERT_TRUE(summarizer.Empty());
 }
 
 TEST(SummarizerTests, DefaultConstructionUsesZeroStartTime) {
@@ -60,7 +60,7 @@ TEST(SummarizerTests, SummaryCounterUpdates) {
 
     auto const num_events = 10;
     for (size_t i = 0; i < num_events; i++) {
-        summarizer.update(event);
+        summarizer.Update(event);
     }
 
     auto const& features = summarizer.features();
@@ -71,8 +71,8 @@ TEST(SummarizerTests, SummaryCounterUpdates) {
         Summarizer::VariationKey(feature_version, feature_variation));
     ASSERT_TRUE(counter != cat_food->second.counters.end());
 
-    ASSERT_EQ(counter->second.value.as_double(), feature_value.as_double());
-    ASSERT_EQ(counter->second.count, num_events);
+    ASSERT_EQ(counter->second.value().as_double(), feature_value.as_double());
+    ASSERT_EQ(counter->second.count(), num_events);
 }
 
 TEST(SummarizerTests, JsonSerialization) {
@@ -103,7 +103,7 @@ TEST(SummarizerTests, JsonSerialization) {
 
     auto const num_events = 10;
     for (size_t i = 0; i < num_events; i++) {
-        summarizer.update(event);
+        summarizer.Update(event);
     }
 
     auto json = boost::json::value_from(Summary{summarizer, ZeroTime()});

Original file line number	Diff line number	Diff line change
`@@ -124,8 +124,8 @@ void tag_invoke(boost::json::value_from_tag const&,`
`124`	`124`	`if (kvp.first.variation) {`
`125`	`125`	`counter.emplace("variation", *kvp.first.variation);`
`126`	`126`	`}`
`127`		`- counter.emplace("value", boost::json::value_from(kvp.second.value));`
`128`		`- counter.emplace("count", kvp.second.count);`
	`127`	`+ counter.emplace("value", boost::json::value_from(kvp.second.value()));`
	`128`	`+ counter.emplace("count", kvp.second.count());`
`129`	`129`	`counters.push_back(std::move(counter));`
`130`	`130`	`}`
`131`	`131`	`obj.emplace("counters", std::move(counters));`