-
-
Notifications
You must be signed in to change notification settings - Fork 84
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
fix: log runtime output to env var "BPFTIME_LOG_OUTPUT" #301
Changes from all commits
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,73 @@ | ||
#include <string> | ||
#include "spdlog/spdlog.h" | ||
#include "spdlog/cfg/env.h" | ||
#include "spdlog/sinks/rotating_file_sink.h" | ||
#include "spdlog/sinks/stdout_color_sinks.h" | ||
#include <cstdlib> | ||
#include <iostream> | ||
#include <filesystem> | ||
#include <fstream> | ||
|
||
namespace bpftime | ||
{ | ||
|
||
inline std::string expand_user_path(const std::string &input_path) | ||
{ | ||
if (input_path.empty()) { | ||
return "console"; | ||
} | ||
|
||
if (input_path[0] == '~') { | ||
const char *homeDir = getenv("HOME"); | ||
if (!homeDir) { | ||
return "console"; | ||
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. Will string "console" be parsed as a path? 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. Yes, to stderr. The console log output will also be used in CI 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 mean, if user want to print things to a file named 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 might still be print to the console...Any suggestion for handling this case? 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. For example, use std::optional as the return type of this function. If $HOME is not set, returning empty optional |
||
} | ||
|
||
if (input_path.size() == 1 || input_path[1] == '/') { | ||
// Replace "~" with the home directory | ||
std::string expandedPath = homeDir + | ||
input_path.substr(1); | ||
return expandedPath; | ||
} else { | ||
return "console"; // Unsupported path format | ||
} | ||
} | ||
|
||
// Return the original path if no tilde expansion is needed | ||
return input_path; | ||
} | ||
|
||
inline void bpftime_set_logger(const std::string &target) noexcept | ||
{ | ||
std::string logger_target = expand_user_path(target); | ||
|
||
if (logger_target == "console") { | ||
// Set logger to stderr | ||
auto logger = spdlog::stderr_color_mt("stderr"); | ||
logger->set_pattern("[%Y-%m-%d %H:%M:%S][%^%l%$][%t] %v"); | ||
logger->flush_on(spdlog::level::info); | ||
spdlog::set_default_logger(logger); | ||
} else { | ||
// Set logger to file, with rotation 5MB and 3 files | ||
auto max_size = 1048576 * 5; | ||
auto max_files = 3; | ||
auto logger = spdlog::rotating_logger_mt( | ||
"bpftime_logger", logger_target, max_size, max_files); | ||
logger->set_pattern("[%Y-%m-%d %H:%M:%S][%^%l%$][%t] %v"); | ||
logger->flush_on(spdlog::level::info); | ||
spdlog::set_default_logger(logger); | ||
} | ||
|
||
// Load log level from environment | ||
spdlog::cfg::load_env_levels(); | ||
} | ||
|
||
/* | ||
Flush the logger. | ||
*/ | ||
inline void bpftime_logger_flush() | ||
{ | ||
spdlog::default_logger()->flush(); | ||
} | ||
|
||
} // namespace bpftime |
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.
Avoid using #define to define constants in C++
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.
Ok, I can fix that in next pr
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.
The problem is that it might be input by the cmake... is it better to
I think the old one is a straightforward and clean way to ensure that a default value is set if none is provided.