Logging level #38
Logging level #38
Conversation
| LOG_ERR("Check failure: %s \n" M, #COND, ##__VA_ARGS__); \ | ||
| #define CHECKM(COND, M, ...) \ | ||
| if (!(COND)) { \ | ||
| LOG_ERROR("Check failure: %s \n" M, #COND, ##__VA_ARGS__); \ |
There was a problem hiding this comment.
CHECKM definitely should be fatal.
That said, do we actually want LOG_WARN, LOG_ERROR, and LOG_FATAL? It seems like either we can recover, in which case it should be LOG_WARN, or we can't, in which case it should be LOG_FATAL.
I think pretty much all of the LOG_ERRORs that we currently have should potentially be fatal errors.
There was a problem hiding this comment.
Hmm, I was just following standard logging levels found in other systems here. I think it makes sense to differentiate between LOG_WARN and LOG_ERROR. Here is a good post that talks about the different levels: link.
I'll look through the LOG_ERRORs and see which ones might need to be LOG_FATAL.
| # Set the request timeout low for testing purposes. | ||
| test: CFLAGS += -DRAY_TIMEOUT=50 | ||
| # Set the request timeout low and logging level at FATAL for testing purposes. | ||
| test: CFLAGS += -DRAY_TIMEOUT=50 -DRAY_COMMON_LOG_LEVEL=4 |
There was a problem hiding this comment.
if we keep the ERROR level, then maybe this should be ERROR. If not, the FATAL is good.
There was a problem hiding this comment.
I wanted to keep the level at FATAL because some of the tests test error scenarios, which is why we have so many log messages right now.
| #define LOG_ERROR(M, ...) \ | ||
| fprintf(stderr, "[ERROR] (%s:%d: errno: %s) " M "\n", __FILE__, __LINE__, \ | ||
| errno == 0 ? "None" : strerror(errno), ##__VA_ARGS__) | ||
| #endif |
There was a problem hiding this comment.
may have introduced some compiler warnings.
state/table.c:70:57: warning: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long') [-Wformat]
LOG_ERROR("Table command with timer ID %ld failed", timer_id);
~~~ ^~~~~~~~
%lld
./common.h:44:52: note: expanded from macro 'LOG_ERROR'
errno == 0 ? "None" : strerror(errno), ##__VA_ARGS__)
| @@ -38,6 +38,7 @@ redis: | |||
| hiredis: | |||
There was a problem hiding this comment.
I just tried running
@ray.remote
def f():
return 1
And it printed
[ERROR] (state/table.c:70: errno: Operation now in progress) Table command with timer ID 0 failed
If that is something that happens normally while Ray is functioning correctly, then there it shouldn't be an error or even a warning. It should be INFO or DEBUG, right?
stephanie-wang
force-pushed
the
log-levels
branch
from
5f7d747 to
4e88f52
Compare
Ability to set Ray's stdout logging level, using -DRAY_COMMON_LOG_LEVEL=level. The level value can be an integer from 0-4, where the levels are, in order, DEBUG, INFO, WARN, ERROR, FATAL.