test(bq_driver): add e2e tracing testing by NeerajDwivedii · Pull Request #1323 · googleapis/cpp-bigquery-odbc

NeerajDwivedii · 2025-12-05T16:55:16Z

This PR fixes the logging issue that was previously not working. Logging now functions correctly across all platforms and adheres to the tracing priorities defined in the design document. Additionally, max_threads has been initialized to 0 to prevent the Windows error:
Run-Time Check Failure #3 – The variable 'max_threads' is being used without being initialized.

sachinpro · 2025-12-06T08:13:04Z

  int log_file_size;
-  int max_threads;
-  bool logging_enabled = false;
+  int max_threads = 0;


The default value of max_threads should be 8.

sachinpro · 2025-12-06T08:14:44Z

-std::string const kLogFileCount = "LogFileCount";
-std::string const kLogFileSize = "LogFileSize";
-std::string const kMaxThreadsParam = "MaxThreads";
+inline std::string const kLogLevel = "LogLevel";


Is this needed to fix the issue in this PR?
I don't understand why these have to be inline.

Yes, this is needed because CreateTraceOptionsFile is invoked multiple times during driver initialization, and in some runs the constant values end up being empty.

Why was this not an issue earlier? I don't believe kLogLevel can be empty in any case.

In any case, we should find the root cause. Lets have a call to discuss this.

it is a case of static initialization fiasco , similar to how static variables should be declared in header file but defined in implementation cc file , in this case either do that or make it inline. stack-over-flow-link

sachinpro · 2025-12-06T08:15:54Z

    options_file_ = std::shared_ptr<TraceOptions>(new TraceOptions());
  }

-  if (log_level > 0 && logging_enabled) {


Did I make some undersirable changes recently? I want to understand where we went wrong.

No, these unwanted changes were introduced in the last logging PR.

sachinpro

We need comprehensive e2e tests for tracing utilities doing something like:

Setting GOOGLEBIGQUERYODBCINI to a new folder path
Use LOG( macro with a specific testing string.
No error should be thrown whether the path is valid or not.
If the path was valid, check that the first log file was created.
If it was created, check the contents for the testing string we logged in step 2.

On top of this, we need similar test(s) for LogFileCount andLogFileSize too.

NeerajDwivedii · 2025-12-10T05:25:15Z

We need comprehensive e2e tests for tracing utilities doing something like:

Setting GOOGLEBIGQUERYODBCINI to a new folder path

Use LOG( macro with a specific testing string.

No error should be thrown whether the path is valid or not.

If the path was valid, check that the first log file was created.

If it was created, check the contents for the testing string we logged in step 2.

On top of this, we need similar test(s) for LogFileCount andLogFileSize too.

All the points listed above are now being validated in the existing CheckTraceLogFileExist test case, except for points 1 and 3, which will be addressed in a follow-up PR.

sachinpro

I have created a PR #1336 with only the implementation changes because we need to have some working version of tracing.

Lets keep this PR for adding E2E tests for all logging options using GOOGLEBIGQUERYODBCINI

shivamd-gpartner · 2025-12-12T11:05:56Z

+  }
+
+  // Should have atleast 2 files if rotation worked
+  EXPECT_LE(log_files.size(), std::stoi(log_file_count));


should have used EXPECT_EQ we need to match the same and if same number of files are not coming let's write more atleast more than 2MB(log_file_count * log_file_size) to make sure we are not creating more log file than the count

Updated the EXPECT_EQ , but kept the log file size at 1 MB. Increasing the max file size would generate more logs, which could lead to test timeouts.

shivamd-gpartner · 2025-12-12T12:47:01Z

 }

 TEST(GetOdbcTraceConfigPath, GetDefaultPath) {
+  auto home = ::google::cloud::internal::GetEnv("GOOGLEBIGQUERYODBCINI");


Please document here why this is needed

shivamd-gpartner · 2025-12-12T12:48:19Z

+  EXPECT_EQ(6, (*test_opts_file)->max_file_size);
+}
+
+#if !defined(_WIN32) && !defined(__APPLE__)


Simplify and add flag for linux only

shivamd-gpartner · 2025-12-26T06:14:04Z

 #endif  // _WIN32
 }

+void UpdateTraceConfig(std::string const& odbc_trace_config,


Maybe you can move this to commons.cc with generic behaviour to update registry entry

shivamd-gpartner · 2025-12-26T06:15:36Z

+  std::unordered_map<std::string, std::string> kv;
+  std::string line;
+
+  if (fs::exists(config_path) && fs::is_regular_file(config_path)) {


It should not check if file exists or not, it should simply update the entry , rest of the logic also seems unnecessary

shivamd-gpartner · 2025-12-26T06:15:44Z

+  if (fs::exists(config_path) && fs::is_regular_file(config_path)) {
+    std::ifstream input(odbc_trace_config);
+    ASSERT_TRUE(input.is_open());
+    std::cout << "check " << std::endl;


there should not be prints

shivamd-gpartner · 2025-12-26T06:16:22Z

+
+  fs::path log_file_path = fs::path(log_path) / "odbcdriverforbigquery_0.log";
+  log_file = log_file_path.string();
+  std::cout << "odbc_ini file " << odbc_ini_path << std::endl;


please remove prints

NeerajDwivedii · 2025-12-26T11:11:03Z

@sachinpro PR ready for review

NeerajDwivedii · 2026-01-19T07:49:50Z

@sachinpro PR is ready for review

NeerajDwivedii · 2026-02-02T06:12:08Z

@sachinpro This PR has been pending review since December. It contains required logging changes and needs to be merged.

sachinpro · 2026-04-22T09:21:13Z

@shivamd-gpartner @NeerajDwivedii Please rebase and clean this PR up.

NeerajDwivedii · 2026-04-24T06:16:37Z

+  int const log_file_count = 2;
+  int const log_file_size = 1024;
+
+  // Unique test dir


Added logic to create an individual googlebigqueryodbc.ini file and directory for log generation, preventing overlap with files used by other git checks, and ensuring they are cleaned up after validation.

NeerajDwivedii · 2026-04-24T06:35:38Z

All previous comments have already been addressed.

Here are the GHA workflow: https://github.com/googleapis/cpp-bigquery-odbc/actions/runs/24875185212

@sachinpro PR ready for review

NeerajDwivedii temporarily deployed to internal December 5, 2025 16:55 — with GitHub Actions Inactive

NeerajDwivedii requested review from sachinpro and shivamd-gpartner December 5, 2025 16:58

NeerajDwivedii marked this pull request as ready for review December 5, 2025 16:58

NeerajDwivedii requested a review from a team December 5, 2025 16:58

sachinpro reviewed Dec 6, 2025

View reviewed changes

sachinpro requested changes Dec 6, 2025

View reviewed changes

NeerajDwivedii force-pushed the fix_logging_issue branch from 716d506 to 0b792f4 Compare December 9, 2025 10:04

NeerajDwivedii temporarily deployed to internal December 9, 2025 10:04 — with GitHub Actions Inactive

NeerajDwivedii temporarily deployed to internal December 9, 2025 10:52 — with GitHub Actions Inactive

NeerajDwivedii temporarily deployed to internal December 9, 2025 10:54 — with GitHub Actions Inactive

NeerajDwivedii temporarily deployed to internal December 9, 2025 10:55 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 451a866 to 3f92d6e Compare December 9, 2025 11:19

NeerajDwivedii temporarily deployed to internal December 9, 2025 11:19 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 3f92d6e to 95d534a Compare December 9, 2025 11:36

NeerajDwivedii temporarily deployed to internal December 9, 2025 11:36 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 95d534a to b38650a Compare December 9, 2025 11:55

NeerajDwivedii temporarily deployed to internal December 9, 2025 11:55 — with GitHub Actions Inactive

sachinpro force-pushed the fix_logging_issue branch from b38650a to b573a2c Compare December 9, 2025 15:09

sachinpro temporarily deployed to internal December 9, 2025 15:09 — with GitHub Actions Inactive

sachinpro force-pushed the fix_logging_issue branch from b573a2c to 8a074f8 Compare December 9, 2025 15:54

sachinpro temporarily deployed to internal December 9, 2025 15:54 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 8a074f8 to 3c34c91 Compare December 9, 2025 16:00

NeerajDwivedii temporarily deployed to internal December 9, 2025 16:00 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 3c34c91 to 214120e Compare December 9, 2025 16:01

NeerajDwivedii temporarily deployed to internal December 9, 2025 16:01 — with GitHub Actions Inactive

NeerajDwivedii requested a review from sachinpro December 10, 2025 05:53

sachinpro requested changes Dec 11, 2025

View reviewed changes

sachinpro mentioned this pull request Dec 11, 2025

fix(bq_driver): Fix logging issue #1336

Merged

NeerajDwivedii requested a review from sachinpro December 12, 2025 05:06

NeerajDwivedii force-pushed the fix_logging_issue branch from 0efd8b8 to e46eec3 Compare December 12, 2025 05:29

NeerajDwivedii temporarily deployed to internal December 12, 2025 05:29 — with GitHub Actions Inactive

NeerajDwivedii changed the title ~~fix(bq_driver): Fix logging issue~~ test(bq_driver): add e2e tracing testing Dec 12, 2025

NeerajDwivedii force-pushed the fix_logging_issue branch from e46eec3 to 56a1984 Compare December 12, 2025 07:08

NeerajDwivedii temporarily deployed to internal December 12, 2025 07:08 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from 56a1984 to c037d30 Compare December 12, 2025 08:06

NeerajDwivedii temporarily deployed to internal December 12, 2025 08:06 — with GitHub Actions Inactive

NeerajDwivedii temporarily deployed to internal December 12, 2025 09:38 — with GitHub Actions Inactive

NeerajDwivedii force-pushed the fix_logging_issue branch from be29e54 to 61383b0 Compare December 12, 2025 10:59

NeerajDwivedii temporarily deployed to internal December 12, 2025 10:59 — with GitHub Actions Inactive

shivamd-gpartner reviewed Dec 12, 2025

View reviewed changes

shivamd-gpartner reviewed Dec 26, 2025

View reviewed changes

shivamd-gpartner approved these changes Jan 19, 2026

View reviewed changes

NeerajDwivedii mentioned this pull request Mar 13, 2026

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

Merged

NeerajDwivedii added 2 commits April 24, 2026 10:58

test(bq_driver): add e2e tracing testing

0670bf2

new changes

d2b13ec

NeerajDwivedii commented Apr 24, 2026

View reviewed changes

Conversation

NeerajDwivedii commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sachinpro Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinpro left a comment

Choose a reason for hiding this comment

Uh oh!

NeerajDwivedii commented Dec 10, 2025

Uh oh!

sachinpro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NeerajDwivedii commented Dec 26, 2025

Uh oh!

NeerajDwivedii commented Jan 19, 2026

Uh oh!

NeerajDwivedii commented Feb 2, 2026

Uh oh!

sachinpro commented Apr 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NeerajDwivedii commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NeerajDwivedii commented Dec 5, 2025 •

edited

Loading

sachinpro Dec 6, 2025 •

edited

Loading