Implement Easy Logging using a config file #747

sfc-gh-ext-simba-nl · 2024-10-02T19:37:18Z

For SNOW-981602: Implement Easy Logging using a config file

Add several new global attributes:
SF_GLOBAL_CLIENT_CONFIG_FILE: To set the client config file so libsnowflakeclient knows where to look for the config file
SF_GLOBAL_LOG_LEVEL: To allow the user to get the log level since libsnowflakeclient handles log setup
SF_GLOBAL_LOG_PATH: To allow the user to get the log path

include/snowflake/ClientConfigParser.hpp

…file

sfc-gh-ext-simba-hx · 2024-10-11T19:59:55Z

tests/test_unit_logger.c

@@ -25,6 +28,47 @@ void test_log_str_to_level(void **unused) {
 }

 #ifndef _WIN32
+/**


Please add a test case to cover the behavior in log_init(), e.g. having a config file in place, after calling snowflake_global_init() check the log settings are as expected.

sfc-gh-ext-simba-hx · 2024-10-11T20:01:53Z

cpp/lib/ClientConfigParser.cpp

+    {
+#if defined(WIN32) || defined(_WIN64)
+      // 4. Try user home dir
+      std::string homeDir = sf_getenv_s("USERPROFILE", envbuf, sizeof(envbuf));


sf_getenv_s could return NULL and cause runtime exception thrown.

sfc-gh-ext-simba-hx · 2024-10-11T20:06:22Z

cpp/lib/ClientConfigParser.cpp

+
+// Private =====================================================================
+////////////////////////////////////////////////////////////////////////////////
+bool ClientConfigParser::checkFileExists(const std::string& in_filePath)


We are on c++17 already maybe we could use std::filesystem::is_regular_file() directly?

std::filesystem isn't available on osx10.14 and boost has a debug assertion on 32-bit windows with MT build

Can we resolve debug assertion issue with 32-bit windows build and use boost filesystem

I think using boost filesystem was able to fix the issue

sfc-gh-ext-simba-hx · 2024-10-11T20:19:33Z

lib/client.c

+    char client_config_file[MAX_PATH] = {0};
+    snowflake_global_get_attribute(
+      SF_GLOBAL_CLIENT_CONFIG_FILE, client_config_file, sizeof(client_config_file));
+    client_config clientConfig = { .logLevel = "", .logPath = "" };


memory leak currently with the buffer allocated in load_client_config().
When you try to free the buffer, initialize with const string could be a problem.

I think if client config is not initialized .logLevel and .logPath should be null

sfc-gh-ext-simba-hx · 2024-10-11T20:21:58Z

cpp/lib/ClientConfigParser.cpp

+}
+
+////////////////////////////////////////////////////////////////////////////////
+void load_client_config(


Seems we are throwing exception from loadClientConfig(). We need to catch that and return error. C interface doesn't expect exception.
Please add error test cases as well this will be caught if we have error test cases.

sfc-gh-ext-simba-hx · 2024-10-11T20:49:27Z

lib/client.c

@@ -36,6 +37,8 @@ sf_bool DEBUG;
 sf_bool SF_OCSP_CHECK;
 char *SF_HEADER_USER_AGENT = NULL;

+static char *CLIENT_CONFIG_FILE = NULL;
+static char* LOG_LEVEL;


why not to set NULL as others? Better not to add LOG_LEVEL as we've had number loglevel maintained with log_set_level() and log_get_level() already. Having another one could cause mismatch.

sfc-gh-ext-simba-hx · 2024-10-11T20:54:23Z

lib/client.c

+        case SF_GLOBAL_CLIENT_CONFIG_FILE:
+            alloc_buffer_and_copy(&CLIENT_CONFIG_FILE, value);
+            break;
+        case SF_GLOBAL_LOG_LEVEL:


I would prefer SF_GLOBAL_LOG_PATH and SF_GLOBAL_LOG_LEVEL to be read only attributes since application already have enough ways to do log settings.

sfc-gh-ext-simba-hx · 2024-10-11T21:13:07Z

include/snowflake/client.h

-    SF_GLOBAL_OCSP_CHECK
+    SF_GLOBAL_OCSP_CHECK,
+    SF_GLOBAL_CLIENT_CONFIG_FILE,
+    SF_GLOBAL_LOG_LEVEL,


remind me why we need SF_GLOBAL_LOG_LEVEL and SF_GLOBAL_LOG_PATH?
Please having a brief note of the interface and usage in either the ticket or the PR description. That could help understanding the implementation and later for documentation as well.

Updated description

cpp/lib/ClientConfigParser.cpp

sfc-gh-jszczerbinski · 2024-10-14T08:04:41Z

cpp/lib/ClientConfigParser.cpp

+  FILE* configFile;
+  try 
+  {
+    configFile = sf_fopen(&configFile, in_filePath.c_str(), "r");


Use std::fstream and std::filesystem APIs?

Unfortunately std::filesystem isn't in osx10.14 yet so it can't be used and there seems to be a 32-bit windows debug assertion issue with reading file with fstream when built with MT

That means we cannot use std::fstream at all? Can you show me the assertion?

Not exactly, it only happens for Windows 32-bit built with MT. All other configurations seem to work fine. If you prefer, I can ifdef it to not run when it's windows debug and it should work fine. The assertion says something like this:
Debug Assertion Failed!

File: minkernel\crts\ucrt\src\appcrt\lowio\read.cpp

Expression: _osfile(fh) & FOPEN

Is this local issue? Does it appear on CI? I think you might be building the project incorrectly. See: https://stackoverflow.com/questions/5984144/assertion-error-in-crt-calling-osfile-in-vs-2008/6010756#6010756

I am using fstream and boost filesystem on my PR without issues

I'm still running into this issue with the tests run on jenkins. Basically the test run just hangs after it hits the assertion error. In the mean time, I've added an ifdef to to run easy logging when in 32-bit debug. I've locally tested running on /MDd and it runs fine, but currently I think we build with dynamic runtime set to OFF

cpp/lib/ClientConfigParser.cpp

sfc-gh-jszczerbinski · 2024-11-25T11:35:39Z

cpp/lib/ClientConfigParser.cpp

+    CXX_LOG_INFO("Using client configuration path from a connection string: %s", in_configFilePath.c_str());
+    return in_configFilePath;
+  }
+  else if (const char* clientConfigEnv = sf_getenv_s(SF_CLIENT_CONFIG_ENV_NAME.c_str(), envbuf, sizeof(envbuf)))


Else no longer needed when we do an early return.

sfc-gh-jszczerbinski · 2024-11-25T11:35:42Z

cpp/lib/ClientConfigParser.cpp

+    CXX_LOG_INFO("Using client configuration path from an environment variable: %s", clientConfigEnv);
+    return clientConfigEnv;
+  }
+  else


Else no longer needed when we do an early return.

sfc-gh-jszczerbinski · 2024-11-25T11:39:57Z

cpp/lib/ClientConfigParser.cpp

+      throw ClientConfigException(errMsg.c_str());
+    }
+  }
+  catch (std::exception& e)


We don't need to explicitly close the file. It will close automatically when destructor of ifstream is called:

https://en.cppreference.com/w/cpp/language/raii

https://en.cppreference.com/w/cpp/io/basic_ifstream (destructor doc)

Right now we catch and rethrow the exception, it makes no sense.

sfc-gh-jszczerbinski · 2024-11-28T16:59:15Z

cpp/lib/ClientConfigParser.cpp

+      throw ClientConfigException(errMsg.c_str());
+    }
+  }
+  catch (std::exception& e)


Right now we catch and rethrow the exception, it makes no sense.

sfc-gh-jszczerbinski · 2024-11-28T17:03:22Z

cpp/lib/ClientConfigParser.cpp

+      CXX_LOG_INFO("Could not open a file. The file may not exist: %s",
+        in_filePath.c_str());
+      std::string errMsg = "Error finding client configuration file: " + in_filePath;
+      throw ClientConfigException(errMsg.c_str());


Please avoid using exceptions (https://google.github.io/styleguide/cppguide.html#Exceptions)

sfc-gh-jszczerbinski · 2024-11-28T17:06:46Z

cpp/lib/ClientConfigParser.cpp

+  } else {
+    // USERPROFILE is empty, try HOMEDRIVE and HOMEPATH
+    char* homeDriveEnv = sf_getenv_s("HOMEDRIVE", envbuf, sizeof(envbuf));
+    char* homePathEnv = sf_getenv_s("HOMEPATH", envbuf, sizeof(envbuf));


I think we overwrite homePathEnv here. envbuf is used twice.

Please add tests for resolveHomeDirConfigPath

sfc-gh-jszczerbinski

In general looks good to me. We need to log and return false if contents of of the config are malformed;

sfc-gh-jszczerbinski · 2024-12-11T16:19:30Z

cpp/lib/ClientConfigParser.cpp

+      sf_strcpy(out_clientConfig.logPath, logPathSize, logPath);
+    }
+  }
+  return true;


I assume we should return true iff we successfully parsed the config, but if json doesn't have expected format (doesn't define log_level, "common" field is not an object etc.) we still return true.

The reason it still returns true is because if the user passes the log level but not log path to log_init() and the config file only has log path, we would still like the logger to log to the log path set in the config (and vice versa).

sfc-gh-jszczerbinski · 2024-12-11T16:20:36Z

cpp/lib/ClientConfigParser.cpp

+    return false;
+  }
+
+  value commonProps = jsonConfig.get("common");


What if "common" does not exist in object or top-level is not an object?

Added checks before and after

sfc-gh-jszczerbinski · 2024-12-11T16:43:46Z

tests/test_unit_logger.c

+ * Tests log settings from client config file with invalid json
+ */
+void test_client_config_log_invalid_json(void** unused) {
+  char clientConfigJSON[] = "{{{\"invalid json\"}";


What about tests when we have configs like [], { "log_level": "warn" } etc. ?

Added several new test cases for this

…file

sfc-gh-jszczerbinski

In general looks good to me.

sfc-gh-dprzybysz · 2024-12-20T10:49:15Z

cpp/lib/ClientConfigParser.cpp

+  client_config* out_clientConfig)
+{
+// Disable easy logging for 32-bit windows debug build due to linking issues
+// with _osfile causing hanging/assertions until dynamic linking is available


does it mean that with static library the EasyLogging feature is not available?

Correct, but only with 32-bit windows debug build, all other configurations/platforms/bitness work fine

sfc-gh-dprzybysz · 2024-12-20T10:57:57Z

cpp/lib/ClientConfigParser.cpp

+
+////////////////////////////////////////////////////////////////////////////////
+sf_bool EasyLoggingConfigParser::loadClientConfig(
+  const std::string& in_configFilePath,


do we need in_ out_ prefixes?

sfc-gh-dprzybysz · 2024-12-20T10:59:33Z

cpp/lib/ClientConfigParser.cpp

+    return clientConfigEnv;
+  }
+
+  // 3. Try DLL binary dir


for not DLL can we just skip the check?
or maybe we should skip checking this location at all - we should more look in the place from which the user application was running, not the dll location?

This was just ported from ODBC. Is this something we should change for libsnowflakeclient?

sfc-gh-dprzybysz · 2024-12-20T11:03:55Z

tests/test_unit_logger.c

-    return cmocka_run_group_tests(tests, NULL, NULL);
-}
+/*
+ * Copyright (c) 2018-2019 Snowflake Computing, Inc. All rights reserved.


2024 or 2025 if it will be merged next year ;)

…file

sfc-gh-ext-simba-nl added 14 commits October 2, 2024 12:35

Implement easy logging

7ab80fb

Add missing function definition

836a82a

Fix linux build errors

c1878f3

Fix conversion issue on linux

21673d4

Fix mac build error and fix logging test failure

806fb9c

Fix mac build error in test case

4a5ad67

Fix setting of global log path

4e05bc7

Use sf_memcpy instead

a79370d

Remove the use of boost algorithm string for compare

be865c1

Fix test issue

f90848d

Fix build issue with test

3823568

Change ifstream to FILE due to 32bit debug read issue

a863b05

Fix linux build issue

994228b

Fix test issues

70bdc60

sfc-gh-ext-simba-hx reviewed Oct 10, 2024

View reviewed changes

include/snowflake/ClientConfigParser.hpp Outdated Show resolved Hide resolved

Rename ifndef for headers to be more consistent

309d118

sfc-gh-ext-simba-nl marked this pull request as ready for review October 11, 2024 16:43

sfc-gh-ext-simba-nl requested a review from a team as a code owner October 11, 2024 16:43

Merge branch 'master' into SNOW-981602-implement-easy-logging-config-…

65e29f1

…file

sfc-gh-ext-simba-hx reviewed Oct 11, 2024

View reviewed changes

sfc-gh-jszczerbinski requested changes Oct 14, 2024

View reviewed changes

sfc-gh-ext-simba-nl added 2 commits October 16, 2024 15:57

Replace boost with std::filesystem, fix possible NULL issues, add tests

21a3b11

Add client_config_parser to include dir

8d97f3a

sfc-gh-ext-simba-nl added 8 commits November 15, 2024 14:57

Use picojson instead of cjson, refactor client config parser

9eaadd0

Add exception to keep track of errors

6c3aa65

Revert accidental inclusions from master-2.0.0 branch

c022028

Add back missing files, fix styling change

b03491a

Disable test case for windows 32-bit debug

0d2e977

Ignore client config loading on win32 debug

36faf07

Add comment explaining the ifdef

be2a7b3

Fix log message

d6a00fe

sfc-gh-ext-simba-nl requested a review from sfc-gh-jszczerbinski November 22, 2024 17:19

sfc-gh-jszczerbinski requested changes Nov 25, 2024

View reviewed changes

Remove unncessary else clauses and removed ifstream.close

726b897

sfc-gh-ext-simba-nl requested a review from sfc-gh-jszczerbinski November 26, 2024 00:31

sfc-gh-jszczerbinski requested changes Nov 28, 2024

View reviewed changes

Remove exceptions, add test case, fix home env bug

a6e6ca0

sfc-gh-ext-simba-nl requested a review from sfc-gh-jszczerbinski November 29, 2024 17:25

sfc-gh-jszczerbinski requested changes Dec 11, 2024

View reviewed changes

sfc-gh-ext-simba-nl added 4 commits December 11, 2024 10:38

Merge branch 'master' into SNOW-981602-implement-easy-logging-config-…

301d333

…file

Add test cases, fix config parsing return when json is malformed

21f7b13

Fix code quality

31001f3

Remove client config logger tests for Windows 32-bit debug

905bc6d

sfc-gh-ext-simba-nl requested a review from sfc-gh-jszczerbinski December 12, 2024 01:20

sfc-gh-jszczerbinski and others added 3 commits December 12, 2024 14:15

Merge branch 'master' into SNOW-981602-implement-easy-logging-config-…

1d2512c

…file

Merge branch 'master' into SNOW-981602-implement-easy-logging-config-…

358b310

…file

Merge master branch

c220e29

sfc-gh-jszczerbinski reviewed Dec 19, 2024

View reviewed changes

sfc-gh-dprzybysz reviewed Dec 20, 2024

View reviewed changes

sfc-gh-ext-simba-nl added 2 commits December 20, 2024 09:21

Merge branch 'master' into SNOW-981602-implement-easy-logging-config-…

44786f6

…file

Remove in_ and out_ from function parameters, update copyright date

6a53849

sfc-gh-ext-simba-nl requested review from sfc-gh-dprzybysz and sfc-gh-jszczerbinski December 21, 2024 01:38

Implement Easy Logging using a config file #747

Are you sure you want to change the base?

Implement Easy Logging using a config file #747

Conversation

sfc-gh-ext-simba-nl commented Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-hx Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-hx Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

sfc-gh-ext-simba-hx Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-hx Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-nl Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-jszczerbinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-jszczerbinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-ext-simba-nl commented Oct 2, 2024 •

edited

Loading

sfc-gh-ext-simba-hx Oct 11, 2024 •

edited

Loading

sfc-gh-ext-simba-hx Oct 11, 2024 •

edited

Loading

sfc-gh-ext-simba-hx Oct 11, 2024 •

edited

Loading

sfc-gh-ext-simba-hx Oct 11, 2024 •

edited

Loading

sfc-gh-ext-simba-nl Oct 17, 2024 •

edited

Loading