Skip to content

Commit

Permalink
Addressed review comments:
Browse files Browse the repository at this point in the history
1. Got rid of std::time in favor of std::chrono
2. Improved guard implementation
  • Loading branch information
kamil-holubicki committed Mar 22, 2024
1 parent 0d5a938 commit 2cc495f
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 46 deletions.
29 changes: 18 additions & 11 deletions components/percona_telemetry/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
9 changes: 5 additions & 4 deletions components/percona_telemetry/config.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef PERCONA_TELEMETRY_CONFIG_H
#define PERCONA_TELEMETRY_CONFIG_H

#include <chrono>
#include <memory>
#include <string>

Expand All @@ -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_;
Expand Down
17 changes: 7 additions & 10 deletions components/percona_telemetry/data_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,10 @@ bool DataProvider::do_query(const std::string &query, QueryResult *result,
if (!sstatus) sstatus |= command_factory_service_.connect(mysql_h);

// starting from this point, if the above succeeded we need to close mysql_h.
std::shared_ptr<bool> mysql_h_close_guard(
new bool(), [&srv = command_factory_service_, &mysql_h,
do_close = !sstatus](bool *p) {
delete p;
if (do_close) srv.close(mysql_h);
std::shared_ptr<void> mysql_h_close_guard(
mysql_h,
[&srv = command_factory_service_, do_close = !sstatus](void *ptr) {
if (do_close && ptr) srv.close(static_cast<MYSQL_H>(ptr));
});

// if any of the above failed, just exit
Expand All @@ -141,11 +140,9 @@ bool DataProvider::do_query(const std::string &query, QueryResult *result,

command_query_result_service_.store_result(mysql_h, &mysql_res);
if (mysql_res) {
std::shared_ptr<bool> query_result_free_guard(
new bool(),
[&srv = command_query_result_service_, &mysql_res](bool *p) {
delete p;
srv.free_result(mysql_res);
std::shared_ptr<void> query_result_free_guard(
mysql_res, [&srv = command_query_result_service_](void *ptr) {
if (ptr) srv.free_result(static_cast<MYSQL_RES_H>(ptr));
});

if (command_query_service_.affected_rows(mysql_h, &row_count)) {
Expand Down
27 changes: 15 additions & 12 deletions components/percona_telemetry/storage.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#include <ctime>
#include <filesystem>
#include <fstream>
#include <string_view>
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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);

Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion components/percona_telemetry/storage.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#ifndef PERCONA_TELEMETRY_STORAGE_H
#define PERCONA_TELEMETRY_STORAGE_H

#include <chrono>
#include <memory>
#include <string>
#include "config.h"
Expand All @@ -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_;
Expand Down
17 changes: 9 additions & 8 deletions components/percona_telemetry/worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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) {
Expand All @@ -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.
}
}
Expand Down

0 comments on commit 2cc495f

Please sign in to comment.