Addressed review comments:

Got rid of std::time in favor of std::chrono
percona · Mar 22, 2024 · d63fe80 · d63fe80
1 parent 0d5a938
commit d63fe80
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 36 deletions.
diff --git a/components/percona_telemetry/config.cc b/components/percona_telemetry/config.cc
@@ -50,8 +50,9 @@ bool Config::init() {
   scrape_interval_arg.blk_sz = 0;
   if (var_register_service_.register_variable(
           CURRENT_COMPONENT_NAME_STR, VAR_SCRAPE_INTERVAL,
-          PLUGIN_VAR_INT | PLUGIN_VAR_READONLY, "Telemetry scrape interval",
-          nullptr, nullptr, &scrape_interval_arg, &scrape_interval_value_)) {
+          PLUGIN_VAR_UNSIGNED | PLUGIN_VAR_INT | PLUGIN_VAR_READONLY,
+          "Telemetry scrape interval", nullptr, nullptr, &scrape_interval_arg,
+          &scrape_interval_value_)) {
     return true;
   }
 
@@ -61,8 +62,9 @@ bool Config::init() {
   grace_interval_arg.blk_sz = 0;
   if (var_register_service_.register_variable(
           CURRENT_COMPONENT_NAME_STR, VAR_GRACE_INTERVAL,
-          PLUGIN_VAR_INT | PLUGIN_VAR_READONLY, "Telemetry grace interval",
-          nullptr, nullptr, &grace_interval_arg, &grace_interval_value_)) {
+          PLUGIN_VAR_UNSIGNED | PLUGIN_VAR_INT | PLUGIN_VAR_READONLY,
+          "Telemetry grace interval", nullptr, nullptr, &grace_interval_arg,
+          &grace_interval_value_)) {
     return true;
   }
 
@@ -72,7 +74,7 @@ bool Config::init() {
   history_keep_interval_arg.blk_sz = 0;
   if (var_register_service_.register_variable(
           CURRENT_COMPONENT_NAME_STR, VAR_HISTORY_KEEP_INTERVAL,
-          PLUGIN_VAR_INT | PLUGIN_VAR_READONLY,
+          PLUGIN_VAR_UNSIGNED | PLUGIN_VAR_INT | PLUGIN_VAR_READONLY,
           "Telemetry history keep interval", nullptr, nullptr,
           &history_keep_interval_arg, &history_keep_interval_value_)) {
     return true;
@@ -110,14 +112,19 @@ const std::string &Config::telemetry_storage_dir_path() const noexcept {
   return telemetry_root_dir_value_str;
 }
 
-uint Config::scrape_interval() const noexcept { return scrape_interval_value_; }
+std::chrono::seconds Config::scrape_interval() const noexcept {
+  return std::chrono::seconds(scrape_interval_value_);
+}
 
-uint Config::grace_interval() const noexcept { return grace_interval_value_; }
+std::chrono::seconds Config::grace_interval() const noexcept {
+  return std::chrono::seconds(grace_interval_value_);
+}
 
-uint Config::history_keep_interval() const noexcept {
-  return history_keep_interval_value_;
+std::chrono::seconds Config::history_keep_interval() const noexcept {
+  return std::chrono::seconds(history_keep_interval_value_);
 }
 
-uint Config::unconditional_history_cleanup_interval() const noexcept {
-  return HISTORY_KEEP_INTERVAL_MAX;
+std::chrono::seconds Config::unconditional_history_cleanup_interval()
+    const noexcept {
+  return std::chrono::seconds(HISTORY_KEEP_INTERVAL_MAX);
 }
diff --git a/components/percona_telemetry/config.h b/components/percona_telemetry/config.h
@@ -1,6 +1,7 @@
 #ifndef PERCONA_TELEMETRY_CONFIG_H
 #define PERCONA_TELEMETRY_CONFIG_H
 
+#include <chrono>
 #include <memory>
 #include <string>
 
@@ -23,10 +24,10 @@ class Config {
   bool deinit();
 
   const std::string &telemetry_storage_dir_path() const noexcept;
-  uint scrape_interval() const noexcept;
-  uint grace_interval() const noexcept;
-  uint history_keep_interval() const noexcept;
-  uint unconditional_history_cleanup_interval() const noexcept;
+  std::chrono::seconds scrape_interval() const noexcept;
+  std::chrono::seconds grace_interval() const noexcept;
+  std::chrono::seconds history_keep_interval() const noexcept;
+  std::chrono::seconds unconditional_history_cleanup_interval() const noexcept;
 
  private:
   SERVICE_TYPE(component_sys_variable_register) & var_register_service_;

diff --git a/components/percona_telemetry/storage.cc b/components/percona_telemetry/storage.cc
@@ -1,4 +1,3 @@
-#include <ctime>
 #include <filesystem>
 #include <fstream>
 #include <string_view>
@@ -41,9 +40,9 @@ static std::string random_uuid() {
 Storage::Storage(Config &config, Logger &logger)
     : config_(config), logger_(logger), uuid_(random_uuid()) {}
 
-void Storage::clean_old_reports(long current_time) {
-  long oldest_timestamp = current_time - config_.history_keep_interval();
-  long oldest_unconditional_cleanup_timestamp =
+void Storage::clean_old_reports(std::chrono::seconds current_time) {
+  auto oldest_timestamp = current_time - config_.history_keep_interval();
+  auto oldest_unconditional_cleanup_timestamp =
       current_time - config_.unconditional_history_cleanup_interval();
 
   std::vector<std::string> files_to_delete;
@@ -65,7 +64,8 @@ void Storage::clean_old_reports(long current_time) {
       bool our_file = (file_uuid == uuid_);
 
       try {
-        long file_timestamp = std::stol(file_timestamp_str);
+        auto file_timestamp =
+            std::chrono::seconds(std::stoul(file_timestamp_str));
         // We need to delete the file in 2 cases:
         // 1. This is our file and it is older than this instance's
         // history_keep_interval
@@ -74,16 +74,17 @@ void Storage::clean_old_reports(long current_time) {
         if (our_file && (file_timestamp < oldest_timestamp)) {
           logger_.info(
               "Scheduling file %s owned by this server for deletion because it "
-              "is older than %d seconds",
-              entry.path().filename().c_str(), config_.history_keep_interval());
+              "is older than %ld seconds",
+              entry.path().filename().c_str(),
+              config_.history_keep_interval().count());
           files_to_delete.push_back(entry.path());
         } else if (!our_file &&
                    (file_timestamp < oldest_unconditional_cleanup_timestamp)) {
           logger_.info(
               "Scheduling file %s owned by other server for deletion because "
-              "it is older than %d seconds",
+              "it is older than %ld seconds",
               entry.path().filename().c_str(),
-              config_.unconditional_history_cleanup_interval());
+              config_.unconditional_history_cleanup_interval().count());
           files_to_delete.push_back(entry.path());
         }
       } catch (const std::invalid_argument &e) {
@@ -109,7 +110,9 @@ void Storage::clean_old_reports(long current_time) {
 
 bool Storage::store_report(const std::string &report) {
   try {
-    long current_time = (long)std::time(0);
+    auto now = std::chrono::system_clock::now();
+    auto current_time = std::chrono::duration_cast<std::chrono::seconds>(
+        now.time_since_epoch());
 
     clean_old_reports(current_time);
 
@@ -120,8 +123,8 @@ bool Storage::store_report(const std::string &report) {
     }
 
     std::string filename = config_.telemetry_storage_dir_path() +
-                           path_separator + std::to_string(current_time) + "-" +
-                           uuid_;
+                           path_separator +
+                           std::to_string(current_time.count()) + "-" + uuid_;
     std::string tmp_filename = filename + TMP_FILE_EXTENSION;
     std::string json_filename = filename + JSON_FILE_EXTENSION;
     std::ofstream file_stream(tmp_filename);

diff --git a/components/percona_telemetry/storage.h b/components/percona_telemetry/storage.h
@@ -1,6 +1,7 @@
 #ifndef PERCONA_TELEMETRY_STORAGE_H
 #define PERCONA_TELEMETRY_STORAGE_H
 
+#include <chrono>
 #include <memory>
 #include <string>
 #include "config.h"
@@ -13,7 +14,7 @@ class Storage {
   bool store_report(const std::string &report);
 
  private:
-  void clean_old_reports(long current_time);
+  void clean_old_reports(std::chrono::seconds current_time);
 
   Config &config_;
   Logger &logger_;

diff --git a/components/percona_telemetry/worker.cc b/components/percona_telemetry/worker.cc
@@ -33,12 +33,12 @@ bool Worker::stop() {
 void Worker::worker_thd_fn() {
   std::mutex m;
 
-  int grace_interval = config_.grace_interval();
-  logger_.info("Applying Telemetry grace interval %d seconds", grace_interval);
+  auto grace_interval = config_.grace_interval();
+  logger_.info("Applying Telemetry grace interval %ld seconds",
+               grace_interval.count());
   {
     std::unique_lock<std::mutex> lock(m);
-    if (std::cv_status::timeout !=
-        cv_.wait_for(lock, std::chrono::seconds(grace_interval))) {
+    if (std::cv_status::timeout != cv_.wait_for(lock, grace_interval)) {
       // the thread was signaled to exit
       return;
     }
@@ -50,9 +50,10 @@ void Worker::worker_thd_fn() {
 
   data_provider_.thread_access_begin();
 
-  logger_.info("Scrape interval: %d seconds", config_.scrape_interval());
-  logger_.info("History keep interval: %d seconds",
-               config_.history_keep_interval());
+  logger_.info("Scrape interval: %ld seconds",
+               config_.scrape_interval().count());
+  logger_.info("History keep interval: %ld seconds",
+               config_.history_keep_interval().count());
   logger_.info("Storage dir: %s", config_.telemetry_storage_dir_path().c_str());
 
   while (true) {
@@ -74,7 +75,7 @@ void Worker::worker_thd_fn() {
       caller_active_.clear();
 
       std::unique_lock<std::mutex> lock(m);
-      cv_.wait_for(lock, std::chrono::seconds(config_.scrape_interval()));
+      cv_.wait_for(lock, config_.scrape_interval());
       // Timed out or signaled to exit. Go on anyway.
     }
   }