-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Easy Logging using a config file #747
base: master
Are you sure you want to change the base?
Changes from 16 commits
7ab80fb
836a82a
c1878f3
21673d4
806fb9c
4a5ad67
4e05bc7
a79370d
be865c1
f90848d
3823568
a863b05
994228b
70bdc60
309d118
65e29f1
21a3b11
8d97f3a
577925a
6a27fe5
fdb5187
2d66b20
e480074
39d1036
1ee5015
f162232
04993e8
0f494ee
0aaea59
6bc1b92
3c4f610
e3e1b88
4b2cc16
81bf6fb
96b9b87
8ce9da6
9fcc790
1f778c0
17e1d10
5c90290
ac79374
99daf14
16fd3bf
2a80a5e
47334d0
3e80253
0d642b2
c89424a
a742893
e16a67c
05331fb
f370f9b
d0cbc0c
e020429
d351866
5cc39ae
75eb366
8651788
48d562f
79128b4
98c4924
9d54bab
a5549aa
169063c
2c285bf
9ede47a
bc61cd7
9eaadd0
6c3aa65
c022028
b03491a
0d2e977
36faf07
be2a7b3
d6a00fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,315 @@ | ||
/** | ||
* Copyright (c) 2024 Snowflake Computing | ||
*/ | ||
|
||
#include "snowflake/ClientConfigParser.hpp" | ||
#include "snowflake/Exceptions.hpp" | ||
#include "../logger/SFLogger.hpp" | ||
#include "client_config_parser.h" | ||
#include "memory.h" | ||
|
||
#include <fstream> | ||
#include <sstream> | ||
#include <set> | ||
|
||
#undef snprintf | ||
#include <boost/filesystem.hpp> | ||
|
||
#ifndef _WIN32 | ||
#include <dlfcn.h> | ||
#endif | ||
|
||
using namespace Snowflake::Client; | ||
|
||
namespace | ||
{ | ||
// constants | ||
const std::string SF_CLIENT_CONFIG_FILE_NAME("sf_client_config.json"); | ||
const std::string SF_CLIENT_CONFIG_ENV_NAME("SF_CLIENT_CONFIG_FILE"); | ||
std::set<std::string> KnownCommonEntries = {"log_level", "log_path"}; | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
void load_client_config( | ||
const char* in_configFilePath, | ||
client_config* out_clientConfig) | ||
{ | ||
ClientConfigParser configParser; | ||
ClientConfig clientConfig; | ||
configParser.loadClientConfig(in_configFilePath, clientConfig); | ||
if (!clientConfig.logLevel.empty()) | ||
{ | ||
out_clientConfig->logLevel = (char*)SF_CALLOC(1, sizeof(clientConfig.logLevel)); | ||
sf_memcpy(out_clientConfig->logLevel, sizeof(out_clientConfig->logLevel), | ||
clientConfig.logLevel.data(), clientConfig.logLevel.size()); | ||
} | ||
if (!clientConfig.logPath.empty()) | ||
{ | ||
out_clientConfig->logPath = (char*)SF_CALLOC(1, sizeof(clientConfig.logPath)); | ||
sf_memcpy(out_clientConfig->logPath, MAX_PATH, | ||
clientConfig.logPath.data(), clientConfig.logPath.size()); | ||
} | ||
} | ||
|
||
// Public ====================================================================== | ||
//////////////////////////////////////////////////////////////////////////////// | ||
ClientConfigParser::ClientConfigParser() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we change the name? Maybe EasyLoggingConfigParser |
||
{ | ||
; // Do nothing. | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
ClientConfigParser::~ClientConfigParser() | ||
{ | ||
; // Do nothing. | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
void ClientConfigParser::loadClientConfig( | ||
const std::string& in_configFilePath, | ||
ClientConfig& out_clientConfig) | ||
{ | ||
char envbuf[MAX_PATH + 1]; | ||
std::string derivedConfigPath = ""; | ||
if (!in_configFilePath.empty()) | ||
sfc-gh-jszczerbinski marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use ealy return instead of else nesting |
||
{ | ||
// 1. Try config file if it was passed in | ||
derivedConfigPath = in_configFilePath; | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "loadClientConfig", | ||
"Using client configuration path from a connection string: %s", in_configFilePath.c_str()); | ||
} | ||
else if (const char* clientConfigEnv = sf_getenv_s(SF_CLIENT_CONFIG_ENV_NAME.c_str(), envbuf, sizeof(envbuf))) | ||
{ | ||
// 2. Try environment variable SF_CLIENT_CONFIG_ENV_NAME | ||
derivedConfigPath = clientConfigEnv; | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "loadClientConfig", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use logging pattern present throughout the project: Remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please adjust other log calls too |
||
"Using client configuration path from an environment variable: %s", clientConfigEnv); | ||
} | ||
else | ||
{ | ||
// 3. Try DLL binary dir | ||
std::string binaryDir = getBinaryPath(); | ||
std::string binaryDirFilePath = binaryDir + SF_CLIENT_CONFIG_FILE_NAME; | ||
if (checkFileExists(binaryDirFilePath)) | ||
{ | ||
derivedConfigPath = binaryDirFilePath; | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "loadClientConfig", | ||
"Using client configuration path from binary directory: %s", binaryDirFilePath.c_str()); | ||
} | ||
else | ||
{ | ||
#if defined(WIN32) || defined(_WIN64) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please move finding home directory to another function. |
||
// 4. Try user home dir | ||
std::string homeDir = sf_getenv_s("USERPROFILE", envbuf, sizeof(envbuf)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sf_getenv_s could return NULL and cause runtime exception thrown.
sfc-gh-jszczerbinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (homeDir.empty()) | ||
{ | ||
// USERPROFILE is empty, try HOMEDRIVE and HOMEPATH | ||
std::string homeDriveEnv = sf_getenv_s("HOMEDRIVE", envbuf, sizeof(envbuf)); | ||
std::string homePathEnv = sf_getenv_s("HOMEPATH", envbuf, sizeof(envbuf)); | ||
if (!homeDriveEnv.empty() && !homePathEnv.empty()) | ||
{ | ||
homeDir = homeDriveEnv + homePathEnv; | ||
} | ||
} | ||
std::string homeDirFilePath = homeDir + PATH_SEP + SF_CLIENT_CONFIG_FILE_NAME; | ||
if (checkFileExists(homeDirFilePath)) | ||
{ | ||
derivedConfigPath = homeDirFilePath; | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "loadClientConfig", | ||
"Using client configuration path from home directory: %s", homeDirFilePath.c_str()); | ||
} | ||
#else | ||
// 4. Try user home dir | ||
if (const char* homeDir = sf_getenv_s("HOME", envbuf, sizeof(envbuf))) | ||
{ | ||
std::string homeDirFilePath = std::string(homeDir) + PATH_SEP + SF_CLIENT_CONFIG_FILE_NAME; | ||
if (checkFileExists(homeDirFilePath)) | ||
{ | ||
derivedConfigPath = homeDirFilePath; | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "loadClientConfig", | ||
"Using client configuration path from home directory: %s", homeDirFilePath.c_str()); | ||
} | ||
} | ||
#endif | ||
} | ||
} | ||
if (!derivedConfigPath.empty()) | ||
{ | ||
parseConfigFile(derivedConfigPath, out_clientConfig); | ||
} | ||
} | ||
|
||
// Private ===================================================================== | ||
//////////////////////////////////////////////////////////////////////////////// | ||
bool ClientConfigParser::checkFileExists(const std::string& in_filePath) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are on c++17 already maybe we could use std::filesystem::is_regular_file() directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. std::filesystem isn't available on osx10.14 and boost has a debug assertion on 32-bit windows with MT build There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we resolve debug assertion issue with 32-bit windows build and use boost filesystem There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think using boost filesystem was able to fix the issue |
||
{ | ||
FILE* file = sf_fopen(&file, in_filePath.c_str(), "r"); | ||
if (file != nullptr) | ||
{ | ||
fclose(file); | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
void ClientConfigParser::parseConfigFile( | ||
const std::string& in_filePath, | ||
ClientConfig& out_clientConfig) | ||
{ | ||
cJSON* jsonConfig; | ||
FILE* configFile; | ||
try | ||
{ | ||
configFile = sf_fopen(&configFile, in_filePath.c_str(), "r"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use std::fstream and std::filesystem APIs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That means we cannot use std::fstream at all? Can you show me the assertion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: File: minkernel\crts\ucrt\src\appcrt\lowio\read.cpp Expression: _osfile(fh) & FOPEN There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am using fstream and boost filesystem on my PR without issues There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
if (!configFile) | ||
{ | ||
CXX_LOG_INFO("sf", "ClientConfigParser", "parseConfigFile", | ||
"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()); | ||
} | ||
#if !defined(WIN32) && !defined(_WIN64) | ||
checkIfValidPermissions(in_filePath); | ||
#endif | ||
fseek(configFile, 0, SEEK_END); | ||
long length = ftell(configFile); | ||
rewind(configFile); | ||
char* buffer = (char*)malloc(length); | ||
if (buffer) | ||
{ | ||
size_t result = fread(buffer, 1, length, configFile); | ||
if (result != length) | ||
{ | ||
CXX_LOG_ERROR( | ||
"sf", | ||
"ClientConfigParser", | ||
"parseConfigFile", | ||
"Error in reading file: %s", in_filePath.c_str()); | ||
} | ||
} | ||
jsonConfig = snowflake_cJSON_Parse(buffer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use picojson in c++ code |
||
const char* error_ptr = snowflake_cJSON_GetErrorPtr(); | ||
if (error_ptr) | ||
{ | ||
CXX_LOG_ERROR( | ||
"sf", | ||
"ClientConfigParser", | ||
"parseConfigFile", | ||
"Error in parsing JSON: %s, err: %s", in_filePath.c_str(), error_ptr); | ||
std::string errMsg = "Error parsing client configuration file: " + in_filePath; | ||
throw ClientConfigException(errMsg.c_str()); | ||
} | ||
} | ||
catch (std::exception& ex) | ||
{ | ||
fclose(configFile); | ||
throw; | ||
} | ||
|
||
const cJSON* commonProps = snowflake_cJSON_GetObjectItem(jsonConfig, "common"); | ||
checkUnknownEntries(snowflake_cJSON_Print(commonProps)); | ||
const cJSON* logLevel = snowflake_cJSON_GetObjectItem(commonProps, "log_level"); | ||
if (snowflake_cJSON_IsString(logLevel)) | ||
{ | ||
out_clientConfig.logLevel = snowflake_cJSON_GetStringValue(logLevel); | ||
} | ||
const cJSON* logPath = snowflake_cJSON_GetObjectItem(commonProps, "log_path"); | ||
if (snowflake_cJSON_IsString(logPath)) | ||
{ | ||
out_clientConfig.logPath = snowflake_cJSON_GetStringValue(logPath); | ||
} | ||
fclose(configFile); | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
void ClientConfigParser::checkIfValidPermissions(const std::string& in_filePath) | ||
{ | ||
boost::filesystem::file_status fileStatus = boost::filesystem::status(in_filePath); | ||
sfc-gh-jszczerbinski marked this conversation as resolved.
Show resolved
Hide resolved
|
||
boost::filesystem::perms permissions = fileStatus.permissions(); | ||
if (permissions & boost::filesystem::group_write || | ||
permissions & boost::filesystem::others_write) | ||
{ | ||
CXX_LOG_ERROR( | ||
"sf", | ||
"ClientConfigParser", | ||
"checkIfValidPermissions", | ||
"Error due to other users having permission to modify the config file: %s", | ||
in_filePath.c_str()); | ||
std::string errMsg = "Error due to other users having permission to modify the config file: " + in_filePath; | ||
throw ClientConfigException(errMsg.c_str()); | ||
} | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
void ClientConfigParser::checkUnknownEntries(const std::string& in_jsonString) | ||
{ | ||
cJSON* jsonConfig = snowflake_cJSON_Parse(in_jsonString.c_str()); | ||
const char* error_ptr = snowflake_cJSON_GetErrorPtr(); | ||
if (error_ptr) | ||
{ | ||
CXX_LOG_ERROR( | ||
"sf", | ||
"ClientConfigParser", | ||
"checkUnknownEntries", | ||
"Error in parsing JSON: %s, err: %s", in_jsonString.c_str(), error_ptr); | ||
std::string errMsg = "Error parsing json: " + in_jsonString; | ||
throw ClientConfigException(errMsg.c_str()); | ||
} | ||
|
||
if (snowflake_cJSON_IsObject(jsonConfig)) | ||
{ | ||
cJSON* entry = NULL; | ||
snowflake_cJSON_ArrayForEach(entry, jsonConfig) | ||
{ | ||
std::string key = entry->string; | ||
bool found = false; | ||
for (std::string knownEntry : KnownCommonEntries) | ||
{ | ||
if (sf_strncasecmp(key.c_str(), knownEntry.c_str(), knownEntry.length()) == 0) | ||
{ | ||
found = true; | ||
} | ||
} | ||
if (!found) | ||
{ | ||
std::string warnMsg = | ||
"Unknown configuration entry: " + key + " with value:" + entry->valuestring; | ||
CXX_LOG_WARN( | ||
"sf", | ||
"ClientConfigParser", | ||
"checkUnknownEntries", | ||
warnMsg.c_str()); | ||
} | ||
} | ||
} | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
std::string ClientConfigParser::getBinaryPath() | ||
{ | ||
std::string binaryFullPath; | ||
#if defined(WIN32) || defined(_WIN64) | ||
std::wstring path; | ||
HMODULE hm = NULL; | ||
wchar_t appName[256]; | ||
GetModuleFileNameW(hm, appName, 256); | ||
path = appName; | ||
binaryFullPath = std::string(path.begin(), path.end()); | ||
#else | ||
Dl_info info; | ||
int result = dladdr((void*)load_client_config, &info); | ||
if (result) | ||
{ | ||
binaryFullPath = std::string(info.dli_fname); | ||
} | ||
#endif | ||
size_t pos = binaryFullPath.find_last_of(PATH_SEP); | ||
if (pos == std::string::npos) | ||
{ | ||
return ""; | ||
} | ||
std::string binaryPath = binaryFullPath.substr(0, pos + 1); | ||
return binaryPath; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.