-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Core] Ray c++ backend structured logging #44468
Conversation
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
src/ray/util/logging.cc
Outdated
logger->log(GetMappedSeverity(severity_), | ||
/*fmt*/ ",\"{}\":\"{}\"{}", | ||
kLogKeyMessage, | ||
json_escape_string(msg_osstream_.str()), |
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.
can we use nlohmann/json to dump a json string instead of doing string concats and formats on our own?
src/ray/util/logging.cc
Outdated
if (severity_ == RayLogLevel::FATAL) { | ||
std::_Exit(EXIT_FAILURE); | ||
} | ||
} | ||
|
||
RayLog &RayLog::WithField(const std::string &key, const std::string &value) { | ||
if (log_format_json_) { | ||
context_osstream_ << ",\"" << key << "\":\"" << json_escape_string(value) << "\""; |
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.
It's possible for user to provide a same key multiple times, resulting in a json with duplicated keys. It's technically legal but I don't think we want it. To avoid this, maybe we can do this:
- in RayLog members, remove context_osstream_ and instead keep a map<str, str> fields_
- in WithField, add to that fields_ (overwriting existing keys)
- in ~RayLog, format the map in json or text mode, a la
#include <nlohmann/json.hpp>
#include "absl/strings/str_join.h"
std::string formatMap(const std::map<std::string, std::string>& map, bool jsonMode) {
if (jsonMode) {
nlohmann::json j(map);
return j.dump();
} else {
return absl::StrJoin(map, " ", absl::PairFormatter("="));
}
}
src/ray/util/logging.cc
Outdated
if (severity_ == RayLogLevel::FATAL) { | ||
std::_Exit(EXIT_FAILURE); | ||
} | ||
} | ||
|
||
RayLog &RayLog::WithField(const std::string &key, const std::string &value) { |
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.
This API requires all fields happen before any text strings, for which I am fine, though it may introduce some inconveniences when we have some optional outputs.
However it also only accepts strings, which seems to be a big deal, since it forces a base_id.Hex()
call. What about:
template <typename T>
RayLog &RayLog::WithField(std::string_view key, const T &value) {
std::stringstream ss;
ss << value;
// put ss.str() to field_ or context_osstream_...
}
src/ray/util/logging.h
Outdated
constexpr char kLogKeyWorkerID[] = "worker_id"; | ||
constexpr char kLogKeyNodeID[] = "node_id"; | ||
constexpr char kLogKeyActorID[] = "actor_id"; | ||
constexpr char kLogKeyTaskID[] = "task_id"; |
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.
what about:
constexpr std::string_view
And, are these keys pre-defined? Can/Should users use custom keys?
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.
Users can provide custom keys. These are just common pre-defined keys that we will likely index.
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
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.
LGTM with nits
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]>
Signed-off-by: Jiajun Yao <[email protected]> Signed-off-by: Richard Liu <[email protected]>
Why are these changes needed?
Introduced RAY_BACKEND_LOG_JSON=1 to control whether to emit backend c++ log in plain text or json.
To add contextual information to the log, use
RAY_LOG(INFO).WithField("key1", "value1").WithField("key2", "value2") << "normal message"
(This is the same as the existing RAY_EVENT)Examples:
JSON format:
TEXT format:
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.