-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fixed:Log level does not work on V2 logs #4300
base: 6.0release
Are you sure you want to change the base?
Conversation
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.
I don't think this PR make sense.
trunk/src/app/srs_app_utility.cpp
Outdated
return SrsLogLevelWarn; | ||
} else if ("error" == level) { | ||
} else if ("ERROR" == level) { |
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.
I think this PR don't make sense.
The SRS_LOG_LEVEL_V2 is enabled by default.
Line 71 in 93cba24
SRS_LOG_LEVEL_V2=YES |
The SRS_LOG_LEVEL_V2
, which means the log level names defined in https://github.com/apache/logging-log4j2/blob/2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java
or
https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209
This func is just a bridge between log level enum, defined by SRS, and srs.conf
. So it's not necessary to redefine another SrsLogLevelXXX
for log level 2.
The only fault of this func is log level fatal
is missing, which can be mapping to SrsLogLevelError
anyway.
If the level
string are upper cases, the srs.conf
and Unit test cases need to change to match this change, so I don't think its a good idea also.
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.
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.
upper cases terms, TRACE, DEBUG, INFO, WARN, ERROR, are print to logs, not the configurable claims, use lower-cases in conf file.
trunk/src/kernel/srs_kernel_log.cpp
Outdated
@@ -14,7 +14,7 @@ | |||
const char* srs_log_level_strings[] = { | |||
#ifdef SRS_LOG_LEVEL_V2 | |||
// The v2 log level specs by log4j. | |||
"FORB", "TRACE", "DEBUG", NULL, "INFO", NULL, NULL, NULL, | |||
"FORB", "VERB", "TRACE", "DEBUG", "INFO", NULL, NULL, NULL, |
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.
https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209
https://github.com/apache/logging-log4j2/blob/2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java
VERB
is not the term defined by log level v2.
And the log level sequence is incorrect.
srs_log_level_strings[1] = TRACE;
srs_log_level_strings[2] = DEBUG;
srs_log_level_strings[4] = INFO;
srs_log_level_strings[8] = WARN;
srs_log_level_strings[16] = ERROR;
srs_log_level_strings[32] = FATAL;
FATAL
is not there, instead a [32] = OFF, maybe it is the only arguable point.
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
trunk/src/kernel/srs_kernel_log.hpp
Outdated
@@ -20,20 +20,36 @@ | |||
|
|||
// The log level, see https://github.com/apache/logging-log4j2/blob/release-2.x/log4j-api/src/main/java/org/apache/logging/log4j/Level.java | |||
// Please note that the enum name might not be the string, to keep compatible with previous definition. | |||
#ifdef SRS_LOG_LEVEL_V2 |
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.
SrsLogLevel
is SRS's internal log level enum.
SRS_LOG_LEVEL_V2
controls the log print with the new standard, ref https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels/2031209#2031209
then the log analysis tools can compatible with those new log level keywords.
So, redefine SrsLogLevel
don't make sense here.
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
Do not replace spaces with tabs; it is uncomfortable to look at.
|
No description provided.