fix(bq_driver): Fix tracing file size and file count

ci/cloudbuild/builds/demo-install.sh

@@ -22,6 +22,7 @@ source module ci/install-dependencies.sh
 source module ci/cloudbuild/builds/lib/cmake.sh
 source module ci/cloudbuild/builds/lib/unit-tests.sh
 source module ci/lib/io.sh
+cp ci/gha/builds/lib/google.googlebigqueryodbc.ini /opt/google.googlebigqueryodbc.ini
 
 cmake_config_testing_details=(
   # -DCMAKE_TOOLCHAIN_FILE="${VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake"
@@ -46,6 +47,7 @@ cmake --build cmake-out -- -j "$(nproc)"
 cmake --build cmake-out --target install
 ## [DONE packaging.md]
 
+export GOOGLEBIGQUERYODBCINI=/opt/google.googlebigqueryodbc.ini
 mapfile -t ctest_args < <(ctest::common_args)
 # I am unable to upgrade coreutils on Centos 7. So,
 # `env -C cmake-out ctest "${ctest_args[@]}"` throws

google/cloud/odbc/bq_driver/internal/driver_log_form.cc

@@ -27,7 +27,7 @@ std::string const kLogError = "LOG_ERROR";
 std::string const kLogInfo = "LOG_INFO";
 std::string const kLogWarning = "LOG_WARNING";
 std::string const kDefaultMaxFiles = "50";
-std::string const kDefaultMaxSize = "20";
+std::string const kDefaultMaxSize = "2000";
 Section LogTraceDialog::last_log_saved_values_;
 std::string LogTraceDialog::log_level_ = kLogOff;
 std::string LogTraceDialog::log_file_path_;
@@ -165,7 +165,7 @@ void LogTraceDialog::InitControls() {
 
   // Max File Size (MB) Label and Edit Box
   HWND h_max_size_label =
-      CreateLabel(parent_hwnd, "Max file size (MB):", KAxisX, KAxisY + 125,
+      CreateLabel(parent_hwnd, "Max file size (KB):", KAxisX, KAxisY + 125,
                   KLabelWidth + 60, kLabelHeight, 0);
   SendMessage(h_max_size_label, WM_SETFONT, (WPARAM)h_font, TRUE);
 

google/cloud/odbc/bq_driver/internal/odbc_sql_tables.cc

@@ -443,11 +443,8 @@ StatusRecordOr<ResultSet> GetResultSetForTables(
   };
 
   // 3. Execute tasks using the generic utility
-  int max_threads = 8;
   std::shared_ptr<TraceOptions> trace_option = TraceOptions::GetTraceOption();
-  if (trace_option != nullptr && trace_option->max_threads > 0) {
-    max_threads = trace_option->max_threads;
-  }
+  int max_threads = trace_option->max_threads;
   auto parallel_results_or = ExecuteParallelTasks<TaskInput, TaskResult>(
       max_threads, tasks, parallel_func);
 

google/cloud/odbc/bq_driver/internal/trace_utils.cc

@@ -24,7 +24,7 @@ using ::google::cloud::odbc_internal::StatusRecord;
 using ::google::cloud::odbc_internal::StatusRecordOr;
 
 namespace fs = std::filesystem;
-constexpr int kCharBufSize1 = 1024;
+constexpr int kKB = 1024;
 constexpr int kCharBufSize2 = 256;
 
 static std::once_flag absl_log_init_flag;
@@ -54,6 +54,11 @@ FileLogSink::FileLogSink(std::shared_ptr<TraceOptions> opts)
     if (!fp_) {
       return;
     }
+    if (std::filesystem::exists(current_file_)) {
+      current_file_size_ = std::filesystem::file_size(current_file_);
+    } else {
+      current_file_size_ = 0;
+    }
   }
 }
 
@@ -64,49 +69,36 @@ FileLogSink::~FileLogSink() {
     fp_ = nullptr;
   }
 }
-// Required for custom log formatting and writing to the driver's default log
-// file
+// Required for writing to the driver's default log file
 void FileLogSink::Send(absl::LogEntry const& entry) {
   std::lock_guard<std::mutex> lock(log_mutex_);
   // Logging disabled or never initialized
-  if (!fp_ || !opts_) {
-    return;
-  }
+  if (!fp_ || !opts_) return;
 
-  auto message = entry.text_message_with_prefix_and_newline();
-  std::size_t new_log_size = message.size() + 1;
-  std::uintmax_t max_file_size_bytes = opts_->max_file_size * 1024 * 1024;
-
-  if (!CanWriteToFile(current_file_, new_log_size, max_file_size_bytes)) {
-    if (fp_ != nullptr) {
-      fclose(fp_);
-      fp_ = nullptr;
-    }
+  auto& opts = *opts_;
+  auto formatted_msg = GetFormattedMsg(entry);
 
+  std::size_t new_log_size = formatted_msg.size();
+  std::uintmax_t max_file_size_bytes =
+      opts.max_file_size * kKB;  // file size is in KB
+  if (current_file_size_ + new_log_size >= max_file_size_bytes) {
+    fclose(fp_);
+    fp_ = nullptr;
     // Remove the oldest log file (if limit reached), then advance to the next
     // index.
-    ClearOldLogFiles(opts_->log_path, opts_->current_file_index,
-                     opts_->max_file_count);
-    ++opts_->current_file_index;
+    ClearOldLogFiles(opts.log_path, opts.current_file_index,
+                     opts.max_file_count);
+    ++opts.current_file_index;
 
-    current_file_ = GetLogFileWithIndex(opts_->log_path);
+    current_file_ = GetLogFileWithIndex(opts.log_path);
     fp_ = fopen(current_file_.c_str(), "a");
-  }
-  std::string time_str = absl::FormatTime(
-      "%Y-%m-%d %H:%M:%S", entry.timestamp(), absl::LocalTimeZone());
+    if (!fp_) return;
 
-  auto log_message = std::string(entry.text_message());
-  std::string log_tag = absl::LogSeverityName(entry.log_severity());
-
-  absl::string_view full_path = entry.source_filename();
-  size_t last_sep = full_path.find_last_of("/\\");
-  absl::string_view file_name = (last_sep == absl::string_view::npos)
-                                    ? full_path
-                                    : full_path.substr(last_sep + 1);
-
-  absl::FPrintF(fp_, "[%s] [%s] [%s:%d] %s\n", log_tag, time_str, file_name,
-                entry.source_line(), entry.text_message());
+    current_file_size_ = 0;
+  }
+  absl::FPrintF(fp_, "%s", formatted_msg.c_str());
   fflush(fp_);
+  current_file_size_ += new_log_size;
 }
 
 absl::LogSeverity GetAbslSeverity(LogLevel level) {
@@ -122,10 +114,30 @@ absl::LogSeverity GetAbslSeverity(LogLevel level) {
   }
 }
 
+// Formats log msg in format: [LOG_LEVEL] [TIME] [FILE:LINE] MSG
+std::string GetFormattedMsg(absl::LogEntry const& entry) {
+  static absl::TimeZone const kTimeZone = absl::LocalTimeZone();
+  std::string time_str =
+      absl::FormatTime("%Y-%m-%d %H:%M:%S", entry.timestamp(), kTimeZone);
+
+  char const* log_tag = absl::LogSeverityName(entry.log_severity());
+  absl::string_view full_path = entry.source_filename();
+
+  size_t pos = full_path.rfind('/');
+  if (pos == absl::string_view::npos) pos = full_path.rfind('\\');
+
+  absl::string_view file_name =
+      pos == absl::string_view::npos ? full_path : full_path.substr(pos + 1);
+
+  std::string msg =
+      absl::StrFormat("[%s] [%s] [%s:%d] %s\n", log_tag, time_str, file_name,
+                      entry.source_line(), entry.text_message());
+  return msg;
+}
+
 void ClearOldLogFiles(std::string const& base_dir, int next_index,
                       int max_file_count) {
-  // No rotation needed if only 1 file allowed
-  if (max_file_count <= 1) return;
+  if (max_file_count < 1) return;
 
   // Oldest index that must be deleted
   int oldest_index = next_index - (max_file_count - 1);
@@ -197,16 +209,6 @@ void FileLogSink::InitializeFileLog(
   absl::log_internal::AddLogSink(file_sink_.get());
 }
 
-bool CanWriteToFile(std::string const& log_file, std::size_t new_log_size,
-                    std::uintmax_t max_file_size_bytes) {
-  std::ifstream file(log_file, std::ios::binary | std::ios::ate);
-  if (!file.is_open()) {
-    return true;
-  }
-  std::uintmax_t current_file_size = file.tellg();
-  return (current_file_size + new_log_size) <= max_file_size_bytes;
-}
-
 bool TraceOptions::InitializeLogging(bool is_trace_override) {
   // suppress all stderr output
   std::call_once(absl_log_init_flag, []() { absl::InitializeLog(); });
@@ -269,11 +271,17 @@ TraceOptions::CreateTraceOptionsFile(
     trace_sections = odbc_section->second;
   }
 
+  std::lock_guard<std::mutex> lk(mu_);
+  if (options_file_ == nullptr) {
+    // Cannot use std::make_shared because constructor is protected.
+    options_file_ = std::shared_ptr<TraceOptions>(new TraceOptions());
+  }
+
   std::string log_path;
-  int log_level = 0;
-  int log_file_count;
-  int log_file_size;
-  int max_threads = 8;  // default max_threads
+  int log_level = options_file_->log_level;
+  int log_file_count = options_file_->max_file_count;
+  int log_file_size = options_file_->max_file_size;
+  int max_threads = options_file_->max_threads;  // default max_threads
   for (auto const& s : trace_sections) {
     if (s.first == kLogLevel && !s.second.empty()) {
       log_level = std::strtol(s.second.c_str(), nullptr, 10);
@@ -288,12 +296,6 @@ TraceOptions::CreateTraceOptionsFile(
     }
   }
 
-  std::lock_guard<std::mutex> lk(mu_);
-  if (options_file_ == nullptr) {
-    // Cannot use std::make_shared because constructor is protected.
-    options_file_ = std::shared_ptr<TraceOptions>(new TraceOptions());
-  }
-
   if (log_level > 0) {
     options_file_->log_level = log_level;
     options_file_->log_path = log_path;

google/cloud/odbc/bq_driver/internal/trace_utils.h

@@ -102,8 +102,8 @@ struct TraceOptions {
   bool logging_enabled;
   bool is_file_closed;
   int log_level{0};
-  int max_file_count{50};  // max number of log files (50).
-  int max_file_size{20};   // max file size of a single file(20 MB)
+  int max_file_count{50};   // max number of log files (50).
+  int max_file_size{2000};  // max file size of a single file(2000 KB)
   int max_threads{8};
   int current_file_index{0};
   std::string log_path;
@@ -144,6 +144,7 @@ class FileLogSink : public absl::LogSink {
 
   std::shared_ptr<TraceOptions> opts_;
   std::string current_file_;
+  std::size_t current_file_size_;
   std::mutex log_mutex_;
   FILE* fp_ = nullptr;
 };
@@ -163,10 +164,9 @@ void UpdateTraceOption(std::optional<int> log_level,
                        std::optional<int> log_file_size,
                        std::optional<int> log_file_count);
 
-bool CanWriteToFile(std::string const& log_file, std::size_t new_log_size,
-                    std::uintmax_t max_file_size_bytes);
-
 std::string GetLogFileWithIndex(std::string const& log_path);
+std::string GetFormattedMsg(absl::LogEntry const& entry);
+
 ////////////////////////////////////////////////////////////////////
 // Additional Helper methods for validating and formatting strings
 // based on parameter types.

google/cloud/odbc/bq_driver/internal/trace_utils_test.cc

@@ -81,27 +81,6 @@ TEST(GetLogFileWithIndex, CustomLogPath) {
   EXPECT_EQ(actual, expected);
 }
 
-TEST(CanWriteToFile, AllSecnarios) {
-  std::string file_name = "sample.log";
-
-  // File doesn't exist
-  EXPECT_TRUE(CanWriteToFile(file_name, 100, 1000));
-
-  // File exist and can store more data
-  CreateTestFile(file_name, 200);
-  EXPECT_TRUE(CanWriteToFile(file_name, 100, 500));
-
-  // File exists and writing would exceed limit
-  CreateTestFile(file_name, 900);
-  EXPECT_FALSE(CanWriteToFile(file_name, 100, 900));
-
-  // File exists and exactly fills the limit
-  CreateTestFile(file_name, 800);
-  EXPECT_TRUE(CanWriteToFile(file_name, 200, 1000));
-
-  DeleteTestFile(file_name);
-}
-
 TEST(ClearOldLogFiles, WhenMaxFileCountIsOne) {
   std::string dir = std::filesystem::temp_directory_path().string();
   std::string file = dir + "/Tracetestingcountone_0.log";

google/cloud/odbc/bq_driver/internal/utils.cc

@@ -214,11 +214,7 @@ StatusRecordOr<std::shared_ptr<Sections>> ParseConfig(
                              0, KEY_READ, &key_handle);
   if (status != ERROR_SUCCESS) {
     RegCloseKey(key_handle);
-    LOG(ERROR)
-        << "ParseConfig::RegOpenKeyEx:: Can't open registry key with path: "
-        << registry_key;
-    return StatusRecord{SQLStates::k_HY000(),
-                        "Can't open registry key with path: " + registry_key};
+    return std::make_shared<Sections>();
   }
 
   TCHAR subkey_name[kMaxKeyLength];
@@ -521,13 +517,9 @@ StatusRecordOr<std::shared_ptr<Sections>> ParseConfig(
         }
       }
     }
-  } else {
-    std::string msg = "Can't open file with path: ";
-    msg.append(file_path);
-    LOG(ERROR) << "ParseConfig::ifstream:: " << msg;
-    return StatusRecord{SQLStates::k_HY000(), msg};
+    return std::make_shared<Sections>(sections);
   }
-  return std::make_shared<Sections>(sections);
+  return std::make_shared<Sections>();
 }
 
 #endif  //_WIN32

google/cloud/odbc/bq_driver/internal/utils_test.cc

@@ -193,14 +193,10 @@ TEST(Parsing, ParseConfigIncorrectPath) {
 #else
   auto sections = ParseConfig("SOFTWARE\\WOW6432Node\\ODBC\\ODBC1.INI");
 #endif
-  EXPECT_THAT(sections,
-              StatusRecordIs(SQLStates::k_HY000(),
-                             HasSubstr("Can't open registry key with path")));
 #else
   auto sections = ParseConfig("/invalid_file_name.ini");
-  EXPECT_THAT(sections, StatusRecordIs(SQLStates::k_HY000(),
-                                       HasSubstr("Can't open file")));
 #endif  // _WIN32
+  EXPECT_TRUE((*sections)->empty());
 }
 
 #ifndef _WIN32
@@ -245,8 +241,15 @@ TEST(GetPathToOdbcIni, GetEmptyPath) {
 }
 
 TEST(GetOdbcTraceConfigPath, GetDefaultPath) {
+  auto home = ::google::cloud::internal::GetEnv("GOOGLEBIGQUERYODBCINI");
+  // Need to remove the environment variable to use the default path
+  google::cloud::odbc_bigquery_client_interface::UnsetEnv(
+      "GOOGLEBIGQUERYODBCINI");
+
   std::string actual = GetOdbcTraceConfigPath();
   EXPECT_EQ(actual, "/etc/google.googlebigqueryodbc.ini");
+  google::cloud::odbc_bigquery_client_interface::SetEnv("GOOGLEBIGQUERYODBCINI",
+                                                        home);
 }
 
 TEST(GetOdbcTraceConfigPath, GetGoogleODBCIniPath) {